[tarantool-patches] [PATCH v3 0/3] box: implement on_shutdown triggers

[tarantool-patches] [PATCH v3 0/3] box: implement on_shutdown triggers

[Serge Petrenko]

This patchset adds on_shutdown triggers which can be registered from lua 
via box.ctl.on_shutdown() and are run on tarantool shutdown.

Issue: https://github.com/tarantool/tarantool/issues/1607
Branch: https://github.com/tarantool/tarantool/tree/sp/gh-1607-on-exit-triggers

Changes in v2: 
  - added 3rd patch which passes signals to triggers.
  - run on_shutdown triggers even if box is not configured.
  - add a documentation request to the 3rd patch.

Changes in v3: 
  - remove the patch which passes signals to the trigger, since
    the triggers are not run on fatal signals at all, and doing
    this for SIGTERM or SIGINT is not of much use.
  - Add a new patch which moves tarantool_free() from atexit()
    to end of main(). Only close the journal and restore terminal state
    (if needed) in atexit handlers. Also in signal handler create
    a new fiber which breaks the event loop. on_shutdown triggers will
    be run from this fiber before ev loop break.
  - add tests that on_shutdown triggers can yield to the third patch.

AFAICS now there are no limitations on what can be done in on_shutdown
triggers: you may sleep, yield, perform DML/DDL requests.

First patch moves tarantool_free() from atexit() to end of main(),
alters signal handler for SIGINT, SIGHUP, SIGTERM to create a new fiber
which can be used to do some work on shutdown (run on_shutdown triggers)
and then break the event loop.
As mentioned above, the only things still done in atexit() are stopping
WAL thread and 

Second patch was originally made by Kostja and implemented on_shutdown
triggers. I altered it a bit so that on_shutdown triggers are run from
a fiber before event loop break, this allows us to yield in on_shutdown
triggers and to use box library.

Third patch expands box.ctl interface by introducing box.ctl.on_shutdown()
function to register on_shutdown triggers from lua.

Konstantin Osipov (1):
  box: implement on_shutdown triggers

Serge Petrenko (2):
  box: get rid of atexit() for calling cleanup routine
  box: introduce on_shutdown triggers to lua

 src/box/box.cc         | 16 +++++++++
 src/box/box.h          | 18 ++++++++++
 src/box/lua/ctl.c      | 21 ++++++++++++
 src/main.cc            | 75 ++++++++++++++++++++++++++++++++++--------
 test/box/misc.result   | 48 +++++++++++++++++++++++++++
 test/box/misc.test.lua | 19 +++++++++++
 6 files changed, 183 insertions(+), 14 deletions(-)

-- 
2.17.2 (Apple Git-113)

[tarantool-patches] [PATCH v3 1/3] box: get rid of atexit() for calling cleanup routine

[Serge Petrenko]

Move a call to tarantool_free() to the end of main().
Also instead of breaking the event loop directly when processing a
signal start a fiber which can do some work before event loop break and
then break the event loop (this will be helpful when on_shutdown
triggers are implemented to run them while ev loop active).
Only wal_thread_stop() is left in atexit() to make sure we close the
journal in case the user exits by typing os.exit() to the console.
---
 src/box/box.cc |  8 +++++++
 src/box/box.h  |  7 ++++++
 src/main.cc    | 59 ++++++++++++++++++++++++++++++++++++++------------
 3 files changed, 60 insertions(+), 14 deletions(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index 9f2fd6da1..9642364f6 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -1693,9 +1693,17 @@ box_free(void)
 		gc_free();
 		engine_shutdown();
 		wal_thread_stop();
+		is_box_configured = false;
 	}
 }
 
+void
+box_shutdown_wal(void)
+{
+	if (is_box_configured)
+		wal_thread_stop();
+}
+
 static void
 engine_init()
 {
diff --git a/src/box/box.h b/src/box/box.h
index 6c6c319fc..cb9a512be 100644
--- a/src/box/box.h
+++ b/src/box/box.h
@@ -77,6 +77,13 @@ box_init(void);
 void
 box_free(void);
 
+/**
+ *  Kill WAL thread and close journal.
+ *  Called at exit.
+ */
+void
+box_shutdown_wal(void);
+
 /**
  * Load configuration for box library.
  * Panics on error.
diff --git a/src/main.cc b/src/main.cc
index 993355ac4..2240c564a 100644
--- a/src/main.cc
+++ b/src/main.cc
@@ -83,6 +83,7 @@ static char *script = NULL;
 static char *pid_file = NULL;
 static char **main_argv;
 static int main_argc;
+
 /** Signals handled after start as part of the event loop. */
 static ev_signal ev_sigs[6];
 static const int ev_sig_count = sizeof(ev_sigs)/sizeof(*ev_sigs);
@@ -119,11 +120,33 @@ sig_checkpoint(ev_loop * /* loop */, struct ev_signal * /* w */,
 	fiber_wakeup(f);
 }
 
+static int
+on_exit_f(va_list ap)
+{
+	(void) ap;
+	/* Terminate the main event loop. */
+	ev_break(loop(), EVBREAK_ALL);
+	return 0;
+}
+
+void
+tarantool_exit(void)
+{
+	struct fiber *f = fiber_new("on_shutdown", on_exit_f);
+	if (f == NULL) {
+		say_warn("failed to allocate a fiber to run shutdown routines.");
+		ev_break(loop(), EVBREAK_ALL);
+		return;
+	}
+	fiber_wakeup(f);
+}
+
 static void
 signal_cb(ev_loop *loop, struct ev_signal *w, int revents)
 {
 	(void) w;
 	(void) revents;
+	(void) loop;
 
 	/**
 	 * If running in daemon mode, complain about possibly
@@ -135,8 +158,8 @@ 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
@@ -534,6 +557,24 @@ load_cfg()
 	box_cfg();
 }
 
+void
+tarantool_atexit(void)
+{
+	box_shutdown_wal();
+
+	/* tarantool_lua_free() was formerly reponsible for terminal reset,
+	 * but it is no longer called
+	 */
+	if (isatty(STDIN_FILENO)) {
+		/*
+		 * Restore terminal state. Doesn't hurt if exiting not
+		 * due to a signal.
+		 */
+		rl_cleanup_after_signal();
+	}
+
+}
+
 void
 tarantool_free(void)
 {
@@ -568,16 +609,6 @@ tarantool_free(void)
 #ifdef ENABLE_GCOV
 	__gcov_flush();
 #endif
-	/* tarantool_lua_free() was formerly reponsible for terminal reset,
-	 * but it is no longer called
-	 */
-	if (isatty(STDIN_FILENO)) {
-		/*
-		 * Restore terminal state. Doesn't hurt if exiting not
-		 * due to a signal.
-		 */
-		rl_cleanup_after_signal();
-	}
 	cbus_free();
 #if 0
 	/*
@@ -752,8 +783,7 @@ main(int argc, char **argv)
 		box_init();
 		box_lua_init(tarantool_L);
 
-		/* main core cleanup routine */
-		atexit(tarantool_free);
+		atexit(tarantool_atexit);
 
 		if (!loop())
 			panic("%s", "can't init event loop");
@@ -793,5 +823,6 @@ main(int argc, char **argv)
 	if (start_loop)
 		say_crit("exiting the event loop");
 	/* freeing resources */
+	tarantool_free();
 	return 0;
 }
-- 
2.17.2 (Apple Git-113)

[tarantool-patches] [PATCH v3 2/3] box: implement on_shutdown triggers

[Serge Petrenko]

From: Konstantin Osipov <kostja@tarantool.org>

@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.

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);
+
 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;
+
+/** 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);
 	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;
+
 	struct fiber *f = fiber_new("on_shutdown", on_exit_f);
 	if (f == NULL) {
 		say_warn("failed to allocate a fiber to run shutdown routines.");
-- 
2.17.2 (Apple Git-113)

[tarantool-patches] [PATCH v3 3/3] box: introduce on_shutdown triggers to lua

[Serge Petrenko]

Make it possible to register on_shutdown triggers from lua via
box.ctl.on_shutdown()

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/lua/ctl.c      | 21 ++++++++++++++++++
 test/box/misc.result   | 48 ++++++++++++++++++++++++++++++++++++++++++
 test/box/misc.test.lua | 19 +++++++++++++++++
 3 files changed, 88 insertions(+)

diff --git a/src/box/lua/ctl.c b/src/box/lua/ctl.c
index 9a105ed5c..aa835d6d5 100644
--- a/src/box/lua/ctl.c
+++ b/src/box/lua/ctl.c
@@ -37,6 +37,7 @@
 #include <lualib.h>
 
 #include "lua/utils.h"
+#include "lua/trigger.h"
 
 #include "box/box.h"
 
@@ -64,9 +65,29 @@ lbox_ctl_wait_rw(struct lua_State *L)
 	return 0;
 }
 
+/*
+ * This is a placeholder.
+ * We may decide to pass some arguments to on_shutdown triggers.
+ */
+static int
+lbox_push_on_shutdown(struct lua_State *L, void *event)
+{
+	(void)L;
+	(void)event;
+	return 0;
+}
+
+static int
+lbox_ctl_on_shutdown(struct lua_State *L)
+{
+	return lbox_trigger_reset(L, 2, &on_shutdown, lbox_push_on_shutdown,
+				  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/test/box/misc.result b/test/box/misc.result
index d266bb334..981e20157 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -1248,3 +1248,51 @@ box.cfg{too_long_threshold = too_long_threshold}
 s:drop()
 ---
 ...
+--
+-- gh-1607: on_shutdown triggers.
+--
+f = function() print('on_shutdown 1') end
+---
+...
+g = function() print('on_shutdown 2') end
+---
+...
+h = function() print('on_shutdown 3') end
+---
+...
+-- check that on_shutdown triggers may yield and sleep.
+fiber = require('fiber')
+---
+...
+trig = function() fiber.sleep(0.01) fiber.yield() print('on_shutdown 4') end
+---
+...
+_ = box.ctl.on_shutdown(f)
+---
+...
+_ = box.ctl.on_shutdown(g)
+---
+...
+_ = box.ctl.on_shutdown(h, g)
+---
+...
+_ = box.ctl.on_shutdown(trig)
+---
+...
+test_run:cmd('restart server default')
+test_run:grep_log('default', 'on_shutdown 1', nil, {noreset=true})
+---
+- on_shutdown 1
+...
+test_run:grep_log('default', 'on_shutdown 2', nil, {noreset=true})
+---
+- null
+...
+test_run:grep_log('default', 'on_shutdown 3', nil, {noreset=true})
+---
+- on_shutdown 3
+...
+test_run:grep_log('default', 'on_shutdown 4', nil, {noreset=true})
+---
+- on_shutdown 4
+...
diff --git a/test/box/misc.test.lua b/test/box/misc.test.lua
index cc6cb34fb..f9d0f53f2 100644
--- a/test/box/misc.test.lua
+++ b/test/box/misc.test.lua
@@ -352,3 +352,22 @@ rows == expected_rows
 lsn == expected_lsn
 box.cfg{too_long_threshold = too_long_threshold}
 s:drop()
+
+--
+-- gh-1607: on_shutdown triggers.
+--
+f = function() print('on_shutdown 1') end
+g = function() print('on_shutdown 2') end
+h = function() print('on_shutdown 3') end
+-- check that on_shutdown triggers may yield and sleep.
+fiber = require('fiber')
+trig = function() fiber.sleep(0.01) fiber.yield() print('on_shutdown 4') end
+_ = box.ctl.on_shutdown(f)
+_ = box.ctl.on_shutdown(g)
+_ = box.ctl.on_shutdown(h, g)
+_ = box.ctl.on_shutdown(trig)
+test_run:cmd('restart server default')
+test_run:grep_log('default', 'on_shutdown 1', nil, {noreset=true})
+test_run:grep_log('default', 'on_shutdown 2', nil, {noreset=true})
+test_run:grep_log('default', 'on_shutdown 3', nil, {noreset=true})
+test_run:grep_log('default', 'on_shutdown 4', nil, {noreset=true})
-- 
2.17.2 (Apple Git-113)

Re: [tarantool-patches] [PATCH v3 1/3] box: get rid of atexit() for calling cleanup routine

[Vladimir Davydov]

On Tue, Dec 11, 2018 at 04:24:38PM +0300, Serge Petrenko wrote:
> Move a call to tarantool_free() to the end of main().
> Also instead of breaking the event loop directly when processing a
> signal start a fiber which can do some work before event loop break and
> then break the event loop (this will be helpful when on_shutdown
> triggers are implemented to run them while ev loop active).
> Only wal_thread_stop() is left in atexit() to make sure we close the
> journal in case the user exits by typing os.exit() to the console.

I don't understand why you need to do it in the scope of the issue
you're intending to fix. You can start a fiber from SITERM/SIGINT signal
handler and run on_shutdown triggers from it without atexit rework,
right?

> @@ -119,11 +120,33 @@ sig_checkpoint(ev_loop * /* loop */, struct ev_signal * /* w */,
>  	fiber_wakeup(f);
>  }
>  
> +static int
> +on_exit_f(va_list ap)
> +{
> +	(void) ap;
> +	/* Terminate the main event loop. */
> +	ev_break(loop(), EVBREAK_ALL);
> +	return 0;
> +}
> +
> +void
> +tarantool_exit(void)
> +{
> +	struct fiber *f = fiber_new("on_shutdown", on_exit_f);
> +	if (f == NULL) {
> +		say_warn("failed to allocate a fiber to run shutdown routines.");
> +		ev_break(loop(), EVBREAK_ALL);
> +		return;
> +	}
> +	fiber_wakeup(f);

This should be done by the patch that introduces on_shutdown triggers,
otherwise it's unclear why you need a fiber here.

> +}
> +
>  static void
>  signal_cb(ev_loop *loop, struct ev_signal *w, int revents)
>  {
>  	(void) w;
>  	(void) revents;
> +	(void) loop;
>  
>  	/**
>  	 * If running in daemon mode, complain about possibly
> @@ -135,8 +158,8 @@ 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();
>  }

Re: [tarantool-patches] [PATCH v3 1/3] box: get rid of atexit() for calling cleanup routine

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [18/12/27 15:45]:
> On Tue, Dec 11, 2018 at 04:24:38PM +0300, Serge Petrenko wrote:
> > Move a call to tarantool_free() to the end of main().
> > Also instead of breaking the event loop directly when processing a
> > signal start a fiber which can do some work before event loop break and
> > then break the event loop (this will be helpful when on_shutdown
> > triggers are implemented to run them while ev loop active).
> > Only wal_thread_stop() is left in atexit() to make sure we close the
> > journal in case the user exits by typing os.exit() to the console.
> 
> I don't understand why you need to do it in the scope of the issue
> you're intending to fix. You can start a fiber from SITERM/SIGINT signal
> handler and run on_shutdown triggers from it without atexit rework,
> right?

I asked Sergey to refactor our atexit machinery. I want atexit
handlers to be able to run the event loop, all such handlers, not
just the exit trigger.

> > +void
> > +tarantool_exit(void)
> > +{
> > +	struct fiber *f = fiber_new("on_shutdown", on_exit_f);
> > +	if (f == NULL) {
> > +		say_warn("failed to allocate a fiber to run shutdown routines.");
> > +		ev_break(loop(), EVBREAK_ALL);
> > +		return;
> > +	}
> > +	fiber_wakeup(f);
> 
> This should be done by the patch that introduces on_shutdown triggers,
> otherwise it's unclear why you need a fiber here.

I'd prefer this fiber to be preallocated or reuse 'sched' fiber
for it.

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

Re: [tarantool-patches] [PATCH v3 1/3] box: get rid of atexit() for calling cleanup routine

[Vladimir Davydov]

On Thu, Dec 27, 2018 at 03:50:58PM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [18/12/27 15:45]:
> > On Tue, Dec 11, 2018 at 04:24:38PM +0300, Serge Petrenko wrote:
> > > Move a call to tarantool_free() to the end of main().
> > > Also instead of breaking the event loop directly when processing a
> > > signal start a fiber which can do some work before event loop break and
> > > then break the event loop (this will be helpful when on_shutdown
> > > triggers are implemented to run them while ev loop active).
> > > Only wal_thread_stop() is left in atexit() to make sure we close the
> > > journal in case the user exits by typing os.exit() to the console.
> > 
> > I don't understand why you need to do it in the scope of the issue
> > you're intending to fix. You can start a fiber from SITERM/SIGINT signal
> > handler and run on_shutdown triggers from it without atexit rework,
> > right?
> 
> I asked Sergey to refactor our atexit machinery. I want atexit
> handlers to be able to run the event loop, all such handlers, not
> just the exit trigger.

But this isn't what this patch does - it just moves some code from
atexit() to the end of main, after the event loop is stopped.

Re: [tarantool-patches] [PATCH v3 2/3] box: implement on_shutdown triggers

[Vladimir Davydov]

On Tue, Dec 11, 2018 at 04:24:39PM +0300, Serge Petrenko wrote:
> From: Konstantin Osipov <kostja@tarantool.org>
> 
> @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.");

Re: [tarantool-patches] [PATCH v3 3/3] box: introduce on_shutdown triggers to lua

[Vladimir Davydov]

On Tue, Dec 11, 2018 at 04:24:40PM +0300, Serge Petrenko wrote:
> Make it possible to register on_shutdown triggers from lua via
> box.ctl.on_shutdown()
> 
> 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/lua/ctl.c      | 21 ++++++++++++++++++
>  test/box/misc.result   | 48 ++++++++++++++++++++++++++++++++++++++++++
>  test/box/misc.test.lua | 19 +++++++++++++++++
>  3 files changed, 88 insertions(+)
> 
> diff --git a/src/box/lua/ctl.c b/src/box/lua/ctl.c
> index 9a105ed5c..aa835d6d5 100644
> --- a/src/box/lua/ctl.c
> +++ b/src/box/lua/ctl.c
> @@ -37,6 +37,7 @@
>  #include <lualib.h>
>  
>  #include "lua/utils.h"
> +#include "lua/trigger.h"
>  
>  #include "box/box.h"
>  
> @@ -64,9 +65,29 @@ lbox_ctl_wait_rw(struct lua_State *L)
>  	return 0;
>  }
>  
> +/*
> + * This is a placeholder.
> + * We may decide to pass some arguments to on_shutdown triggers.

I don't think so. Let's instead remove this placeholder and add a check
for push_event != NULL to lbox_trigger_run().

> + */
> +static int
> +lbox_push_on_shutdown(struct lua_State *L, void *event)
> +{
> +	(void)L;
> +	(void)event;
> +	return 0;
> +}
> +
> +static int
> +lbox_ctl_on_shutdown(struct lua_State *L)
> +{
> +	return lbox_trigger_reset(L, 2, &on_shutdown, lbox_push_on_shutdown,
> +				  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/test/box/misc.test.lua b/test/box/misc.test.lua
> index cc6cb34fb..f9d0f53f2 100644
> --- a/test/box/misc.test.lua
> +++ b/test/box/misc.test.lua
> @@ -352,3 +352,22 @@ rows == expected_rows
>  lsn == expected_lsn
>  box.cfg{too_long_threshold = too_long_threshold}
>  s:drop()
> +
> +--
> +-- gh-1607: on_shutdown triggers.
> +--
> +f = function() print('on_shutdown 1') end
> +g = function() print('on_shutdown 2') end
> +h = function() print('on_shutdown 3') end
> +-- check that on_shutdown triggers may yield and sleep.
> +fiber = require('fiber')
> +trig = function() fiber.sleep(0.01) fiber.yield() print('on_shutdown 4') end

Let's test something more complicated, e.g. inserting a tuple into
a space.

> +_ = box.ctl.on_shutdown(f)
> +_ = box.ctl.on_shutdown(g)
> +_ = box.ctl.on_shutdown(h, g)
> +_ = box.ctl.on_shutdown(trig)
> +test_run:cmd('restart server default')
> +test_run:grep_log('default', 'on_shutdown 1', nil, {noreset=true})
> +test_run:grep_log('default', 'on_shutdown 2', nil, {noreset=true})
> +test_run:grep_log('default', 'on_shutdown 3', nil, {noreset=true})
> +test_run:grep_log('default', 'on_shutdown 4', nil, {noreset=true})

[tarantool-patches] Re: [PATCH v3 1/3] box: get rid of atexit() for calling cleanup routine

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [18/12/27 16:00]:
> > > On Tue, Dec 11, 2018 at 04:24:38PM +0300, Serge Petrenko wrote:
> > > > Move a call to tarantool_free() to the end of main().
> > > > Also instead of breaking the event loop directly when processing a
> > > > signal start a fiber which can do some work before event loop break and
> > > > then break the event loop (this will be helpful when on_shutdown
> > > > triggers are implemented to run them while ev loop active).
> > > > Only wal_thread_stop() is left in atexit() to make sure we close the
> > > > journal in case the user exits by typing os.exit() to the console.
> > > 
> > > I don't understand why you need to do it in the scope of the issue
> > > you're intending to fix. You can start a fiber from SITERM/SIGINT signal
> > > handler and run on_shutdown triggers from it without atexit rework,
> > > right?
> > 
> > I asked Sergey to refactor our atexit machinery. I want atexit
> > handlers to be able to run the event loop, all such handlers, not
> > just the exit trigger.
> 
> But this isn't what this patch does - it just moves some code from
> atexit() to the end of main, after the event loop is stopped.

This is OK for existing handlers, since they don't require an
event loop today. 

-- 
Konstantin Osipov, Moscow, Russia, +7 903 626 22 32
http://tarantool.io - www.twitter.com/kostja_osipov

Re: [tarantool-patches] Re: [PATCH v3 1/3] box: get rid of atexit() for calling cleanup routine

[Vladimir Davydov]

On Fri, Dec 28, 2018 at 10:43:26AM +0300, Konstantin Osipov wrote:
> * Vladimir Davydov <vdavydov.dev@gmail.com> [18/12/27 16:00]:
> > > > On Tue, Dec 11, 2018 at 04:24:38PM +0300, Serge Petrenko wrote:
> > > > > Move a call to tarantool_free() to the end of main().
> > > > > Also instead of breaking the event loop directly when processing a
> > > > > signal start a fiber which can do some work before event loop break and
> > > > > then break the event loop (this will be helpful when on_shutdown
> > > > > triggers are implemented to run them while ev loop active).
> > > > > Only wal_thread_stop() is left in atexit() to make sure we close the
> > > > > journal in case the user exits by typing os.exit() to the console.
> > > > 
> > > > I don't understand why you need to do it in the scope of the issue
> > > > you're intending to fix. You can start a fiber from SITERM/SIGINT signal
> > > > handler and run on_shutdown triggers from it without atexit rework,
> > > > right?
> > > 
> > > I asked Sergey to refactor our atexit machinery. I want atexit
> > > handlers to be able to run the event loop, all such handlers, not
> > > just the exit trigger.
> > 
> > But this isn't what this patch does - it just moves some code from
> > atexit() to the end of main, after the event loop is stopped.
> 
> This is OK for existing handlers, since they don't require an
> event loop today. 

Then why move them from atexit() to the end of main()? It doesn't
achieve anything, it doesn't have anything to do with on_shutdown
trigger. For some reason, we just stop freeing memory if the program
terminates by exit()-ing.

Re: [tarantool-patches] Re: [PATCH v3 1/3] box: get rid of atexit() for calling cleanup routine

[Serge Petrenko]

[-- Attachment #1: Type: text/plain, Size: 1996 bytes --]

Hi!
I addressed your comments regarding all the 3 patches and
sent v4 for review («[PATCH v4 0/2] box: implement on_shutdown triggers»)

As for this particular patch, I made it independent from the next one,
so it may be dropped altogether if you wish.

> 28 дек. 2018 г., в 10:58, Vladimir Davydov <vdavydov.dev@gmail.com> написал(а):
> 
> On Fri, Dec 28, 2018 at 10:43:26AM +0300, Konstantin Osipov wrote:
>> * Vladimir Davydov <vdavydov.dev@gmail.com> [18/12/27 16:00]:
>>>>> On Tue, Dec 11, 2018 at 04:24:38PM +0300, Serge Petrenko wrote:
>>>>>> Move a call to tarantool_free() to the end of main().
>>>>>> Also instead of breaking the event loop directly when processing a
>>>>>> signal start a fiber which can do some work before event loop break and
>>>>>> then break the event loop (this will be helpful when on_shutdown
>>>>>> triggers are implemented to run them while ev loop active).
>>>>>> Only wal_thread_stop() is left in atexit() to make sure we close the
>>>>>> journal in case the user exits by typing os.exit() to the console.
>>>>> 
>>>>> I don't understand why you need to do it in the scope of the issue
>>>>> you're intending to fix. You can start a fiber from SITERM/SIGINT signal
>>>>> handler and run on_shutdown triggers from it without atexit rework,
>>>>> right?
>>>> 
>>>> I asked Sergey to refactor our atexit machinery. I want atexit
>>>> handlers to be able to run the event loop, all such handlers, not
>>>> just the exit trigger.
>>> 
>>> But this isn't what this patch does - it just moves some code from
>>> atexit() to the end of main, after the event loop is stopped.
>> 
>> This is OK for existing handlers, since they don't require an
>> event loop today. 
> 
> Then why move them from atexit() to the end of main()? It doesn't
> achieve anything, it doesn't have anything to do with on_shutdown
> trigger. For some reason, we just stop freeing memory if the program
> terminates by exit()-ing.
> 


[-- Attachment #2: Type: text/html, Size: 3137 bytes --]