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

[Georgy Kirichenko <kirichenkoga@gmail.com>]

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.