Re: [Tarantool-patches] [PATCH v2] Add some cancellation guard

[lvasiliev <lvasiliev@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?
>