Tarantool development patches archive
 help / color / mirror / Atom feed
From: Oleg Babin via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>,
	tarantool-patches@dev.tarantool.org,
	yaroslav.dynnikov@tarantool.org
Subject: Re: [Tarantool-patches] [PATCH vshard 05/11] util: introduce safe fiber_cond_wait()
Date: Wed, 24 Feb 2021 13:27:41 +0300
Message-ID: <8be0f758-10ae-98e5-d6f5-359db7eaf7cd@tarantool.org> (raw)
In-Reply-To: <f5ca6e65c2505e68f9202cee57d6598d66d0be67.1614039039.git.v.shpilevoy@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,
>   }

  reply	other threads:[~2021-02-24 10:29 UTC|newest]

Thread overview: 47+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-23  0:15 [Tarantool-patches] [PATCH vshard 00/11] VShard Map-Reduce, part 2: Ref, Sched, Map Vladislav Shpilevoy via Tarantool-patches
2021-02-23  0:15 ` [Tarantool-patches] [PATCH vshard 01/11] error: introduce vshard.error.timeout() Vladislav Shpilevoy via Tarantool-patches
2021-02-24 10:27   ` Oleg Babin via Tarantool-patches
2021-02-24 21:46     ` Vladislav Shpilevoy via Tarantool-patches
2021-02-25 12:42       ` Oleg Babin via Tarantool-patches
2021-02-23  0:15 ` [Tarantool-patches] [PATCH vshard 10/11] sched: introduce vshard.storage.sched module Vladislav Shpilevoy via Tarantool-patches
2021-02-24 10:28   ` Oleg Babin via Tarantool-patches
2021-02-24 21:50     ` Vladislav Shpilevoy via Tarantool-patches
2021-03-04 21:02   ` Oleg Babin via Tarantool-patches
2021-03-05 22:06     ` Vladislav Shpilevoy via Tarantool-patches
2021-03-09  8:03       ` Oleg Babin via Tarantool-patches
2021-02-23  0:15 ` [Tarantool-patches] [PATCH vshard 11/11] router: introduce map_callrw() Vladislav Shpilevoy via Tarantool-patches
2021-02-24 10:28   ` Oleg Babin via Tarantool-patches
2021-02-24 22:04     ` Vladislav Shpilevoy via Tarantool-patches
2021-02-25 12:43       ` Oleg Babin via Tarantool-patches
2021-02-26 23:58         ` Vladislav Shpilevoy via Tarantool-patches
2021-03-01 10:58           ` Oleg Babin via Tarantool-patches
2021-02-23  0:15 ` [Tarantool-patches] [PATCH vshard 02/11] storage: add helper for local functions invocation Vladislav Shpilevoy via Tarantool-patches
2021-02-24 10:27   ` Oleg Babin via Tarantool-patches
2021-02-23  0:15 ` [Tarantool-patches] [PATCH vshard 03/11] storage: cache bucket count Vladislav Shpilevoy via Tarantool-patches
2021-02-24 10:27   ` Oleg Babin via Tarantool-patches
2021-02-24 21:47     ` Vladislav Shpilevoy via Tarantool-patches
2021-02-25 12:42       ` Oleg Babin via Tarantool-patches
2021-02-23  0:15 ` [Tarantool-patches] [PATCH vshard 04/11] registry: module for circular deps resolution Vladislav Shpilevoy via Tarantool-patches
2021-02-24 10:27   ` Oleg Babin via Tarantool-patches
2021-02-23  0:15 ` [Tarantool-patches] [PATCH vshard 05/11] util: introduce safe fiber_cond_wait() Vladislav Shpilevoy via Tarantool-patches
2021-02-24 10:27   ` Oleg Babin via Tarantool-patches [this message]
2021-02-24 21:48     ` Vladislav Shpilevoy via Tarantool-patches
2021-02-25 12:42       ` Oleg Babin via Tarantool-patches
2021-02-23  0:15 ` [Tarantool-patches] [PATCH vshard 06/11] util: introduce fiber_is_self_canceled() Vladislav Shpilevoy via Tarantool-patches
2021-02-24 10:27   ` Oleg Babin via Tarantool-patches
2021-02-23  0:15 ` [Tarantool-patches] [PATCH vshard 07/11] storage: introduce bucket_generation_wait() Vladislav Shpilevoy via Tarantool-patches
2021-02-24 10:27   ` Oleg Babin via Tarantool-patches
2021-02-23  0:15 ` [Tarantool-patches] [PATCH vshard 08/11] storage: introduce bucket_are_all_rw() Vladislav Shpilevoy via Tarantool-patches
2021-02-24 10:27   ` Oleg Babin via Tarantool-patches
2021-02-24 21:48     ` Vladislav Shpilevoy via Tarantool-patches
2021-02-23  0:15 ` [Tarantool-patches] [PATCH vshard 09/11] ref: introduce vshard.storage.ref module Vladislav Shpilevoy via Tarantool-patches
2021-02-24 10:28   ` Oleg Babin via Tarantool-patches
2021-02-24 21:49     ` Vladislav Shpilevoy via Tarantool-patches
2021-02-25 12:42       ` Oleg Babin via Tarantool-patches
2021-03-04 21:22   ` Oleg Babin via Tarantool-patches
2021-03-05 22:06     ` Vladislav Shpilevoy via Tarantool-patches
2021-03-09  8:03       ` Oleg Babin via Tarantool-patches
2021-03-21 18:49   ` Vladislav Shpilevoy via Tarantool-patches
2021-03-12 23:13 ` [Tarantool-patches] [PATCH vshard 00/11] VShard Map-Reduce, part 2: Ref, Sched, Map Vladislav Shpilevoy via Tarantool-patches
2021-03-15  7:05   ` Oleg Babin via Tarantool-patches
2021-03-28 18:17 ` Vladislav Shpilevoy via Tarantool-patches

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8be0f758-10ae-98e5-d6f5-359db7eaf7cd@tarantool.org \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=olegrok@tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --cc=yaroslav.dynnikov@tarantool.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Tarantool development patches archive

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://lists.tarantool.org/tarantool-patches/0 tarantool-patches/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 tarantool-patches tarantool-patches/ https://lists.tarantool.org/tarantool-patches \
		tarantool-patches@dev.tarantool.org.
	public-inbox-index tarantool-patches

Example config snippet for mirrors.


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git