From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 27 Dec 2018 16:11:44 +0300 From: Vladimir Davydov Subject: Re: [tarantool-patches] [PATCH v3 2/3] box: implement on_shutdown triggers Message-ID: <20181227131144.fivm2d65bcpfeyw5@esperanza> References: <33b51ca5edbeb8bce134d62bff83e02faaa936e9.1544533138.git.sergepetrenko@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <33b51ca5edbeb8bce134d62bff83e02faaa936e9.1544533138.git.sergepetrenko@tarantool.org> To: Serge Petrenko Cc: kostja@tarantool.org, tarantool-patches@freelists.org List-ID: On Tue, Dec 11, 2018 at 04:24:39PM +0300, Serge Petrenko wrote: > From: Konstantin Osipov > > @sergepetrenko: > Instead of running on_shutdown trigggers in box_free() execute them in a > fiber which breaks event loop before the break. > In tarantool_free() move box_free() back after coio_shutdown(). > Otherwise tarantool hangs on shutdown occasionally. Please reset the author of this patch - you've rewritten a big chunk of it already. Also, without patch 3 this one doesn't make much sense. Let's please squash them. Actually, I'd remove atexit() rework altogether (because I don't see any point in it at this point) and squash all three patches. > > Part of #1607 > --- > src/box/box.cc | 8 ++++++++ > src/box/box.h | 11 +++++++++++ > src/main.cc | 16 ++++++++++++++++ > 3 files changed, 35 insertions(+) > > diff --git a/src/box/box.cc b/src/box/box.cc > index 9642364f6..72119eca1 100644 > --- a/src/box/box.cc > +++ b/src/box/box.cc > @@ -79,6 +79,8 @@ static char status[64] = "unknown"; > /** box.stat rmean */ > struct rmean *rmean_box; > > +struct rlist on_shutdown = RLIST_HEAD_INITIALIZER(on_shutdown); > + Let's call it box_on_shutdown, because it's declared in box.h. > static void title(const char *new_status) > { > snprintf(status, sizeof(status), "%s", new_status); > @@ -1671,6 +1673,12 @@ box_set_replicaset_uuid(const struct tt_uuid *replicaset_uuid) > diag_raise(); > } > > +void > +box_run_on_shutdown_triggers(void) > +{ > + trigger_run(&on_shutdown, NULL); > +} > + > void > box_free(void) > { > diff --git a/src/box/box.h b/src/box/box.h > index cb9a512be..e9c14e5cf 100644 > --- a/src/box/box.h > +++ b/src/box/box.h > @@ -64,6 +64,11 @@ struct vclock; > */ > extern const struct vclock *box_vclock; > > +struct trigger; Pointless forward declaration. > + > +/** Invoked on box shutdown. */ > +extern struct rlist on_shutdown; > + > /* > * Initialize box library > * @throws C++ exception > @@ -84,6 +89,12 @@ box_free(void); > void > box_shutdown_wal(void); > > +/** > + * Run on_shutdown triggers. > + */ > +void > +box_run_on_shutdown_triggers(void); > + > /** > * Load configuration for box library. > * Panics on error. > diff --git a/src/main.cc b/src/main.cc > index 2240c564a..8bcc785d5 100644 > --- a/src/main.cc > +++ b/src/main.cc > @@ -124,6 +124,11 @@ static int > on_exit_f(va_list ap) > { > (void) ap; > + /* > + * run on_shutdown triggers before event loop break, > + * so that we are able to yield in them. > + */ > + box_run_on_shutdown_triggers(); > /* Terminate the main event loop. */ > ev_break(loop(), EVBREAK_ALL); What about using a trigger to terminate the loop? Then you could neatly hide all the logic in box_run_on_shutdown(). > return 0; > @@ -132,6 +137,17 @@ on_exit_f(va_list ap) > void > tarantool_exit(void) > { > + static volatile sig_atomic_t num_calls = 0; > + /* > + * We are already running on_shutdown triggers, > + * and will exit as soon as they'll finish. > + * Do not execute them twice. > + */ > + if (num_calls > 0) > + return; > + > + ++num_calls; > + I don't think we need sig_atomic_t here - it's a libev signal handler, which is always executed in the same thread. Anyway, IMO this check should be done by box_run_on_shutdown(), similarly to how box_checkpoint() handles concurrent executions. > struct fiber *f = fiber_new("on_shutdown", on_exit_f); > if (f == NULL) { > say_warn("failed to allocate a fiber to run shutdown routines.");