[Tarantool-patches] [PATCH] lua: handle fiber cancellation for fiber.cond

[Tarantool-patches] [PATCH] lua: handle fiber cancellation for fiber.cond

[sergos]

From: Sergey Ostanevich <sergos@tarantool.org>

Before this patch fiber.cond():wait() just returns for cancelled
fiber. In contrast fiber.channel():get() threw "fiber is
canceled" error.
This patch unify behaviour of channels and condvars and also fixes
related net.box module problem - it was impossible to interrupt
net.box call with fiber.cancel because it used fiber.cond under
the hood. Test cases for both bugs are added.

Closes #4834
Closes #5013

Co-authored-by: Oleg Babin <olegrok@tarantool.org>

@TarantoolBot document
Title: fiber.cond():wait() throws if fiber is cancelled

Currently fiber.cond():wait() throws an error if waiting fiber is
cancelled like in case with fiber.channel():get().
---

Github: https://gitlab.com/tarantool/tarantool/-/commits/sergos/gh-5013-fiber-cond
Issue: https://github.com/tarantool/tarantool/issues/5013

 src/lua/fiber_cond.c                          |  3 +-
 test/app-tap/gh-5013-fiber-cancel.test.lua    | 23 +++++++
 test/box/net.box_fiber_cancel_gh-4834.result  | 65 +++++++++++++++++++
 .../box/net.box_fiber_cancel_gh-4834.test.lua | 29 +++++++++
 4 files changed, 118 insertions(+), 2 deletions(-)
 create mode 100755 test/app-tap/gh-5013-fiber-cancel.test.lua
 create mode 100644 test/box/net.box_fiber_cancel_gh-4834.result
 create mode 100644 test/box/net.box_fiber_cancel_gh-4834.test.lua

diff --git a/src/lua/fiber_cond.c b/src/lua/fiber_cond.c
index 617226969..fdb2ab7fe 100644
--- a/src/lua/fiber_cond.c
+++ b/src/lua/fiber_cond.c
@@ -95,8 +95,7 @@ luaT_fiber_cond_wait(struct lua_State *L)
 		}
 	}
 	rc = fiber_cond_wait_timeout(e, timeout);
-	if (rc != 0)
-		luaL_testcancel(L);
+	luaL_testcancel(L);
 	lua_pushboolean(L, rc == 0);
 	return 1;
 }
diff --git a/test/app-tap/gh-5013-fiber-cancel.test.lua b/test/app-tap/gh-5013-fiber-cancel.test.lua
new file mode 100755
index 000000000..ae805c5bf
--- /dev/null
+++ b/test/app-tap/gh-5013-fiber-cancel.test.lua
@@ -0,0 +1,23 @@
+#!/usr/bin/env tarantool
+
+local tap = require('tap')
+local fiber = require('fiber')
+local test = tap.test("gh-5013-fiber-cancel")
+
+test:plan(2)
+
+local result = {}
+
+function test_f()
+    local cond = fiber.cond() 
+    local res, err = pcall(cond.wait, cond) 
+    result.res = res
+    result.err = err
+end
+
+local f = fiber.create(test_f)
+f:cancel()
+fiber.yield()
+
+test:ok(result.res == false, tostring(result.res))
+test:ok(tostring(result.err) == 'fiber is cancelled', tostring(result.err))
diff --git a/test/box/net.box_fiber_cancel_gh-4834.result b/test/box/net.box_fiber_cancel_gh-4834.result
new file mode 100644
index 000000000..eab0a5e4d
--- /dev/null
+++ b/test/box/net.box_fiber_cancel_gh-4834.result
@@ -0,0 +1,65 @@
+-- test-run result file version 2
+remote = require 'net.box'
+ | ---
+ | ...
+fiber = require 'fiber'
+ | ---
+ | ...
+test_run = require('test_run').new()
+ | ---
+ | ...
+
+-- #4834: Cancelling fiber doesn't interrupt netbox operations
+function infinite_call() fiber.channel(1):get() end
+ | ---
+ | ...
+box.schema.func.create('infinite_call')
+ | ---
+ | ...
+box.schema.user.grant('guest', 'execute', 'function', 'infinite_call')
+ | ---
+ | ...
+
+error_msg = nil
+ | ---
+ | ...
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+function gh4834()
+    local cn = remote.connect(box.cfg.listen)
+    local f = fiber.new(function()
+        _, error_msg = pcall(cn.call, cn, 'infinite_call')
+    end)
+    f:set_joinable(true)
+    fiber.yield()
+    f:cancel()
+    f:join()
+    cn:close()
+end;
+ | ---
+ | ...
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | - true
+ | ...
+gh4834()
+ | ---
+ | ...
+error_msg
+ | ---
+ | - fiber is cancelled
+ | ...
+box.schema.func.drop('infinite_call')
+ | ---
+ | ...
+infinite_call = nil
+ | ---
+ | ...
+channel = nil
+ | ---
+ | ...
+error_msg = nil
+ | ---
+ | ...
diff --git a/test/box/net.box_fiber_cancel_gh-4834.test.lua b/test/box/net.box_fiber_cancel_gh-4834.test.lua
new file mode 100644
index 000000000..06fb3ceac
--- /dev/null
+++ b/test/box/net.box_fiber_cancel_gh-4834.test.lua
@@ -0,0 +1,29 @@
+remote = require 'net.box'
+fiber = require 'fiber'
+test_run = require('test_run').new()
+
+-- #4834: Cancelling fiber doesn't interrupt netbox operations
+function infinite_call() fiber.channel(1):get() end
+box.schema.func.create('infinite_call')
+box.schema.user.grant('guest', 'execute', 'function', 'infinite_call')
+
+error_msg = nil
+test_run:cmd("setopt delimiter ';'")
+function gh4834()
+    local cn = remote.connect(box.cfg.listen)
+    local f = fiber.new(function()
+        _, error_msg = pcall(cn.call, cn, 'infinite_call')
+    end)
+    f:set_joinable(true)
+    fiber.yield()
+    f:cancel()
+    f:join()
+    cn:close()
+end;
+test_run:cmd("setopt delimiter ''");
+gh4834()
+error_msg
+box.schema.func.drop('infinite_call')
+infinite_call = nil
+channel = nil
+error_msg = nil
-- 
2.24.3 (Apple Git-128)

Re: [Tarantool-patches] [PATCH] lua: handle fiber cancellation for fiber.cond

[Oleg Babin]

Hi! Thanks for the patch!

The main reason I didn't send this patch is inconsistency between Lua 
and C API.

To eliminate such inconsistency [1] was implemented. Unfortunately some 
test fails and I've failed to find root of the problem.

However I still believe that the problem should be solved at "core" level.

Anyway, it would be great if some solution will be pushed to upstream.


[1] 
https://github.com/tarantool/tarantool/commit/e7a5120603f362364953315d31aa529342263e0b


On 28/10/2020 19:22, sergos@tarantool.org wrote:
> From: Sergey Ostanevich <sergos@tarantool.org>
>
> Before this patch fiber.cond():wait() just returns for cancelled
> fiber. In contrast fiber.channel():get() threw "fiber is
> canceled" error.
> This patch unify behaviour of channels and condvars and also fixes
> related net.box module problem - it was impossible to interrupt
> net.box call with fiber.cancel because it used fiber.cond under
> the hood. Test cases for both bugs are added.
>
> Closes #4834
> Closes #5013
>
> Co-authored-by: Oleg Babin <olegrok@tarantool.org>
>
> @TarantoolBot document
> Title: fiber.cond():wait() throws if fiber is cancelled
>
> Currently fiber.cond():wait() throws an error if waiting fiber is
> cancelled like in case with fiber.channel():get().
> ---
>

Re: [Tarantool-patches] [PATCH] lua: handle fiber cancellation for fiber.cond

[Sergey Ostanevich]

Hi Oleg!

Thanks for the comment, I believe I have to resend the patch along with
C API changes.

What I see at the moment - failures like 

[076]  test_run:wait_upstream(1, {message_re = 'Missing %.xlog file',
status = 'loading'})
[076]  ---
[076] -- true
[076] +- false

which incurred by the intended behavior change, AFAIU. We no longer
return 'true' in case the fiber is cancelled. The wait_upstream returns
the cond wait result, hence the output differs.

Did you observe any other failures?

Regards,
Sergos


On 28 окт 20:14, Oleg Babin wrote:
> Hi! Thanks for the patch!
> 
> The main reason I didn't send this patch is inconsistency between Lua and C
> API.
> 
> To eliminate such inconsistency [1] was implemented. Unfortunately some test
> fails and I've failed to find root of the problem.
> 
> However I still believe that the problem should be solved at "core" level.
> 
> Anyway, it would be great if some solution will be pushed to upstream.
> 
> 
> [1] https://github.com/tarantool/tarantool/commit/e7a5120603f362364953315d31aa529342263e0b
> 
> 
> On 28/10/2020 19:22, sergos@tarantool.org wrote:
> > From: Sergey Ostanevich <sergos@tarantool.org>
> > 
> > Before this patch fiber.cond():wait() just returns for cancelled
> > fiber. In contrast fiber.channel():get() threw "fiber is
> > canceled" error.
> > This patch unify behaviour of channels and condvars and also fixes
> > related net.box module problem - it was impossible to interrupt
> > net.box call with fiber.cancel because it used fiber.cond under
> > the hood. Test cases for both bugs are added.
> > 
> > Closes #4834
> > Closes #5013
> > 
> > Co-authored-by: Oleg Babin <olegrok@tarantool.org>
> > 
> > @TarantoolBot document
> > Title: fiber.cond():wait() throws if fiber is cancelled
> > 
> > Currently fiber.cond():wait() throws an error if waiting fiber is
> > cancelled like in case with fiber.channel():get().
> > ---
> > 

Re: [Tarantool-patches] [PATCH] lua: handle fiber cancellation for fiber.cond

[Alexander Turenko]

On Thu, Oct 29, 2020 at 08:14:37PM +0300, Sergey Ostanevich wrote:
> Hi Oleg!
> 
> Thanks for the comment, I believe I have to resend the patch along with
> C API changes.
> 
> What I see at the moment - failures like 
> 
> [076]  test_run:wait_upstream(1, {message_re = 'Missing %.xlog file',
> status = 'loading'})
> [076]  ---
> [076] -- true
> [076] +- false
> 
> which incurred by the intended behavior change, AFAIU. We no longer
> return 'true' in case the fiber is cancelled. The wait_upstream returns
> the cond wait result, hence the output differs.

It just a loop with sleeps. I don't see any relation to your change. If
the fail is stable, it should be the reason to investigate it.

If it is actually intended behaviour change (I doubt that it is so), you
should fix the test.

Re: [Tarantool-patches] [PATCH] lua: handle fiber cancellation for fiber.cond

[Oleg Babin]

On 29/10/2020 20:14, Sergey Ostanevich wrote:
> Hi Oleg!
>
> Thanks for the comment, I believe I have to resend the patch along with
> C API changes.
>
> What I see at the moment - failures like
>
> [076]  test_run:wait_upstream(1, {message_re = 'Missing %.xlog file',
> status = 'loading'})
> [076]  ---
> [076] -- true
> [076] +- false
>
> which incurred by the intended behavior change, AFAIU. We no longer
> return 'true' in case the fiber is cancelled. The wait_upstream returns
> the cond wait result, hence the output differs.
>
> Did you observe any other failures?


Yes, seems something goes wrong with "applier" status. It looks like 
behaviour change.

[018] xlog/panic_on_wal_error.test.lua                                [ 
fail ]
[018]
[018] Test failed! Result content mismatch:
[018] --- xlog/panic_on_wal_error.result    Sun Sep  6 06:00:02 2020
[018] +++ xlog/panic_on_wal_error.reject    Sun Sep  6 06:05:59 2020
[018] @@ -134,14 +134,18 @@
[018]  ...
[018]  while box.info.replication[1].upstream.status ~= "stopped" do 
fiber.sleep(0.001) end
[018]  ---
[018] +- error: '[string "while 
box.info.replication[1].upstream.status..."]:1: attempt to
[018] +    index field ''upstream'' (a nil value)'
[018]  ...
[018]  box.info.replication[1].upstream.status
[018]  ---
[018] -- stopped
[018] +- error: '[string "return box.info.replication[1].upstream.status 
"]:1: attempt to
[018] +    index field ''upstream'' (a nil value)'
[018]  ...
[018]  box.info.replication[1].upstream.message
[018]  ---
[018] -- 'Missing .xlog file between LSN 6 {1: 6} and 8 {1: 8}'
[018] +- error: '[string "return 
box.info.replication[1].upstream.message "]:1: attempt to
[018] +    index field ''upstream'' (a nil value)'
[018]  ...
[018]  box.space.test:select{}


Also see https://travis-ci.org/github/tarantool/tarantool/jobs/724596598

> Regards,
> Sergos
>
>
> On 28 окт 20:14, Oleg Babin wrote:
>> Hi! Thanks for the patch!
>>
>> The main reason I didn't send this patch is inconsistency between Lua and C
>> API.
>>
>> To eliminate such inconsistency [1] was implemented. Unfortunately some test
>> fails and I've failed to find root of the problem.
>>
>> However I still believe that the problem should be solved at "core" level.
>>
>> Anyway, it would be great if some solution will be pushed to upstream.
>>
>>
>> [1] https://github.com/tarantool/tarantool/commit/e7a5120603f362364953315d31aa529342263e0b
>>
>>
>> On 28/10/2020 19:22, sergos@tarantool.org wrote:
>>> From: Sergey Ostanevich <sergos@tarantool.org>
>>>
>>> Before this patch fiber.cond():wait() just returns for cancelled
>>> fiber. In contrast fiber.channel():get() threw "fiber is
>>> canceled" error.
>>> This patch unify behaviour of channels and condvars and also fixes
>>> related net.box module problem - it was impossible to interrupt
>>> net.box call with fiber.cancel because it used fiber.cond under
>>> the hood. Test cases for both bugs are added.
>>>
>>> Closes #4834
>>> Closes #5013
>>>
>>> Co-authored-by: Oleg Babin <olegrok@tarantool.org>
>>>
>>> @TarantoolBot document
>>> Title: fiber.cond():wait() throws if fiber is cancelled
>>>
>>> Currently fiber.cond():wait() throws an error if waiting fiber is
>>> cancelled like in case with fiber.channel():get().
>>> ---
>>>