From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Wed, 9 Jan 2019 13:14:49 +0300 From: Vladimir Davydov Subject: Re: [PATCH v4 2/2] box: implement on_shutdown triggers Message-ID: <20190109101449.2325eebtrj7xsdlr@esperanza> References: <9dcaebf07a3018f46968748a8df04007b36ba4d6.1546090754.git.sergepetrenko@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <9dcaebf07a3018f46968748a8df04007b36ba4d6.1546090754.git.sergepetrenko@tarantool.org> To: Serge Petrenko Cc: kostja@tarantool.org, tarantool-patches@freelists.org List-ID: On Sat, Dec 29, 2018 at 04:45:40PM +0300, Serge Petrenko wrote: > Add on_shutdown triggers which are run by a preallocated fiber on > shutdown and make it possible to register them via box.ctl.on_shutdown() > Make use of the new triggers: now dedicate an on_shutdown trigger to > break event loop instead of doing it explicitly from signal handler. > The trigger is run last, so that all other on_shutdown triggers may > yield, sleep and so on. > Also make sure we can register lbox_triggers without push_event function > in case we don't need one. > > Closes #1607 > > @TarantoolBot document > Title: Document box.ctl.on_shutdown triggers > on_shutdown triggers may be set similar to space:on_replace triggers: > ``` > box.ctl.on_shutdown(new_trigger, old_trigger) > ``` > The triggers will be run when tarantool exits due to receiving one of > the signals: `SIGTERM`, `SIGINT`, `SIGHUP`. > > Note that the triggers will not be run if you exit tarantool by typing > `os.exit()` to the lua console or if tarantool receives a fatal signal: > `SIGSEGV`, `SIGABORT` or any signal causing immediate program > termination. > --- > src/box/box.cc | 2 ++ > src/box/box.h | 3 ++ > src/box/lua/ctl.c | 8 +++++ > src/lua/trigger.c | 5 ++- > src/main.cc | 51 ++++++++++++++++++++++++++++-- > test/box/misc.result | 72 ++++++++++++++++++++++++++++++++++++++++++ > test/box/misc.test.lua | 32 +++++++++++++++++++ > 7 files changed, 170 insertions(+), 3 deletions(-) > > diff --git a/src/box/box.cc b/src/box/box.cc > index 9642364f6..049ed234f 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 box_on_shutdown = RLIST_HEAD_INITIALIZER(box_on_shutdown); > + > static void title(const char *new_status) > { > snprintf(status, sizeof(status), "%s", new_status); > diff --git a/src/box/box.h b/src/box/box.h > index cb9a512be..16adc4135 100644 > --- a/src/box/box.h > +++ b/src/box/box.h > @@ -64,6 +64,9 @@ struct vclock; > */ > extern const struct vclock *box_vclock; > > +/** Invoked on box shutdown. */ > +extern struct rlist box_on_shutdown; > + Nit: it would be nice to say here that it is only invoked upon receiving a terminating signal, not when os.exit() is called. > /* > * Initialize box library > * @throws C++ exception > diff --git a/src/box/lua/ctl.c b/src/box/lua/ctl.c > index 9a105ed5c..7010be138 100644 > --- a/src/box/lua/ctl.c > +++ b/src/box/lua/ctl.c > @@ -37,6 +37,7 @@ > #include > > #include "lua/utils.h" > +#include "lua/trigger.h" > > #include "box/box.h" > > @@ -64,9 +65,16 @@ lbox_ctl_wait_rw(struct lua_State *L) > return 0; > } > > +static int > +lbox_ctl_on_shutdown(struct lua_State *L) > +{ > + return lbox_trigger_reset(L, 2, &box_on_shutdown, NULL, NULL); > +} > + > static const struct luaL_Reg lbox_ctl_lib[] = { > {"wait_ro", lbox_ctl_wait_ro}, > {"wait_rw", lbox_ctl_wait_rw}, > + {"on_shutdown", lbox_ctl_on_shutdown}, > {NULL, NULL} > }; > > diff --git a/src/lua/trigger.c b/src/lua/trigger.c > index 2c2ede212..ec4d8aab3 100644 > --- a/src/lua/trigger.c > +++ b/src/lua/trigger.c > @@ -91,7 +91,10 @@ lbox_trigger_run(struct trigger *ptr, void *event) > } > int top = lua_gettop(L); > lua_rawgeti(L, LUA_REGISTRYINDEX, trigger->ref); > - int nargs = trigger->push_event(L, event); > + int nargs = 0; > + if (trigger->push_event != NULL) { > + nargs = trigger->push_event(L, event); > + } > if (luaT_call(L, nargs, LUA_MULTRET)) { > luaL_unref(tarantool_L, LUA_REGISTRYINDEX, coro_ref); > diag_raise(); > diff --git a/src/main.cc b/src/main.cc > index 4303fb32c..a6bdac5d8 100644 > --- a/src/main.cc > +++ b/src/main.cc > @@ -89,6 +89,10 @@ static const int ev_sig_count = sizeof(ev_sigs)/sizeof(*ev_sigs); > > static double start_time; > > +/** A preallocated fiber to run on_shutdown triggers. */ > +static struct fiber *on_exit_fiber = NULL; Nit: I'd call it on_shutdown_fiber, to match other variable names. > +static bool is_shutting_down = false; > + > double > tarantool_uptime(void) > { > @@ -119,9 +123,32 @@ sig_checkpoint(ev_loop * /* loop */, struct ev_signal * /* w */, > fiber_wakeup(f); > } > > +static int > +on_exit_f(va_list ap) > +{ > + (void) ap; > + trigger_run(&box_on_shutdown, NULL); > + return 0; > +} > + > +void > +tarantool_exit(void) > +{ > + if (is_shutting_down) > + /* > + * We are already running on_shutdown triggers, > + * and will exit as soon as they'll finish. > + * Do not execute them twice. > + */ > + return; Nit: multi-line if-clause should be enclosed in braces {}. > + is_shutting_down = true; > + fiber_wakeup(on_exit_fiber); > +} > + > static void > signal_cb(ev_loop *loop, struct ev_signal *w, int revents) > { > + (void) loop; > (void) w; > (void) revents; > > @@ -135,8 +162,7 @@ signal_cb(ev_loop *loop, struct ev_signal *w, int revents) > if (pid_file) > say_crit("got signal %d - %s", w->signum, strsignal(w->signum)); > start_loop = false; > - /* Terminate the main event loop */ > - ev_break(loop, EVBREAK_ALL); > + tarantool_exit(); > } > > static void > @@ -636,6 +662,12 @@ print_help(const char *program) > puts("to see online documentation, submit bugs or contribute a patch."); > } > > +void > +break_loop(struct trigger *, void *) Nit: this function should be static. > +{ > + ev_break(loop(), EVBREAK_ALL); > +} > + > int > main(int argc, char **argv) > { > @@ -759,6 +791,21 @@ main(int argc, char **argv) > try { > box_init(); > box_lua_init(tarantool_L); > + /* Reserve a fiber to run on_shutdown triggers. */ > + on_exit_fiber = fiber_new("on_exit", on_exit_f); I would rather allocate the fiber when the signal handler is called, because keeping the fiber around throughout the whole box lifetime although it's going to be used only on exit looks awkward to me. Also, we could get rid of one global variable then (on_exit_fiber). BTW panicking if we fail to allocate a fiber from the signal handler is perfectly OK. > + if (on_exit_fiber == NULL) > + diag_raise(); > + /* > + * Register a on_shutdown trigger which will break the > + * main event loop. The trigger will be the last to run > + * since it's the first one we register. > + */ > + struct trigger *trig = (struct trigger *)malloc(sizeof(*trig)); > + if (trig == NULL) > + tnt_raise(OutOfMemory, sizeof(*trig), > + "malloc", "struct trigger"); I think it would be better to make this trigger a static global variable instead of allocating it on the heap (BTW you don't bother to free it anywhere), because then all shutdown related variables (the flag, the trigger) would be gathered in one place, making the code easier to follow. > + trigger_create(trig, break_loop, NULL, NULL); > + trigger_add(&box_on_shutdown, trig); > > atexit(tarantool_atexit); >