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 745F870202; Wed, 24 Feb 2021 13:29:21 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 745F870202 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1614162561; bh=Otil3FJgZd+GGz31QIRZtsDUlMa3XJlS9UraueThs60=; h=To:References:Date:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=gXPlqmx5hYLOsfefOEf/af0NamoQVrVhvHM70OAnBK59p8t8wXndonEzz3vnIkgwx Ix+goe4zqGsDCciGufbN+fUqUernV4SQdqe52B3gR+L03olIww6FMnrWmvH+qBqTxb rSek95oAXVnLlTHbdD+vuPzg6bDrhC8j/S3hreBI= Received: from smtp46.i.mail.ru (smtp46.i.mail.ru [94.100.177.106]) (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 4066071827 for ; Wed, 24 Feb 2021 13:27:43 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 4066071827 Received: by smtp46.i.mail.ru with esmtpa (envelope-from ) id 1lErOQ-0004b8-C2; Wed, 24 Feb 2021 13:27:42 +0300 To: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org, yaroslav.dynnikov@tarantool.org References: Message-ID: <8be0f758-10ae-98e5-d6f5-359db7eaf7cd@tarantool.org> Date: Wed, 24 Feb 2021 13:27:41 +0300 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.16; rv:78.0) Gecko/20100101 Thunderbird/78.7.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-GB X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD975C3EC174F5669221340953D8FEAAFCA26BB79E5C093AA91182A05F53808504065C620510CBA14A09B942F6A2F75B8FC00BB2B5F1211AEADA2F4C925627A32F5 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE721B3E54BB37EA0B4EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006371E4BC0E00C009995EA1F7E6F0F101C674E70A05D1297E1BBC6CDE5D1141D2B1CC8CD956854CCCECA3854489DD13BB3D6CB72A979199E65699FA2833FD35BB23D9E625A9149C048EE33AC447995A7AD18CB629EEF1311BF91D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8B60CDF180582EB8FBA471835C12D1D977C4224003CC836476EB9C4185024447017B076A6E789B0E975F5C1EE8F4F765FCFE2C9C77650FFF953AA81AA40904B5D9CF19DD082D7633A078D18283394535A93AA81AA40904B5D98AA50765F79006377F6B291853BA38E2D81D268191BDAD3D698AB9A7B718F8C442539A7722CA490C13377AFFFEAFD26923F8577A6DFFEA7CB59C7783CC88FA9693EC92FD9297F6715571747095F342E857739F23D657EF2BD5E8D9A59859A8B655EC1579764E9EAF089D37D7C0E48F6C5571747095F342E857739F23D657EF2B6825BDBE14D8E7028C9DFF55498CEFB0BD9CCCA9EDD067B1EDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A5AA68354491D572631A4547ADE2D2BA4B054B31A2A46EAE8ED59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75B7BFB303F1C7DB4D8E8E86DC7131B365E7726E8460B7C23C X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34A541DB4061E93832CA8D2097C59FA214885735BD7BE33331660B6EF2D87624206A876554AAEAC3461D7E09C32AA3244C86D7CFD06B38BEBF3C5EC247E0B1F11997FE24653F78E668FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojyK6JYJ15DtKkYBvbVnd13A== X-Mailru-Sender: 583F1D7ACE8F49BD9317CE1922F30C7E8D1ABD5923D024C633E931BD6B50CBA0D9DB16C68D118F3D23E75C7104EB1B885DEE61814008E47C7013064206BFB89F93956FB04BA385BE9437F6177E88F7363CDA0F3B3F5B9367 X-Mras: Ok Subject: Re: [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: Oleg Babin via Tarantool-patches Reply-To: Oleg Babin Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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, > }