[Tarantool-patches] [PATCH 1/1] limbo: don't wake self fiber on CONFIRM write

[Tarantool-patches] [PATCH 1/1] limbo: don't wake self fiber on CONFIRM write

[Vladislav Shpilevoy]

During recovery WAL writes end immediately, without yields.
Therefore WAL write completion callback is executed in the
currently active fiber.

Txn limbo on CONFIRM WAL write wakes up the waiting fiber, which
appears to be the same as the active fiber during recovery.

That breaks the fiber scheduler, because apparently it is not safe
to wake the currently active fiber unless it is going to call
fiber_yield() immediately after. See a comment in fiber_wakeup()
implementation about that way of usage.

The patch simply stops waking the waiting fiber, if it is the
currently active one.

Closes #5288
Closes #5232
---
Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-5288-limbo-wakeup
Issue: https://github.com/tarantool/tarantool/issues/5288
Issue: https://github.com/tarantool/tarantool/issues/5232

 src/box/txn_limbo.c                           |  3 ++-
 .../replication/gh-5288-qsync-recovery.result | 27 +++++++++++++++++++
 .../gh-5288-qsync-recovery.test.lua           | 11 ++++++++
 3 files changed, 40 insertions(+), 1 deletion(-)
 create mode 100644 test/replication/gh-5288-qsync-recovery.result
 create mode 100644 test/replication/gh-5288-qsync-recovery.test.lua

diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
index 2ff3bdd05..3655338d8 100644
--- a/src/box/txn_limbo.c
+++ b/src/box/txn_limbo.c
@@ -281,7 +281,8 @@ static void
 txn_limbo_write_cb(struct journal_entry *entry)
 {
 	assert(entry->complete_data != NULL);
-	fiber_wakeup(entry->complete_data);
+	if (fiber() != entry->complete_data)
+		fiber_wakeup(entry->complete_data);
 }
 
 static void
diff --git a/test/replication/gh-5288-qsync-recovery.result b/test/replication/gh-5288-qsync-recovery.result
new file mode 100644
index 000000000..dc0babef6
--- /dev/null
+++ b/test/replication/gh-5288-qsync-recovery.result
@@ -0,0 +1,27 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+--
+-- gh-5288: transaction limbo could crash during recovery, because in WAL write
+-- completion callback it woken up the currently active fiber.
+--
+s = box.schema.space.create('sync', {is_sync = true})
+ | ---
+ | ...
+_ = s:create_index('pk')
+ | ---
+ | ...
+s:insert{1}
+ | ---
+ | - [1]
+ | ...
+box.snapshot()
+ | ---
+ | - ok
+ | ...
+test_run:cmd('restart server default')
+ | 
+box.space.sync:drop()
+ | ---
+ | ...
diff --git a/test/replication/gh-5288-qsync-recovery.test.lua b/test/replication/gh-5288-qsync-recovery.test.lua
new file mode 100644
index 000000000..00bff7b87
--- /dev/null
+++ b/test/replication/gh-5288-qsync-recovery.test.lua
@@ -0,0 +1,11 @@
+test_run = require('test_run').new()
+--
+-- gh-5288: transaction limbo could crash during recovery, because in WAL write
+-- completion callback it woken up the currently active fiber.
+--
+s = box.schema.space.create('sync', {is_sync = true})
+_ = s:create_index('pk')
+s:insert{1}
+box.snapshot()
+test_run:cmd('restart server default')
+box.space.sync:drop()
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH 1/1] limbo: don't wake self fiber on CONFIRM write

[Alexander V. Tikhonov]

Hi Vlad, thanks a lot for your patch. I've checked and it really helped
to fix the issue, patch LGTM.

On Thu, Sep 10, 2020 at 11:53:57PM +0200, Vladislav Shpilevoy wrote:
> During recovery WAL writes end immediately, without yields.
> Therefore WAL write completion callback is executed in the
> currently active fiber.
> 
> Txn limbo on CONFIRM WAL write wakes up the waiting fiber, which
> appears to be the same as the active fiber during recovery.
> 
> That breaks the fiber scheduler, because apparently it is not safe
> to wake the currently active fiber unless it is going to call
> fiber_yield() immediately after. See a comment in fiber_wakeup()
> implementation about that way of usage.
> 
> The patch simply stops waking the waiting fiber, if it is the
> currently active one.
> 
> Closes #5288
> Closes #5232
> ---
> Branch: http://github.com/tarantool/tarantool/tree/gerold103/gh-5288-limbo-wakeup
> Issue: https://github.com/tarantool/tarantool/issues/5288
> Issue: https://github.com/tarantool/tarantool/issues/5232
> 
>  src/box/txn_limbo.c                           |  3 ++-
>  .../replication/gh-5288-qsync-recovery.result | 27 +++++++++++++++++++
>  .../gh-5288-qsync-recovery.test.lua           | 11 ++++++++
>  3 files changed, 40 insertions(+), 1 deletion(-)
>  create mode 100644 test/replication/gh-5288-qsync-recovery.result
>  create mode 100644 test/replication/gh-5288-qsync-recovery.test.lua
> 
> diff --git a/src/box/txn_limbo.c b/src/box/txn_limbo.c
> index 2ff3bdd05..3655338d8 100644
> --- a/src/box/txn_limbo.c
> +++ b/src/box/txn_limbo.c
> @@ -281,7 +281,8 @@ static void
>  txn_limbo_write_cb(struct journal_entry *entry)
>  {
>  	assert(entry->complete_data != NULL);
> -	fiber_wakeup(entry->complete_data);
> +	if (fiber() != entry->complete_data)
> +		fiber_wakeup(entry->complete_data);
>  }
>  
>  static void
> diff --git a/test/replication/gh-5288-qsync-recovery.result b/test/replication/gh-5288-qsync-recovery.result
> new file mode 100644
> index 000000000..dc0babef6
> --- /dev/null
> +++ b/test/replication/gh-5288-qsync-recovery.result
> @@ -0,0 +1,27 @@
> +-- test-run result file version 2
> +test_run = require('test_run').new()
> + | ---
> + | ...
> +--
> +-- gh-5288: transaction limbo could crash during recovery, because in WAL write
> +-- completion callback it woken up the currently active fiber.
> +--
> +s = box.schema.space.create('sync', {is_sync = true})
> + | ---
> + | ...
> +_ = s:create_index('pk')
> + | ---
> + | ...
> +s:insert{1}
> + | ---
> + | - [1]
> + | ...
> +box.snapshot()
> + | ---
> + | - ok
> + | ...
> +test_run:cmd('restart server default')
> + | 
> +box.space.sync:drop()
> + | ---
> + | ...
> diff --git a/test/replication/gh-5288-qsync-recovery.test.lua b/test/replication/gh-5288-qsync-recovery.test.lua
> new file mode 100644
> index 000000000..00bff7b87
> --- /dev/null
> +++ b/test/replication/gh-5288-qsync-recovery.test.lua
> @@ -0,0 +1,11 @@
> +test_run = require('test_run').new()
> +--
> +-- gh-5288: transaction limbo could crash during recovery, because in WAL write
> +-- completion callback it woken up the currently active fiber.
> +--
> +s = box.schema.space.create('sync', {is_sync = true})
> +_ = s:create_index('pk')
> +s:insert{1}
> +box.snapshot()
> +test_run:cmd('restart server default')
> +box.space.sync:drop()
> -- 
> 2.21.1 (Apple Git-122.3)
> 

Re: [Tarantool-patches] [PATCH 1/1] limbo: don't wake self fiber on CONFIRM write

[Vladislav Shpilevoy]

Pushed to master and 2.5 as trivial.

@ChangeLog
* During recovery of synchronous changes from snapshot the instance could crash (gh-5288).