Tarantool development patches archive
 help / color / mirror / Atom feed
* [PATCH v4 0/2] box: implement on_shutdown triggers
@ 2018-12-29 13:45 Serge Petrenko
  2018-12-29 13:45 ` [PATCH v4 1/2] box: get rid of atexit() for calling cleanup routines Serge Petrenko
  2018-12-29 13:45 ` [PATCH v4 2/2] box: implement on_shutdown triggers Serge Petrenko
  0 siblings, 2 replies; 5+ messages in thread
From: Serge Petrenko @ 2018-12-29 13:45 UTC (permalink / raw)
  To: vdavydov.dev; +Cc: kostja, tarantool-patches, 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. 

The 1st patch moves tarantool_free() from atexit() to end of main().
As mentioned above, the only things still done in atexit() are stopping WAL 
thread and restoring terminal state.

Second 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()

Serge Petrenko (2):
  box: get rid of atexit() for calling cleanup routines
  box: implement on_shutdown triggers

 src/box/box.cc         | 10 +++++
 src/box/box.h          | 10 +++++
 src/box/lua/ctl.c      |  8 ++++
 src/lua/trigger.c      |  5 ++-
 src/main.cc            | 83 +++++++++++++++++++++++++++++++++++-------
 test/box/misc.result   | 72 ++++++++++++++++++++++++++++++++++++
 test/box/misc.test.lua | 32 ++++++++++++++++
 7 files changed, 205 insertions(+), 15 deletions(-)

-- 
2.17.2 (Apple Git-113)

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v4 1/2] box: get rid of atexit() for calling cleanup routines
  2018-12-29 13:45 [PATCH v4 0/2] box: implement on_shutdown triggers Serge Petrenko
@ 2018-12-29 13:45 ` Serge Petrenko
  2018-12-29 13:45 ` [PATCH v4 2/2] box: implement on_shutdown triggers Serge Petrenko
  1 sibling, 0 replies; 5+ messages in thread
From: Serge Petrenko @ 2018-12-29 13:45 UTC (permalink / raw)
  To: vdavydov.dev; +Cc: kostja, tarantool-patches, Serge Petrenko

Move a call to tarantool_free() to the end of main().
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    | 32 ++++++++++++++++++++------------
 3 files changed, 35 insertions(+), 12 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..4303fb32c 100644
--- a/src/main.cc
+++ b/src/main.cc
@@ -534,6 +534,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 +586,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 +760,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 +800,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)

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v4 2/2] box: implement on_shutdown triggers
  2018-12-29 13:45 [PATCH v4 0/2] box: implement on_shutdown triggers Serge Petrenko
  2018-12-29 13:45 ` [PATCH v4 1/2] box: get rid of atexit() for calling cleanup routines Serge Petrenko
@ 2018-12-29 13:45 ` Serge Petrenko
  2019-01-09 10:14   ` Vladimir Davydov
  1 sibling, 1 reply; 5+ messages in thread
From: Serge Petrenko @ 2018-12-29 13:45 UTC (permalink / raw)
  To: vdavydov.dev; +Cc: kostja, tarantool-patches, 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.

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;
+
 /*
  * 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 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;
+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;
+	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 *)
+{
+	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);
+		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");
+		trigger_create(trig, break_loop, NULL, NULL);
+		trigger_add(&box_on_shutdown, trig);
 
 		atexit(tarantool_atexit);
 
diff --git a/test/box/misc.result b/test/box/misc.result
index 9fecbce76..9ffc11b88 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)

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v4 2/2] box: implement on_shutdown triggers
  2018-12-29 13:45 ` [PATCH v4 2/2] box: implement on_shutdown triggers Serge Petrenko
@ 2019-01-09 10:14   ` Vladimir Davydov
  2019-01-09 14:01     ` [tarantool-patches] " Konstantin Osipov
  0 siblings, 1 reply; 5+ messages in thread
From: Vladimir Davydov @ 2019-01-09 10:14 UTC (permalink / raw)
  To: Serge Petrenko; +Cc: kostja, tarantool-patches

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 <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 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);
>  

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [tarantool-patches] Re: [PATCH v4 2/2] box: implement on_shutdown triggers
  2019-01-09 10:14   ` Vladimir Davydov
@ 2019-01-09 14:01     ` Konstantin Osipov
  0 siblings, 0 replies; 5+ messages in thread
From: Konstantin Osipov @ 2019-01-09 14:01 UTC (permalink / raw)
  To: tarantool-patches; +Cc: Serge Petrenko

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/01/09 13:16]:
> > +/** 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.

This is a usability problem. We need to monkey-patch os.exit() to
call these triggers. Sergey, please do it in a separate patch.


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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-01-09 14:01 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-29 13:45 [PATCH v4 0/2] box: implement on_shutdown triggers Serge Petrenko
2018-12-29 13:45 ` [PATCH v4 1/2] box: get rid of atexit() for calling cleanup routines Serge Petrenko
2018-12-29 13:45 ` [PATCH v4 2/2] box: implement on_shutdown triggers Serge Petrenko
2019-01-09 10:14   ` Vladimir Davydov
2019-01-09 14:01     ` [tarantool-patches] " Konstantin Osipov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox