Re: [Tarantool-patches] [PATCH vshard 05/11] util: introduce safe fiber_cond_wait()

[Oleg Babin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi! Thanks for your patch. LGTM.

I see several usages of cond:wait() in code. Maybe after introducing 
this helper you could use it.

E.g. in "bucket_send_xc" function.

On 23.02.2021 03:15, Vladislav Shpilevoy wrote:
> Original fiber_cond:wait() has a few issues:
>
> - Raises exception when fiber is canceled, which makes it
>    inapplicable in exception-intolerant code;
>
> - Raises an ugly misleading usage exception when timeout is
>    negative which easily can happen if the caller's code calls
>    wait() multiple times retrying something and does not want to
>    bother with doing a part of cond's job;
>
> - When fails, pushes an error of type 'TimedOut' which is not the
>    same as 'ClientError' with box.error.TIMEOUT code. The latter is
>    used wider, at least in vshard.
>
> The patch introduces util.fiber_cond_wait() function which fixes
> the mentioned issues.
>
> It is needed in the future map-reduce subsystem modules revolving
> around waiting on various conditions.
>
> Part of #147
> ---
>   test/unit/util.result   | 82 +++++++++++++++++++++++++++++++++++++++++
>   test/unit/util.test.lua | 31 ++++++++++++++++
>   vshard/util.lua         | 35 ++++++++++++++++++
>   3 files changed, 148 insertions(+)
>
> diff --git a/test/unit/util.result b/test/unit/util.result
> index 42a361a..679c087 100644
> --- a/test/unit/util.result
> +++ b/test/unit/util.result
> @@ -184,3 +184,85 @@ t ~= res
>   ---
>   - true
>   ...
> +--
> +-- Exception-safe cond wait.
> +--
> +cond_wait = util.fiber_cond_wait
> +---
> +...
> +cond = fiber.cond()
> +---
> +...
> +ok, err = cond_wait(cond, -1)
> +---
> +...
> +assert(not ok and err.message)
> +---
> +- Timeout exceeded
> +...
> +-- Ensure it does not return 'false' like pcall(). It must conform to nil,err
> +-- signature.
> +assert(type(ok) == 'nil')
> +---
> +- true
> +...
> +ok, err = cond_wait(cond, 0)
> +---
> +...
> +assert(not ok and err.message)
> +---
> +- Timeout exceeded
> +...
> +ok, err = cond_wait(cond, 0.000001)
> +---
> +...
> +assert(not ok and err.message)
> +---
> +- Timeout exceeded
> +...
> +ok, err = nil
> +---
> +...
> +_ = fiber.create(function() ok, err = cond_wait(cond, 1000000) end)
> +---
> +...
> +fiber.yield()
> +---
> +...
> +cond:signal()
> +---
> +...
> +_ = test_run:wait_cond(function() return ok or err end)
> +---
> +...
> +assert(ok and not err)
> +---
> +- true
> +...
> +ok, err = nil
> +---
> +...
> +f = fiber.create(function() ok, err = cond_wait(cond, 1000000) end)
> +---
> +...
> +fiber.yield()
> +---
> +...
> +f:cancel()
> +---
> +...
> +_ = test_run:wait_cond(function() return ok or err end)
> +---
> +...
> +assert(not ok)
> +---
> +- true
> +...
> +err.message
> +---
> +- fiber is cancelled
> +...
> +assert(type(err) == 'table')
> +---
> +- true
> +...
> diff --git a/test/unit/util.test.lua b/test/unit/util.test.lua
> index 9550a95..df3db6f 100644
> --- a/test/unit/util.test.lua
> +++ b/test/unit/util.test.lua
> @@ -76,3 +76,34 @@ yield_count
>   t
>   res
>   t ~= res
> +
> +--
> +-- Exception-safe cond wait.
> +--
> +cond_wait = util.fiber_cond_wait
> +cond = fiber.cond()
> +ok, err = cond_wait(cond, -1)
> +assert(not ok and err.message)
> +-- Ensure it does not return 'false' like pcall(). It must conform to nil,err
> +-- signature.
> +assert(type(ok) == 'nil')
> +ok, err = cond_wait(cond, 0)
> +assert(not ok and err.message)
> +ok, err = cond_wait(cond, 0.000001)
> +assert(not ok and err.message)
> +
> +ok, err = nil
> +_ = fiber.create(function() ok, err = cond_wait(cond, 1000000) end)
> +fiber.yield()
> +cond:signal()
> +_ = test_run:wait_cond(function() return ok or err end)
> +assert(ok and not err)
> +
> +ok, err = nil
> +f = fiber.create(function() ok, err = cond_wait(cond, 1000000) end)
> +fiber.yield()
> +f:cancel()
> +_ = test_run:wait_cond(function() return ok or err end)
> +assert(not ok)
> +err.message
> +assert(type(err) == 'table')
> diff --git a/vshard/util.lua b/vshard/util.lua
> index 2362607..d78f3a5 100644
> --- a/vshard/util.lua
> +++ b/vshard/util.lua
> @@ -1,6 +1,7 @@
>   -- vshard.util
>   local log = require('log')
>   local fiber = require('fiber')
> +local lerror = require('vshard.error')
>   
>   local MODULE_INTERNALS = '__module_vshard_util'
>   local M = rawget(_G, MODULE_INTERNALS)
> @@ -191,6 +192,39 @@ local function table_minus_yield(dst, src, interval)
>       return dst
>   end
>   
> +local function fiber_cond_wait_xc(cond, timeout)
> +    -- Handle negative timeout specifically - otherwise wait() will throw an
> +    -- ugly usage error.
> +    -- Don't trust this check to the caller's code, because often it just calls
> +    -- wait many times until it fails or the condition is met. Code looks much
> +    -- cleaner when it does not need to check the timeout sign. On the other
> +    -- hand, perf is not important here - anyway wait() yields which is slow on
> +    -- its own, but also breaks JIT trace recording which makes pcall() in the
> +    -- non-xc version of this function inconsiderable.
> +    if timeout < 0 or not cond:wait(timeout) then
> +        -- Don't use the original error if cond sets it. Because it sets
> +        -- TimedOut error. It does not have a proper error code, and may not be
> +        -- detected by router as a special timeout case if necessary. Or at
> +        -- least would complicate the handling in future. Instead, try to use a
> +        -- unified timeout error where possible.
> +        error(lerror.timeout())
> +    end
> +    -- Still possible though that the fiber is canceled and cond:wait() throws.
> +    -- This is why the _xc() version of this function throws even the timeout -
> +    -- anyway pcall() is inevitable.
> +end
> +
> +--
> +-- Exception-safe cond wait with unified errors in vshard format.
> +--
> +local function fiber_cond_wait(cond, timeout)
> +    local ok, err = pcall(fiber_cond_wait_xc, cond, timeout)
> +    if ok then
> +        return true
> +    end
> +    return nil, lerror.make(err)
> +end
> +
>   return {
>       tuple_extract_key = tuple_extract_key,
>       reloadable_fiber_create = reloadable_fiber_create,
> @@ -200,4 +234,5 @@ return {
>       version_is_at_least = version_is_at_least,
>       table_copy_yield = table_copy_yield,
>       table_minus_yield = table_minus_yield,
> +    fiber_cond_wait = fiber_cond_wait,
>   }