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 886866FC86; Sun, 25 Apr 2021 12:21:37 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 886866FC86 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1619342497; bh=2yUmvkxjzvan6iQ3p//IIS2F7LYvn3qboWzq8MidNFI=; 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=VXMCCHdmabIy39gwuG0Fw9vFoVb9b7t0lBx23waKi8upN5KT6mqRkMniDodQHoea0 rrKVufeCoOczCRvahWrQATDBEzzOCDA/7nnP8H6LdSPNWcctGoL2fCE5Y48NPSi1OY b+4gFLKGroaVH16A7n56KekZsJwuXwZNZ64vE0ho= Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [94.100.177.99]) (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 739DD6FC86 for ; Sun, 25 Apr 2021 12:21:36 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 739DD6FC86 Received: by smtp39.i.mail.ru with esmtpa (envelope-from ) id 1laaxL-0005IE-Lj; Sun, 25 Apr 2021 12:21:36 +0300 To: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org, gorcunov@gmail.com References: <8c48e98e0c9780e2fe4ffbf511ac95678561fcfe.1619219639.git.v.shpilevoy@tarantool.org> Message-ID: <2e89c2b4-fd13-2ca1-9c05-f8e95d2e92e6@tarantool.org> Date: Sun, 25 Apr 2021 12:21:35 +0300 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.9.1 MIME-Version: 1.0 In-Reply-To: <8c48e98e0c9780e2fe4ffbf511ac95678561fcfe.1619219639.git.v.shpilevoy@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD9203E2ABA940B7548CA3406377FA04A72641288DF385C0094182A05F538085040DE0C380DC2CE8D22FFB2F026A195E3E9F86E171CD335ED95440FF2FDFDCB7193 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7A7819E2C739EB5BBEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063702DFA59B3C994360EA1F7E6F0F101C67CDEEF6D7F21E0D1D9295C2E9FA3191EE1B59CA4C82EFA6581AB2502A2122D96D2B1CE22EAEC66F62F6B57BC7E64490618DEB871D839B73339E8FC8737B5C22498424CA1AAF98A6958941B15DA834481FCF19DD082D7633A0EF3E4896CB9E6436389733CBF5DBD5E9D5E8D9A59859A8B6D082881546D93491CC7F00164DA146DA6F5DAA56C3B73B237318B6A418E8EAB8D32BA5DBAC0009BE9E8FC8737B5C2249CDA8EF9AFDB9C29A76E601842F6C81A12EF20D2F80756B5F7E9C4E3C761E06A776E601842F6C81A127C277FBC8AE2E8B1810775DF687DAD93AA81AA40904B5D9DBF02ECDB25306B2201CA6A4E26CD07C3BBE47FD9DD3FB595F5C1EE8F4F765FC72CEEB2601E22B093A03B725D353964B0B7D0EA88DDEDAC722CA9DD8327EE4930A3850AC1BE2E73533F64B4C514AF25EC4224003CC83647689D4C264860C145E X-C1DE0DAB: 0D63561A33F958A5B08105B5E6A51CBC0C23542A4FDEE7B4BC49FB149B8CA219D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA7502E6951B79FF9A3F410CA545F18667F91A7EA1CDA0B5A7A0 X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D343D1F112031EF3D62DFFE7D89144E7CD4027B833ABF5BCA8B301555E1B1B00A834264752027967D981D7E09C32AA3244C3DBE643A6790C0405F0E77BBF12ECA941E098CBE561D6343FACE5A9C96DEB163 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2bioj08M52wfuxcHjFzBrqJQ4DQ== X-Mailru-Sender: 583F1D7ACE8F49BDD2846D59FC20E9F8CAEC231191DC3E12561C00CBC3AB96C902C5CE79F7D8EA61424AE0EB1F3D1D21E2978F233C3FAE6EE63DB1732555E4A8EE80603BA4A5B0BC112434F685709FCF0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH 1/2] fiber: make wakeup in Lua and C nop on self 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: Serge Petrenko via Tarantool-patches Reply-To: Serge Petrenko Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 24.04.2021 02:15, Vladislav Shpilevoy пишет: > fiber.wakeup() in Lua and fiber_wakeup() in C could lead to a > crash or undefined behaviour when called on the currently running > fiber. > > In particular, if after wakeup was started a new fiber in a > blocking way (fiber.create() and fiber_start()) it would crash in > debug build, and lead to unknown results in release. > > If after wakeup was made a sleep with non-zero timeout or an > infinite yield (fiber_yield()), the fiber woke up in the same > event loop iteration regardless of any timeout or other wakeups. > It was a spurious wakeup, which is not expected in most of the > places internally. > > The patch makes the wakeup nop on the current fiber making it safe > to use anywhere. > > Closes #5292 > Closes #6043 > > @TarantoolBot document > Title: fiber.wakeup() in Lua and fiber_wakeup() in C are nop on self > In Lua `fiber.wakeup()` being called on the current fiber does not > do anything, safe to use. The same for `fiber_wakeup()` in C. > --- Thanks for the patch! LGTM, generally, with one test-related comment. ... > diff --git a/test/app-tap/gh-6043-fiber-wakeup-self.test.lua b/test/app-tap/gh-6043-fiber-wakeup-self.test.lua > new file mode 100755 > index 000000000..082c1228d > --- /dev/null > +++ b/test/app-tap/gh-6043-fiber-wakeup-self.test.lua > @@ -0,0 +1,50 @@ > +#!/usr/bin/env tarantool > + > +local tap = require('tap') > +local fiber = require('fiber') > + > +-- > +-- gh-6043: fiber.wakeup() on self could lead to a crash if followed by > +-- fiber.create(). Also it could lead to spurious wakeup if followed by a sleep > +-- with non-zero timeout, or by, for example, an infinite yield such as WAL > +-- write. > +-- > + > +local function test_wakeup_self_and_call(test) > + test:plan(3) > + > + local self = fiber.self() > + fiber.wakeup(self) > + local count = 0 > + local f = fiber.create(function() > + count = count + 1 > + fiber.yield() > + count = count + 1 > + end) > + test:is(count, 1, 'created a fiber') > + fiber.yield() > + test:is(count, 2, 'it yielded') > + test:is(f:status(), 'dead', 'and ended') > +end > + > +local function test_wakeup_self_and_wal_write(test) > + test:plan(1) > + > + local s = box.schema.create_space('test') > + s:create_index('pk') > + > + fiber.wakeup(fiber.self()) > + local lsn = box.info.lsn > + s:replace{1} > + test:is(box.info.lsn, lsn + 1, 'written to WAL') > + s:drop() > +end Again, shouldn't everything related to space creation, WAL writes and so on reside in box-tap ? I may be wrong, just asking. > + > +box.cfg{} > + > +local test = tap.test('gh-6043-fiber-wakeup-self') > +test:plan(2) > +test:test('wakeup self + call', test_wakeup_self_and_call) > +test:test('wakeup self + wal write', test_wakeup_self_and_wal_write) > + > +os.exit(test:check() and 0 or 1) > diff --git a/test/unit/fiber.cc b/test/unit/fiber.cc > index 9c1a23bdd..a1e3ded87 100644 > --- a/test/unit/fiber.cc > +++ b/test/unit/fiber.cc > @@ -138,8 +138,7 @@ fiber_join_test() > fiber_set_joinable(fiber, true); > fiber_wakeup(fiber); > /** Let the fiber schedule */ > - fiber_wakeup(fiber()); > - fiber_yield(); > + fiber_reschedule(); > note("by this time the fiber should be dead already"); > fiber_cancel(fiber); > fiber_join(fiber); > @@ -201,12 +200,42 @@ fiber_name_test() > footer(); > } > > +static void > +fiber_wakeup_self_test() > +{ > + header(); > + > + struct fiber *f = fiber(); > + > + fiber_wakeup(f); > + double duration = 0.001; > + uint64_t t1 = fiber_clock64(); > + fiber_sleep(duration); > + uint64_t t2 = fiber_clock64(); > + /* > + * It was a real sleep, not 0 duration. Wakeup is nop on the running > + * fiber. > + */ > + assert(t2 - t1 >= duration); > + > + /* > + * Wakeup + start of a new fiber. This is different from yield but > + * works without crashes too. > + */ > + struct fiber *newf = fiber_new_xc("nop", noop_f); > + fiber_wakeup(f); > + fiber_start(newf); > + > + footer(); > +} > + > static int > main_f(va_list ap) > { > fiber_name_test(); > fiber_join_test(); > fiber_stack_test(); > + fiber_wakeup_self_test(); > ev_break(loop(), EVBREAK_ALL); > return 0; > } > diff --git a/test/unit/fiber.result b/test/unit/fiber.result > index a61e0a2b8..36bf2a599 100644 > --- a/test/unit/fiber.result > +++ b/test/unit/fiber.result > @@ -17,3 +17,5 @@ SystemError Failed to allocate 42 bytes in allocator for exception: Cannot alloc > # normal-stack fiber not crashed > # big-stack fiber not crashed > *** fiber_stack_test: done *** > + *** fiber_wakeup_self_test *** > + *** fiber_wakeup_self_test: done *** > diff --git a/test/unit/fiber_stress.cc b/test/unit/fiber_stress.cc > index 44ab12487..aa892d0a0 100644 > --- a/test/unit/fiber_stress.cc > +++ b/test/unit/fiber_stress.cc > @@ -9,10 +9,8 @@ enum { > static int > yield_f(va_list ap) > { > - for (int i = 0; i < ITERATIONS; i++) { > - fiber_wakeup(fiber()); > - fiber_yield(); > - } > + for (int i = 0; i < ITERATIONS; i++) > + fiber_reschedule(); > return 0; > } > -- Serge Petrenko