From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <v.shpilevoy@tarantool.org>
Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
 (No client certificate requested)
 by dev.tarantool.org (Postfix) with ESMTPS id 3F2C345C304
 for <tarantool-patches@dev.tarantool.org>;
 Fri, 18 Dec 2020 00:43:22 +0300 (MSK)
References: <cover.1607633488.git.sergepetrenko@tarantool.org>
 <7c1516165eee62eb534cc5080971223e1edc3ca8.1607633488.git.sergepetrenko@tarantool.org>
From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Message-ID: <3d0aa43b-bd36-12b3-2abd-754468f2a301@tarantool.org>
Date: Thu, 17 Dec 2020 22:43:20 +0100
MIME-Version: 1.0
In-Reply-To: <7c1516165eee62eb534cc5080971223e1edc3ca8.1607633488.git.sergepetrenko@tarantool.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [Tarantool-patches] [PATCH 1/4] box: add a single execution
 guard to clear_synchro_queue
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
To: Serge Petrenko <sergepetrenko@tarantool.org>, cyrillos@gmail.com
Cc: tarantool-patches@dev.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.