From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id F126E469719 for ; Wed, 18 Mar 2020 12:00:56 +0300 (MSK) References: <0e7fddec984120e36c098cd546ecae4fddc726cd.1577346185.git.lvasiliev@tarantool.org> <20200316112356.GA27301@uranus> From: lvasiliev Message-ID: <237e38c3-3d92-6b2b-6e7e-e6d8f7ceab96@tarantool.org> Date: Wed, 18 Mar 2020 12:00:55 +0300 MIME-Version: 1.0 In-Reply-To: <20200316112356.GA27301@uranus> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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: Cyrill Gorcunov Cc: tarantool-patches@dev.tarantool.org Hi! Thank you for the review. diff --git a/src/lib/core/cbus.c b/src/lib/core/cbus.c index d3566bc..a2f6ffa 100644 --- a/src/lib/core/cbus.c +++ b/src/lib/core/cbus.c @@ -132,6 +132,13 @@ cbus_endpoint_poison_f(struct cmsg *msg) void cpipe_destroy(struct cpipe *pipe) { + /* + * The thread should not be canceled while mutex is locked. + * And everything else must be protected for consistency. + */ + int old_cancel_state; + tt_pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &old_cancel_state); + ev_async_stop(pipe->producer, &pipe->flush_input); static const struct cmsg_hop route[1] = { @@ -145,12 +152,6 @@ cpipe_destroy(struct cpipe *pipe) 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); - - /* * Avoid the general purpose cpipe_push_input() since * we want to control the way the poison message is * delivered. On 16.03.2020 14:23, 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-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? >