Re: [Tarantool-patches] [PATCH 1/4] box: add a single execution guard to clear_synchro_queue

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch!

Looks fine. Only 2 notes below.

> diff --git a/src/box/box.cc b/src/box/box.cc
> index a8bc3471d..8e0c9a160 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -1001,15 +1001,25 @@ box_set_replication_anon(void)
>  
>  }
>  
> -void
> +int
>  box_clear_synchro_queue(bool try_wait)
>  {
> +	/* A guard to block multiple simultaneous function invocations. */
> +	static bool in_clear_synchro_queue = false;
> +	if (in_clear_synchro_queue) {
> +		diag_set(ClientError, ER_UNSUPPORTED, "clear_synchro_queue",
> +			 "simultaneous invocations");
> +		return -1;
> +	}
>  	if (!is_box_configured || txn_limbo_is_empty(&txn_limbo))
> -		return;
> +		return 0;
>  	uint32_t former_leader_id = txn_limbo.owner_id;
>  	assert(former_leader_id != REPLICA_ID_NIL);
>  	if (former_leader_id == instance_id)
> -		return;
> +		return 0;
> +
> +	in_clear_synchro_queue = true;
> +	auto guard = make_scoped_guard([&] { in_clear_synchro_queue = false; });

I would better not use C++ here, because guards were
introduced only for protection against exceptions.

But I don't mind having this guard here if you want it.
Only my thoughts.

>  	if (try_wait) {
>  		/* Wait until pending confirmations/rollbacks reach us. */
> diff --git a/src/box/lua/ctl.c b/src/box/lua/ctl.c
> index bf26465e6..a3447f3e7 100644
> --- a/src/box/lua/ctl.c
> +++ b/src/box/lua/ctl.c
> @@ -81,8 +81,8 @@ lbox_ctl_on_schema_init(struct lua_State *L)
>  static int
>  lbox_ctl_clear_synchro_queue(struct lua_State *L)
>  {
> -	(void) L;
> -	box_clear_synchro_queue(true);
> +	if (box_clear_synchro_queue(true) != 0)
> +		return luaT_error(L);

Maybe better use nil + error object return way? I thought
we still use it in the new code.