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

[Cyrill Gorcunov <gorcunov@gmail.com>]

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?