[PATCH v5 0/3] box: implement on_shutdown triggers

[PATCH v5 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.

Changes in v4: 
  - squashed 2nd and 3rd patches
  - review fixes as per review by @locker
  - made 1st and 2nd patches independent.
    1st patch may be dropped if we decide not to
    implement it. 

Changes in v5:
  - patched os.exit() to call on_shutdown triggers
  - some minor fixes as per review by @locker
  - the patch which removes atexit() registration for
    cleanup routines is moved last. It is now completely
    trivial thanks to the os.exit() patch.

The first patch implements on_shutdown triggers. The triggers are run from
a fiber before ev_loop break, and the last executed trigger breaks ev_loop.
The triggers are exposed to lua via box.ctl.on_shutdown().

The second patch alters os.exit() to call on_shutdown triggers. Now behaviour
after os.exit() is identical to the behaviour after receiving SIGINT.
(Except that exit code is changed).

The third patch removes cleanup routines from atexit to the end of main().
This can be done thanks to the first two patches, which ensure that during
normal termination control always reaches end of main().

Serge Petrenko (3):
  box: implement on_shutdown triggers
  lua: patch os.exit() to execute on_shutdown triggers.
  box: get rid of atexit() for calling cleanup routines

 extra/exports          |  1 +
 src/box/box.cc         |  2 ++
 src/box/box.h          |  3 ++
 src/box/lua/ctl.c      |  8 +++++
 src/lua/init.lua       |  7 ++++
 src/lua/trigger.c      |  5 ++-
 src/main.cc            | 60 +++++++++++++++++++++++++++++++----
 src/main.h             |  3 ++
 test/box/misc.result   | 72 ++++++++++++++++++++++++++++++++++++++++++
 test/box/misc.test.lua | 32 +++++++++++++++++++
 10 files changed, 185 insertions(+), 8 deletions(-)

-- 
2.17.2 (Apple Git-113)

[PATCH v5 1/3] box: implement on_shutdown triggers

[Serge Petrenko]

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.

Part of #1607
---
 src/box/box.cc         |  2 ++
 src/box/box.h          |  3 ++
 src/box/lua/ctl.c      |  8 +++++
 src/lua/trigger.c      |  5 ++-
 src/main.cc            | 50 +++++++++++++++++++++++++++--
 test/box/misc.result   | 72 ++++++++++++++++++++++++++++++++++++++++++
 test/box/misc.test.lua | 32 +++++++++++++++++++
 7 files changed, 169 insertions(+), 3 deletions(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index 9f2fd6da1..fb595aeee 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 6c6c319fc..53d88ab71 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;
+
 /*
  * 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 <lualib.h>
 
 #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 993355ac4..96df9e851 100644
--- a/src/main.cc
+++ b/src/main.cc
@@ -89,6 +89,12 @@ 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_shutdown_fiber = NULL;
+static bool is_shutting_down = false;
+/** A trigger which will break the event loop on shutdown. */
+static struct trigger shutdown_trig;
+
 double
 tarantool_uptime(void)
 {
@@ -119,9 +125,33 @@ sig_checkpoint(ev_loop * /* loop */, struct ev_signal * /* w */,
 	fiber_wakeup(f);
 }
 
+static int
+on_shutdown_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;
+	}
+	is_shutting_down = true;
+	fiber_wakeup(on_shutdown_fiber);
+}
+
 static void
 signal_cb(ev_loop *loop, struct ev_signal *w, int revents)
 {
+	(void) loop;
 	(void) w;
 	(void) revents;
 
@@ -135,8 +165,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
@@ -628,6 +657,12 @@ print_help(const char *program)
 	puts("to see online documentation, submit bugs or contribute a patch.");
 }
 
+static void
+break_loop(struct trigger *, void *)
+{
+	ev_break(loop(), EVBREAK_ALL);
+}
+
 int
 main(int argc, char **argv)
 {
@@ -751,6 +786,17 @@ main(int argc, char **argv)
 	try {
 		box_init();
 		box_lua_init(tarantool_L);
+		/* Reserve a fiber to run on_shutdown triggers. */
+		on_shutdown_fiber = fiber_new("on_exit", on_shutdown_f);
+		if (on_shutdown_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.
+		 */
+		trigger_create(&shutdown_trig, break_loop, NULL, NULL);
+		trigger_add(&box_on_shutdown, &shutdown_trig);
 
 		/* main core cleanup routine */
 		atexit(tarantool_free);
diff --git a/test/box/misc.result b/test/box/misc.result
index c3cabcc8a..6912915c1 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -1249,3 +1249,75 @@ 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 perform some complicated actions.
+fiber = require('fiber')
+---
+...
+test_run:cmd("setopt delimiter ';'")
+---
+- true
+...
+trig = function()
+    fiber.sleep(0.01)
+    fiber.yield()
+    box.schema.space.create("shutdown")
+    box.space.shutdown:create_index("pk")
+    box.space.shutdown:insert{1,2,3}
+    print('on_shutdown 4')
+end;
+---
+...
+test_run:cmd("setopt delimiter ''");
+---
+- true
+...
+_ = box.ctl.on_shutdown(f)
+---
+...
+_ = box.ctl.on_shutdown(g)
+---
+...
+-- Check that replacing triggers works
+_ = 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
+...
+box.space.shutdown:select{}
+---
+- - [1, 2, 3]
+...
+box.space.shutdown:drop()
+---
+...
diff --git a/test/box/misc.test.lua b/test/box/misc.test.lua
index cc6cb34fb..f1c9d8e8c 100644
--- a/test/box/misc.test.lua
+++ b/test/box/misc.test.lua
@@ -352,3 +352,35 @@ 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 perform some complicated actions.
+fiber = require('fiber')
+test_run:cmd("setopt delimiter ';'")
+trig = function()
+    fiber.sleep(0.01)
+    fiber.yield()
+    box.schema.space.create("shutdown")
+    box.space.shutdown:create_index("pk")
+    box.space.shutdown:insert{1,2,3}
+    print('on_shutdown 4')
+end;
+test_run:cmd("setopt delimiter ''");
+_ = box.ctl.on_shutdown(f)
+_ = box.ctl.on_shutdown(g)
+-- Check that replacing triggers works
+_ = 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})
+box.space.shutdown:select{}
+box.space.shutdown:drop()
-- 
2.17.2 (Apple Git-113)

[PATCH v5 2/3] lua: patch os.exit() to execute on_shutdown triggers.

[Serge Petrenko]

Make os.exit() call tarantool_exit(), just like the signal handler does.
Now on_shutdown triggers are not run only when a fatal signal is
received.

@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` or when user executes
`os.exit()`.

Note that the triggers will not be run if tarantool receives a fatal
signal: `SIGSEGV`, `SIGABORT` or any signal causing immediate program
termination.

Closes #1607
---
 extra/exports    |  1 +
 src/lua/init.lua |  7 +++++++
 src/main.cc      | 10 ++++++----
 src/main.h       |  3 +++
 4 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/extra/exports b/extra/exports
index 5f69e0730..35b47ae00 100644
--- a/extra/exports
+++ b/extra/exports
@@ -69,6 +69,7 @@ say_set_log_level
 say_logrotate
 say_set_log_format
 tarantool_uptime
+tarantool_exit
 log_pid
 space_by_id
 space_run_triggers
diff --git a/src/lua/init.lua b/src/lua/init.lua
index fa324d34c..a961db328 100644
--- a/src/lua/init.lua
+++ b/src/lua/init.lua
@@ -38,6 +38,8 @@ double
 tarantool_uptime(void);
 typedef int32_t pid_t;
 pid_t getpid(void);
+void
+tarantool_exit(int);
 ]]
 
 local fio = require("fio")
@@ -50,6 +52,11 @@ dostring = function(s, ...)
     return chunk(...)
 end
 
+os.exit = function(code)
+    code = (type(code) == 'number') and code or 0
+    ffi.C.tarantool_exit(code)
+end
+
 local function uptime()
     return tonumber(ffi.C.tarantool_uptime());
 end
diff --git a/src/main.cc b/src/main.cc
index 96df9e851..a3d1a6f22 100644
--- a/src/main.cc
+++ b/src/main.cc
@@ -94,6 +94,7 @@ static struct fiber *on_shutdown_fiber = NULL;
 static bool is_shutting_down = false;
 /** A trigger which will break the event loop on shutdown. */
 static struct trigger shutdown_trig;
+static int exit_code = 0;
 
 double
 tarantool_uptime(void)
@@ -134,8 +135,9 @@ on_shutdown_f(va_list ap)
 }
 
 void
-tarantool_exit(void)
+tarantool_exit(int code)
 {
+	start_loop = false;
 	if (is_shutting_down) {
 		/*
 		 * We are already running on_shutdown triggers,
@@ -145,6 +147,7 @@ tarantool_exit(void)
 		return;
 	}
 	is_shutting_down = true;
+	exit_code = code;
 	fiber_wakeup(on_shutdown_fiber);
 }
 
@@ -164,8 +167,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;
-	tarantool_exit();
+	tarantool_exit(0);
 }
 
 static void
@@ -839,5 +841,5 @@ main(int argc, char **argv)
 	if (start_loop)
 		say_crit("exiting the event loop");
 	/* freeing resources */
-	return 0;
+	return exit_code;
 }
diff --git a/src/main.h b/src/main.h
index 221374144..f509e905b 100644
--- a/src/main.h
+++ b/src/main.h
@@ -39,6 +39,9 @@ extern "C" {
 
 double tarantool_uptime(void);
 
+void
+tarantool_exit(int);
+
 void
 load_cfg();
 
-- 
2.17.2 (Apple Git-113)

[PATCH v5 3/3] box: get rid of atexit() for calling cleanup routines

[Serge Petrenko]

Move a call to tarantool_free() to the end of main().
We needn't call atexit() at all anymore, since we've implemented
on_shutdown triggers and patched os.exit() so that when exiting not
due to a fatal signal (when no cleanup routines are called anyway)
control always reaches a call to tarantool_free().
---
 src/main.cc | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/src/main.cc b/src/main.cc
index a3d1a6f22..3fc2dd310 100644
--- a/src/main.cc
+++ b/src/main.cc
@@ -800,9 +800,6 @@ main(int argc, char **argv)
 		trigger_create(&shutdown_trig, break_loop, NULL, NULL);
 		trigger_add(&box_on_shutdown, &shutdown_trig);
 
-		/* main core cleanup routine */
-		atexit(tarantool_free);
-
 		if (!loop())
 			panic("%s", "can't init event loop");
 
@@ -841,5 +838,6 @@ main(int argc, char **argv)
 	if (start_loop)
 		say_crit("exiting the event loop");
 	/* freeing resources */
+	tarantool_free();
 	return exit_code;
 }
-- 
2.17.2 (Apple Git-113)

Re: [PATCH v5 1/3] box: implement on_shutdown triggers

[Vladimir Davydov]

The patch is generally OK. See a few really minor comments below.

On Fri, Jan 25, 2019 at 06:41:32PM +0300, Serge Petrenko wrote:
> diff --git a/src/main.cc b/src/main.cc
> index 993355ac4..96df9e851 100644
> --- a/src/main.cc
> +++ b/src/main.cc
> @@ -89,6 +89,12 @@ 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_shutdown_fiber = NULL;
> +static bool is_shutting_down = false;

Missing comment.

> +/** A trigger which will break the event loop on shutdown. */
> +static struct trigger shutdown_trig;

IMO break_loop_trigger would be a more appropriate name.

> +
>  double
>  tarantool_uptime(void)
>  {
> @@ -119,9 +125,33 @@ sig_checkpoint(ev_loop * /* loop */, struct ev_signal * /* w */,
>  	fiber_wakeup(f);
>  }
>  
> +static int
> +on_shutdown_f(va_list ap)
> +{
> +	(void) ap;
> +	trigger_run(&box_on_shutdown, NULL);
> +	return 0;
> +}
> +
> +void
> +tarantool_exit(void)

Should be static.

> +{
> +	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;
> +	}
> +	is_shutting_down = true;
> +	fiber_wakeup(on_shutdown_fiber);
> +}
> +
>  static void
>  signal_cb(ev_loop *loop, struct ev_signal *w, int revents)
>  {
> +	(void) loop;
>  	(void) w;
>  	(void) revents;
>  
> @@ -135,8 +165,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
> @@ -628,6 +657,12 @@ print_help(const char *program)
>  	puts("to see online documentation, submit bugs or contribute a patch.");
>  }
>  
> +static void
> +break_loop(struct trigger *, void *)
> +{
> +	ev_break(loop(), EVBREAK_ALL);
> +}
> +
>  int
>  main(int argc, char **argv)
>  {
> @@ -751,6 +786,17 @@ main(int argc, char **argv)
>  	try {
>  		box_init();
>  		box_lua_init(tarantool_L);
> +		/* Reserve a fiber to run on_shutdown triggers. */
> +		on_shutdown_fiber = fiber_new("on_exit", on_shutdown_f);
> +		if (on_shutdown_fiber == NULL)
> +			diag_raise();

I guess the fiber should be made non-cancellable so that it couldn't be
occasionally woken up from Lua.

> +		/*
> +		 * 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.
> +		 */
> +		trigger_create(&shutdown_trig, break_loop, NULL, NULL);
> +		trigger_add(&box_on_shutdown, &shutdown_trig);
>  
>  		/* main core cleanup routine */
>  		atexit(tarantool_free);

Re: [PATCH v5 2/3] lua: patch os.exit() to execute on_shutdown triggers.

[Vladimir Davydov]

On Fri, Jan 25, 2019 at 06:41:33PM +0300, Serge Petrenko wrote:
> Make os.exit() call tarantool_exit(), just like the signal handler does.
> Now on_shutdown triggers are not run only when a fatal signal is
> received.
> 
> @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` or when user executes
> `os.exit()`.
> 
> Note that the triggers will not be run if tarantool receives a fatal
> signal: `SIGSEGV`, `SIGABORT` or any signal causing immediate program
> termination.
> 

> Closes #1607

Should go before the TarantoolBot request.

> ---
>  extra/exports    |  1 +
>  src/lua/init.lua |  7 +++++++
>  src/main.cc      | 10 ++++++----
>  src/main.h       |  3 +++
>  4 files changed, 17 insertions(+), 4 deletions(-)

A test is missing. Please add.

> 
> diff --git a/extra/exports b/extra/exports
> index 5f69e0730..35b47ae00 100644
> --- a/extra/exports
> +++ b/extra/exports
> @@ -69,6 +69,7 @@ say_set_log_level
>  say_logrotate
>  say_set_log_format
>  tarantool_uptime
> +tarantool_exit
>  log_pid
>  space_by_id
>  space_run_triggers
> diff --git a/src/lua/init.lua b/src/lua/init.lua
> index fa324d34c..a961db328 100644
> --- a/src/lua/init.lua
> +++ b/src/lua/init.lua
> @@ -38,6 +38,8 @@ double
>  tarantool_uptime(void);
>  typedef int32_t pid_t;
>  pid_t getpid(void);
> +void
> +tarantool_exit(int);
>  ]]
>  
>  local fio = require("fio")
> @@ -50,6 +52,11 @@ dostring = function(s, ...)
>      return chunk(...)
>  end
>  
> +os.exit = function(code)
> +    code = (type(code) == 'number') and code or 0
> +    ffi.C.tarantool_exit(code)
> +end
> +
>  local function uptime()
>      return tonumber(ffi.C.tarantool_uptime());
>  end
> diff --git a/src/main.cc b/src/main.cc
> index 96df9e851..a3d1a6f22 100644
> --- a/src/main.cc
> +++ b/src/main.cc
> @@ -94,6 +94,7 @@ static struct fiber *on_shutdown_fiber = NULL;
>  static bool is_shutting_down = false;
>  /** A trigger which will break the event loop on shutdown. */
>  static struct trigger shutdown_trig;
> +static int exit_code = 0;
>  
>  double
>  tarantool_uptime(void)
> @@ -134,8 +135,9 @@ on_shutdown_f(va_list ap)
>  }
>  
>  void
> -tarantool_exit(void)
> +tarantool_exit(int code)
>  {
> +	start_loop = false;
>  	if (is_shutting_down) {
>  		/*
>  		 * We are already running on_shutdown triggers,
> @@ -145,6 +147,7 @@ tarantool_exit(void)
>  		return;
>  	}
>  	is_shutting_down = true;
> +	exit_code = code;
>  	fiber_wakeup(on_shutdown_fiber);

os.exit() should exit even if the fiber doesn't yield, but the following
code will hang, which is unexpected. Please fix.

	os.exit()
	while true do end

>  }
>  
> @@ -164,8 +167,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;
> -	tarantool_exit();
> +	tarantool_exit(0);
>  }
>  
>  static void
> @@ -839,5 +841,5 @@ main(int argc, char **argv)
>  	if (start_loop)
>  		say_crit("exiting the event loop");
>  	/* freeing resources */
> -	return 0;
> +	return exit_code;
>  }
> diff --git a/src/main.h b/src/main.h
> index 221374144..f509e905b 100644
> --- a/src/main.h
> +++ b/src/main.h
> @@ -39,6 +39,9 @@ extern "C" {
>  
>  double tarantool_uptime(void);
>  
> +void
> +tarantool_exit(int);
> +
>  void
>  load_cfg();

Re: [PATCH v5 1/3] box: implement on_shutdown triggers

[Serge Petrenko]

> 28 янв. 2019 г., в 11:43, Vladimir Davydov <vdavydov.dev@gmail.com> написал(а):
> 
> The patch is generally OK. See a few really minor comments below.
> 

Hi! Thankyou for review. I fixed your comments. The fixes are on the branch
https://github.com/tarantool/tarantool/tree/sp/gh-1607-on-exit-triggers
Incremental diff for this patch is below

diff --git a/src/main.cc b/src/main.cc
index 96df9e851..a3d96843d 100644
--- a/src/main.cc
+++ b/src/main.cc
@@ -91,9 +91,10 @@ static double start_time;
 
 /** A preallocated fiber to run on_shutdown triggers. */
 static struct fiber *on_shutdown_fiber = NULL;
+/** A flag restricting repeated execution of tarantool_exit(). */
 static bool is_shutting_down = false;
 /** A trigger which will break the event loop on shutdown. */
-static struct trigger shutdown_trig;
+static struct trigger break_loop_trigger;
 
 double
 tarantool_uptime(void)
@@ -133,7 +134,7 @@ on_shutdown_f(va_list ap)
 	return 0;
 }
 
-void
+static void
 tarantool_exit(void)
 {
 	if (is_shutting_down) {
@@ -145,7 +146,7 @@ tarantool_exit(void)
 		return;
 	}
 	is_shutting_down = true;
-	fiber_wakeup(on_shutdown_fiber);
+	fiber_call(on_shutdown_fiber);
 }
 
 static void
@@ -786,8 +787,17 @@ main(int argc, char **argv)
 	try {
 		box_init();
 		box_lua_init(tarantool_L);
-		/* Reserve a fiber to run on_shutdown triggers. */
-		on_shutdown_fiber = fiber_new("on_exit", on_shutdown_f);
+		/*
+		 * Reserve a fiber to run on_shutdown triggers.
+		 * Make sure the fiber is non-cancellable so that
+		 * it doesn't get woken up from Lua unintentionally.
+		 */
+		struct fiber_attr attr;
+		fiber_attr_create(&attr);
+		attr.flags &= ~FIBER_IS_CANCELLABLE;
+		on_shutdown_fiber = fiber_new_ex("on_shutdown",
+						 &attr,
+						 on_shutdown_f);
 		if (on_shutdown_fiber == NULL)
 			diag_raise();
 		/*
@@ -795,8 +805,8 @@ main(int argc, char **argv)
 		 * main event loop. The trigger will be the last to run
 		 * since it's the first one we register.
 		 */
-		trigger_create(&shutdown_trig, break_loop, NULL, NULL);
-		trigger_add(&box_on_shutdown, &shutdown_trig);
+		trigger_create(&break_loop_trigger, break_loop, NULL, NULL);
+		trigger_add(&box_on_shutdown, &break_loop_trigger);
 
 		/* main core cleanup routine */
 		atexit(tarantool_free);

Re: [tarantool-patches] [PATCH v5 2/3] lua: patch os.exit() to execute on_shutdown triggers.

[Serge Petrenko]

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

Hi! Thankyou for review. I addressed your comments.

> 28 янв. 2019 г., в 11:45, Vladimir Davydov <vdavydov.dev@gmail.com> написал(а):
> 
> On Fri, Jan 25, 2019 at 06:41:33PM +0300, Serge Petrenko wrote:
>> Note that the triggers will not be run if tarantool receives a fatal
>> signal: `SIGSEGV`, `SIGABORT` or any signal causing immediate program
>> termination.
>> 
> 
>> Closes #1607
> 
> Should go before the TarantoolBot request.
> 

Fixed.

>> ---
>> extra/exports    |  1 +
>> src/lua/init.lua |  7 +++++++
>> src/main.cc      | 10 ++++++----
>> src/main.h       |  3 +++
>> 4 files changed, 17 insertions(+), 4 deletions(-)
> 
> A test is missing. Please add.
> 

Added a test

>> 
>> 
>> 	is_shutting_down = true;
>> +	exit_code = code;
>> 	fiber_wakeup(on_shutdown_fiber);
> 
> os.exit() should exit even if the fiber doesn't yield, but the following
> code will hang, which is unexpected. Please fix.
> 
> 	os.exit()
> 	while true do end

Fixed

I pushed the fixes on the branch
https://github.com/tarantool/tarantool/tree/sp/gh-1607-on-exit-triggers
Incremental diff is below.

diff --git a/src/lua/init.lua b/src/lua/init.lua
index a961db328..9fd56f483 100644
--- a/src/lua/init.lua
+++ b/src/lua/init.lua
@@ -52,9 +52,14 @@ dostring = function(s, ...)
     return chunk(...)
 end
 
+local fiber = require("fiber")
 os.exit = function(code)
     code = (type(code) == 'number') and code or 0
     ffi.C.tarantool_exit(code)
+    -- Make sure we yield even if the code after
+    -- os.exit() never yields. After on_shutdown
+    -- fiber completes, we will never wake up again.
+    while true do fiber.yield() end
 end
 
 local function uptime()
diff --git a/test/box/misc.result b/test/box/misc.result
index 6912915c1..97189ecbb 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -1321,3 +1321,92 @@ box.space.shutdown:select{}
 box.space.shutdown:drop()
 ---
 ...
+-- Check that os.exit invokes triggers
+fiber = require("fiber")
+---
+...
+test_run:cmd("create server test with script='box/proxy.lua'")
+---
+- true
+...
+test_run:cmd("start server test")
+---
+- true
+...
+logfile = test_run:eval("test", "box.cfg.log")[1]
+---
+...
+test_run:cmd("stop server test")
+---
+- true
+...
+-- clean up any leftover logs
+require("fio").unlink(logfile)
+---
+- true
+...
+test_run:cmd("start server test")
+---
+- true
+...
+test_run:cmd("switch test")
+---
+- true
+...
+_ = box.ctl.on_shutdown(function() print("on_shutdown 5") end)
+---
+...
+-- Check that we don't hang infinitely after os.exit()
+-- even if the following code doesn't yield.
+fiber = require("fiber")
+---
+...
+_ = fiber.create(function() fiber.sleep(0.05) os.exit() while true do end end)
+---
+...
+test_run:cmd("switch default")
+---
+- true
+...
+fiber.sleep(0.1)
+---
+...
+-- The server should be already stopped by os.exit(),
+-- but start doesn't work without a prior call to stop.
+test_run:cmd("stop server test")
+---
+- true
+...
+test_run:cmd("start server test")
+---
+- true
+...
+test_run:cmd("switch test")
+---
+- true
+...
+test_run:grep_log('test', 'on_shutdown 5', nil, {noreset=true})
+---
+- on_shutdown 5
+...
+-- make sure we exited because of os.exit(), not a signal.
+test_run:grep_log('test', 'signal', nil, {noreset=true})
+---
+- null
+...
+test_run:cmd("switch default")
+---
+- true
+...
+test_run:cmd("stop server test")
+---
+- true
+...
+test_run:cmd("cleanup server test")
+---
+- true
+...
+test_run:cmd("delete server test")
+---
+- true
+...
diff --git a/test/box/misc.test.lua b/test/box/misc.test.lua
index f1c9d8e8c..18128b299 100644
--- a/test/box/misc.test.lua
+++ b/test/box/misc.test.lua
@@ -384,3 +384,33 @@ test_run:grep_log('default', 'on_shutdown 3', nil, {noreset=true})
 test_run:grep_log('default', 'on_shutdown 4', nil, {noreset=true})
 box.space.shutdown:select{}
 box.space.shutdown:drop()
+
+-- Check that os.exit invokes triggers
+fiber = require("fiber")
+test_run:cmd("create server test with script='box/proxy.lua'")
+test_run:cmd("start server test")
+logfile = test_run:eval("test", "box.cfg.log")[1]
+test_run:cmd("stop server test")
+-- clean up any leftover logs
+require("fio").unlink(logfile)
+test_run:cmd("start server test")
+test_run:cmd("switch test")
+_ = box.ctl.on_shutdown(function() print("on_shutdown 5") end)
+-- Check that we don't hang infinitely after os.exit()
+-- even if the following code doesn't yield.
+fiber = require("fiber")
+_ = fiber.create(function() fiber.sleep(0.05) os.exit() while true do end end)
+test_run:cmd("switch default")
+fiber.sleep(0.1)
+-- The server should be already stopped by os.exit(),
+-- but start doesn't work without a prior call to stop.
+test_run:cmd("stop server test")
+test_run:cmd("start server test")
+test_run:cmd("switch test")
+test_run:grep_log('test', 'on_shutdown 5', nil, {noreset=true})
+-- make sure we exited because of os.exit(), not a signal.
+test_run:grep_log('test', 'signal', nil, {noreset=true})
+test_run:cmd("switch default")
+test_run:cmd("stop server test")
+test_run:cmd("cleanup server test")
+test_run:cmd("delete server test")

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

Re: [PATCH v5 0/3] box: implement on_shutdown triggers

[Vladimir Davydov]

Pushed to 2.1.