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

[Vladimir Davydov <vdavydov.dev@gmail.com>]

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