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

[Serge Petrenko <sergepetrenko@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