From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id F0CFA71814; Tue, 23 Feb 2021 03:19:26 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org F0CFA71814 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1614039567; bh=dvLHOT9xJdXjpa4a4JZKvrsc/M3EiDsfc80d2NBmJzY=; h=To:Date:In-Reply-To:References:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=KNYBHvHX6sNXCpVwLQaMaH6hCQrpQE7NPeAnb5mqejEARRpTSIupecALIrfTAbW5L DwZFo3qFDylpD7Hd193MSDM+gIt9iObxfGdHJ5HDyBlCIzHPZYsP9wxS1OIb39MFRZ lnRC8Lgt5ren/4CEYtyUz4Zbgj7wfE4EcfPryRN8= Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 3DD7C71859 for ; Tue, 23 Feb 2021 03:15:57 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3DD7C71859 Received: by smtpng2.m.smailru.net with esmtpa (envelope-from ) id 1lELMq-0003CR-BR; Tue, 23 Feb 2021 03:15:56 +0300 To: tarantool-patches@dev.tarantool.org, olegrok@tarantool.org, yaroslav.dynnikov@tarantool.org Date: Tue, 23 Feb 2021 01:15:43 +0100 Message-Id: X-Mailer: git-send-email 2.24.3 (Apple Git-128) In-Reply-To: References: MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD975C3EC174F56692243410BA6471F0166336C1783AA96243D182A05F5380850403CAF8F4B99A4ECFFC8FDFE937F97879FBFB544AB7F27E6B8C4049D2908E99D2E X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7D6964C9E324ABA58EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063726CA83C7ABDB938E8638F802B75D45FF5571747095F342E8C7A0BC55FA0FE5FC425FEB8730D38403EA6D82A221B910B6950322F076DEB4DE389733CBF5DBD5E913377AFFFEAFD269176DF2183F8FC7C04CF195F1528592878941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B6AEEA5BB16A939343CC7F00164DA146DA6F5DAA56C3B73B23C77107234E2CFBA567F23339F89546C55F5C1EE8F4F765FC08F9A42B2210255C75ECD9A6C639B01BBD4B6F7A4D31EC0BC0CAF46E325F83A522CA9DD8327EE4930A3850AC1BE2E73579C543ECCDAE434EC4224003CC836476C0CAF46E325F83A50BF2EBBBDD9D6B0F347543BADC64E7283B503F486389A921A5CC5B56E945C8DA X-C1DE0DAB: C20DE7B7AB408E4181F030C43753B8186998911F362727C4C7A0BC55FA0FE5FC425FEB8730D38403EA6D82A221B910B6171BF86C7C799925B1881A6453793CE9C32612AADDFBE061C61BE10805914D3804EBA3D8E7E5B87ABF8C51168CD8EBDB587F3D2152687E5CDC48ACC2A39D04F89CDFB48F4795C241BDAD6C7F3747799A X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D348F343DA43F62289F65CE4F479B318E8887AE405A1D7BA1DE1422D62C1387FE443796D6990A31C8561D7E09C32AA3244C829576CB1FF42C51F06C89E1065C53F0A95CA90A1D8AC565FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj2drqE2xHc+EW0o5hWwOHfg== X-Mailru-Sender: 689FA8AB762F73936BC43F508A0638223BC365C0C027FD807D81EE3CE12BB65E3841015FED1DE5223CC9A89AB576DD93FB559BB5D741EB963CF37A108A312F5C27E8A8C3839CE0E267EA787935ED9F1B X-Mras: Ok Subject: [Tarantool-patches] [PATCH vshard 05/11] util: introduce safe fiber_cond_wait() X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Vladislav Shpilevoy via Tarantool-patches Reply-To: Vladislav Shpilevoy Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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)