* [tarantool-patches] [PATCH v3 1/3] box: get rid of atexit() for calling cleanup routine
2018-12-11 13:24 [tarantool-patches] [PATCH v3 0/3] box: implement on_shutdown triggers Serge Petrenko
@ 2018-12-11 13:24 ` Serge Petrenko
2018-12-27 12:43 ` Vladimir Davydov
2018-12-11 13:24 ` [tarantool-patches] [PATCH v3 2/3] box: implement on_shutdown triggers Serge Petrenko
2018-12-11 13:24 ` [tarantool-patches] [PATCH v3 3/3] box: introduce on_shutdown triggers to lua Serge Petrenko
2 siblings, 1 reply; 12+ messages in thread
From: Serge Petrenko @ 2018-12-11 13:24 UTC (permalink / raw)
To: kostja; +Cc: tarantool-patches, 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)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [tarantool-patches] [PATCH v3 1/3] box: get rid of atexit() for calling cleanup routine
2018-12-11 13:24 ` [tarantool-patches] [PATCH v3 1/3] box: get rid of atexit() for calling cleanup routine Serge Petrenko
@ 2018-12-27 12:43 ` Vladimir Davydov
2018-12-27 12:50 ` Konstantin Osipov
0 siblings, 1 reply; 12+ messages in thread
From: Vladimir Davydov @ 2018-12-27 12:43 UTC (permalink / raw)
To: Serge Petrenko; +Cc: kostja, tarantool-patches
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();
> }
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [tarantool-patches] [PATCH v3 1/3] box: get rid of atexit() for calling cleanup routine
2018-12-27 12:43 ` Vladimir Davydov
@ 2018-12-27 12:50 ` Konstantin Osipov
2018-12-27 12:56 ` Vladimir Davydov
0 siblings, 1 reply; 12+ messages in thread
From: Konstantin Osipov @ 2018-12-27 12:50 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: Serge Petrenko, tarantool-patches
* 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [tarantool-patches] [PATCH v3 1/3] box: get rid of atexit() for calling cleanup routine
2018-12-27 12:50 ` Konstantin Osipov
@ 2018-12-27 12:56 ` Vladimir Davydov
2018-12-28 7:43 ` [tarantool-patches] " Konstantin Osipov
0 siblings, 1 reply; 12+ messages in thread
From: Vladimir Davydov @ 2018-12-27 12:56 UTC (permalink / raw)
To: Konstantin Osipov; +Cc: Serge Petrenko, tarantool-patches
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] Re: [PATCH v3 1/3] box: get rid of atexit() for calling cleanup routine
2018-12-27 12:56 ` Vladimir Davydov
@ 2018-12-28 7:43 ` Konstantin Osipov
2018-12-28 7:58 ` Vladimir Davydov
0 siblings, 1 reply; 12+ messages in thread
From: Konstantin Osipov @ 2018-12-28 7:43 UTC (permalink / raw)
To: tarantool-patches; +Cc: Serge Petrenko
* 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [tarantool-patches] Re: [PATCH v3 1/3] box: get rid of atexit() for calling cleanup routine
2018-12-28 7:43 ` [tarantool-patches] " Konstantin Osipov
@ 2018-12-28 7:58 ` Vladimir Davydov
2018-12-29 13:54 ` Serge Petrenko
0 siblings, 1 reply; 12+ messages in thread
From: Vladimir Davydov @ 2018-12-28 7:58 UTC (permalink / raw)
To: Konstantin Osipov; +Cc: tarantool-patches, Serge Petrenko
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [tarantool-patches] Re: [PATCH v3 1/3] box: get rid of atexit() for calling cleanup routine
2018-12-28 7:58 ` Vladimir Davydov
@ 2018-12-29 13:54 ` Serge Petrenko
0 siblings, 0 replies; 12+ messages in thread
From: Serge Petrenko @ 2018-12-29 13:54 UTC (permalink / raw)
To: Vladimir Davydov; +Cc: tarantool-patches, Konstantin Osipov
[-- 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 --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] [PATCH v3 2/3] box: implement on_shutdown triggers
2018-12-11 13:24 [tarantool-patches] [PATCH v3 0/3] box: implement on_shutdown triggers Serge Petrenko
2018-12-11 13:24 ` [tarantool-patches] [PATCH v3 1/3] box: get rid of atexit() for calling cleanup routine Serge Petrenko
@ 2018-12-11 13:24 ` Serge Petrenko
2018-12-27 13:11 ` Vladimir Davydov
2018-12-11 13:24 ` [tarantool-patches] [PATCH v3 3/3] box: introduce on_shutdown triggers to lua Serge Petrenko
2 siblings, 1 reply; 12+ messages in thread
From: Serge Petrenko @ 2018-12-11 13:24 UTC (permalink / raw)
To: kostja; +Cc: tarantool-patches
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)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [tarantool-patches] [PATCH v3 2/3] box: implement on_shutdown triggers
2018-12-11 13:24 ` [tarantool-patches] [PATCH v3 2/3] box: implement on_shutdown triggers Serge Petrenko
@ 2018-12-27 13:11 ` Vladimir Davydov
0 siblings, 0 replies; 12+ messages in thread
From: Vladimir Davydov @ 2018-12-27 13:11 UTC (permalink / raw)
To: Serge Petrenko; +Cc: kostja, tarantool-patches
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.");
^ permalink raw reply [flat|nested] 12+ messages in thread
* [tarantool-patches] [PATCH v3 3/3] box: introduce on_shutdown triggers to lua
2018-12-11 13:24 [tarantool-patches] [PATCH v3 0/3] box: implement on_shutdown triggers Serge Petrenko
2018-12-11 13:24 ` [tarantool-patches] [PATCH v3 1/3] box: get rid of atexit() for calling cleanup routine Serge Petrenko
2018-12-11 13:24 ` [tarantool-patches] [PATCH v3 2/3] box: implement on_shutdown triggers Serge Petrenko
@ 2018-12-11 13:24 ` Serge Petrenko
2018-12-27 13:14 ` Vladimir Davydov
2 siblings, 1 reply; 12+ messages in thread
From: Serge Petrenko @ 2018-12-11 13:24 UTC (permalink / raw)
To: kostja; +Cc: tarantool-patches, 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)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [tarantool-patches] [PATCH v3 3/3] box: introduce on_shutdown triggers to lua
2018-12-11 13:24 ` [tarantool-patches] [PATCH v3 3/3] box: introduce on_shutdown triggers to lua Serge Petrenko
@ 2018-12-27 13:14 ` Vladimir Davydov
0 siblings, 0 replies; 12+ messages in thread
From: Vladimir Davydov @ 2018-12-27 13:14 UTC (permalink / raw)
To: Serge Petrenko; +Cc: kostja, tarantool-patches
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})
^ permalink raw reply [flat|nested] 12+ messages in thread