[tarantool-patches] [PATCH v3 2/3] box: implement on_shutdown triggers
Vladimir Davydov
vdavydov.dev at gmail.com
Thu Dec 27 16:11:44 MSK 2018
On Tue, Dec 11, 2018 at 04:24:39PM +0300, Serge Petrenko wrote:
> From: Konstantin Osipov <kostja at 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.");
More information about the Tarantool-patches
mailing list