From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-f193.google.com (mail-lj1-f193.google.com [209.85.208.193]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id CFE2E469719 for ; Tue, 17 Mar 2020 21:49:16 +0300 (MSK) Received: by mail-lj1-f193.google.com with SMTP id w1so24187055ljh.5 for ; Tue, 17 Mar 2020 11:49:16 -0700 (PDT) From: Georgy Kirichenko Date: Tue, 17 Mar 2020 21:49:14 +0300 Message-ID: <11490847.O9o76ZdvQC@localhost> In-Reply-To: <20200316112356.GA27301@uranus> References: <0e7fddec984120e36c098cd546ecae4fddc726cd.1577346185.git.lvasiliev@tarantool.org> <20200316112356.GA27301@uranus> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [Tarantool-patches] [PATCH v2] Add some cancellation guard List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Leonid Vasiliev , tarantool-patches@dev.tarantool.org On Monday, March 16, 2020 2:23:56 PM MSK Cyrill Gorcunov wrote: > On Thu, Dec 26, 2019 at 10:46:38AM +0300, Leonid Vasiliev wrote: > > We need to set a thread cancellation guard, because > > another thread may cancel the current thread at a > > really bad time (messages flush, mutex lock) > > > > Fixes: #4127 > > > > https://github.com/tarantool/tarantool/issues/4127 > > https://github.com/tarantool/tarantool/tree/lvasiliev/gh-4127-WAL-thread-s > > tucks > > > > --- > > > > src/lib/core/cbus.c | 22 +++++ > > src/tt_pthread.h | 5 + > > test/unit/CMakeLists.txt | 5 + > > test/unit/cbus_hang.c | 187 +++++++++++++++++++++++++++++++++++++ > > test/unit/cbus_hang.result | 4 + > > 5 files changed, 223 insertions(+) > > create mode 100644 test/unit/cbus_hang.c > > create mode 100644 test/unit/cbus_hang.result > > > > diff --git a/src/lib/core/cbus.c b/src/lib/core/cbus.c > > index b3b1280e7..d3566bc3a 100644 > > --- a/src/lib/core/cbus.c > > +++ b/src/lib/core/cbus.c > > @@ -143,6 +143,13 @@ cpipe_destroy(struct cpipe *pipe) > > > > struct cmsg_poison *poison = malloc(sizeof(struct cmsg_poison)); > > cmsg_init(&poison->msg, route); > > poison->endpoint = pipe->endpoint; > > > > + > > + /* > > + * The thread should not be canceled while mutex is locked > > + */ > > + int old_cancel_state; > > + tt_pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &old_cancel_state); > > Don't we need to disable cancel at the entry of this cpipe_destroy? > Thus trigger_destroy would be guarantee to finish, or it is not > that important in which moment we disable cancelling? > > Other than that the patch looks ok to me, still the overall cbus > code looks a bit fishy. Do we really need these threads to be > cancellable? A relay thread is canceled instead of gracefully exit. Despite relay exits issue is addressed by my replication-form-memory patch I think this patch is useful.