From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp51.i.mail.ru (smtp51.i.mail.ru [94.100.177.111]) (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 8E1A94765E0 for ; Mon, 21 Dec 2020 13:18:42 +0300 (MSK) References: <7c1516165eee62eb534cc5080971223e1edc3ca8.1607633488.git.sergepetrenko@tarantool.org> <3d0aa43b-bd36-12b3-2abd-754468f2a301@tarantool.org> From: Serge Petrenko Message-ID: <99e20e44-9833-5e32-00dc-d6ed91058960@tarantool.org> Date: Mon, 21 Dec 2020 13:18:40 +0300 MIME-Version: 1.0 In-Reply-To: <3d0aa43b-bd36-12b3-2abd-754468f2a301@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-GB Subject: Re: [Tarantool-patches] [PATCH 1/4] box: add a single execution guard to clear_synchro_queue List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy , Cyrill Gorcunov Cc: tarantool-patches@dev.tarantool.org 18.12.2020 00:43, Vladislav Shpilevoy пишет: > Hi! Thanks for the patch! Looks fine. Only 2 notes below. Thanks for the review! >> 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. I agree. I thought there would be multiple returns below so I introduced the guard to not write ``` in_clear_synchro_queue = false; return 0; ``` every time. Turns out there are only 2 such places, and I can still use `goto end` to omit an extraneous `in_clear_synchro_queue = false`. The diff for this commit is below. > 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. Hm, I haven't seen us do that in lua/C. As far as I know, every box.* method throws a lua error in case of failure. I may miss something. Is there a reason for returning nil + error instead of throwing? ============================================== ``` diff --git a/src/box/box.cc b/src/box/box.cc index 8e0c9a160..6f7a89d8d 100644 --- a/src/box/box.cc +++ b/src/box/box.cc @@ -1019,7 +1019,6 @@ box_clear_synchro_queue(bool try_wait)                 return 0;         in_clear_synchro_queue = true; -       auto guard = make_scoped_guard([&] { in_clear_synchro_queue = false; });         if (try_wait) {                 /* Wait until pending confirmations/rollbacks reach us. */ @@ -1060,6 +1059,8 @@ box_clear_synchro_queue(bool try_wait)                 txn_limbo_force_empty(&txn_limbo, confirm_lsn);                 assert(txn_limbo_is_empty(&txn_limbo));         } + +       in_clear_synchro_queue = false;         return 0;  } ``` > -- Serge Petrenko