Tarantool development patches archive
 help / color / mirror / Atom feed
From: Vladislav Shpilevoy via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: tarantool-patches@dev.tarantool.org, olegrok@tarantool.org,
	yaroslav.dynnikov@tarantool.org
Subject: [Tarantool-patches] [PATCH vshard 05/11] util: introduce safe fiber_cond_wait()
Date: Tue, 23 Feb 2021 01:15:43 +0100
Message-ID: <f5ca6e65c2505e68f9202cee57d6598d66d0be67.1614039039.git.v.shpilevoy@tarantool.org> (raw)
In-Reply-To: <cover.1614039039.git.v.shpilevoy@tarantool.org>

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,
 }
-- 
2.24.3 (Apple Git-128)


  parent reply	other threads:[~2021-02-23  0:19 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 ` Vladislav Shpilevoy via Tarantool-patches [this message]
2021-02-24 10:27   ` [Tarantool-patches] [PATCH vshard 05/11] util: introduce safe fiber_cond_wait() Oleg Babin via Tarantool-patches
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=f5ca6e65c2505e68f9202cee57d6598d66d0be67.1614039039.git.v.shpilevoy@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