From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-f66.google.com (mail-lf1-f66.google.com [209.85.167.66]) (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 645B1469719 for ; Mon, 16 Mar 2020 14:23:59 +0300 (MSK) Received: by mail-lf1-f66.google.com with SMTP id j11so13688210lfg.4 for ; Mon, 16 Mar 2020 04:23:59 -0700 (PDT) Date: Mon, 16 Mar 2020 14:23:56 +0300 From: Cyrill Gorcunov Message-ID: <20200316112356.GA27301@uranus> References: <0e7fddec984120e36c098cd546ecae4fddc726cd.1577346185.git.lvasiliev@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <0e7fddec984120e36c098cd546ecae4fddc726cd.1577346185.git.lvasiliev@tarantool.org> 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 Cc: tarantool-patches@dev.tarantool.org 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-stucks > > --- > 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?