From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 173EB234C0 for ; Wed, 18 Jul 2018 08:25:19 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id MxzVgnMxJU6G for ; Wed, 18 Jul 2018 08:25:19 -0400 (EDT) Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id C2F5C2332B for ; Wed, 18 Jul 2018 08:25:18 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH] Protect lua socket io against a spurious wakeup References: <661cb14302af460fb3779040fb9258ae37c1075e.1531914062.git.georgy@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Wed, 18 Jul 2018 15:25:16 +0300 MIME-Version: 1.0 In-Reply-To: <661cb14302af460fb3779040fb9258ae37c1075e.1531914062.git.georgy@tarantool.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: tarantool-patches@freelists.org, Georgy Kirichenko Hello. Thanks for the patch! See 6 comments below. On 18/07/2018 14:43, Georgy Kirichenko wrote: > socket_readable/socket_writable might return until socket become a > requested state in case of a spurious wakeup. Socket functions are > refactored with considering that fact. > > Old behavior leads to test failures. Are you sure, that the only reason is the tests? Please, explain here briefly, why it is important to track spurious wakeups and cancels. > --- > Issue: https://github.com/tarantool/tarantool/issues/3344 > Branch: > https://github.com/tarantool/tarantool/tree/g.kirichenko/gh-3344-socket-io-spurios-wakeup > src/lua/socket.lua | 54 ++++++++++++++++++++++----------------- > test/box/net.box.result | 3 +++ > test/box/net.box.test.lua | 2 ++ > 3 files changed, 35 insertions(+), 24 deletions(-) > > diff --git a/src/lua/socket.lua b/src/lua/socket.lua > index 06306eae2..0b22bb91c 100644 > --- a/src/lua/socket.lua > +++ b/src/lua/socket.lua > @@ -689,12 +689,8 @@ local function read(self, limit, timeout, check, ...) > return nil > end > > - if not socket_readable(self, timeout) then > - return nil > - end > - if timeout <= 0 then > - break > - end > + socket_readable(self, timeout) > + fiber.testcancel() 1. I see that you put this testcancel after each wait in this file. But why can not you just put it inside wait? This is obvious code duplication. > timeout = timeout - ( fiber.clock() - started ) > end > self._errno = boxerrno.ETIMEDOUT > @@ -736,7 +732,7 @@ local function socket_write(self, octets, timeout) > end > > local started = fiber.clock() > - while true do > + while timeout > 0 do 2. Why? As I remember, Kostja said, that timeout 0 is ok and means that we have only one attempt to do an action (like write, read ...). And how is it linked with the patch? > local written = syswrite(self, p, e - p) > if written == 0 then > return p - s -- eof > @@ -935,16 +930,21 @@ local function socket_tcp_connect(s, address, port, timeout) > -- Wait until the connection is established or ultimately fails. > -- In either condition the socket becomes writable. To tell these > -- conditions appart SO_ERROR must be consulted (man connect). > - if socket_writable(s, timeout) then > - s._errno = socket_getsockopt(s, 'SOL_SOCKET', 'SO_ERROR') > - else > - s._errno = boxerrno.ETIMEDOUT > - end > - if s._errno ~= 0 then > - return nil > + local deadline = timeout + fiber.clock() > + while deadline - fiber.clock() > 0 do > + if socket_writable(s, deadline - fiber.clock()) then 3. If the socket became writable, and the fiber was canceled, then you will return true/nil with no testcancel. > + s._errno = socket_getsockopt(s, 'SOL_SOCKET', 'SO_ERROR') > + if s._errno == 0 then > + return true > + else > + return nil > + end > + end > + -- timeout, spurious wakeup or cancel > + fiber.testcancel() > end > - -- Connected > - return true > + s._errno = boxerrno.ETIMEDOUT > + return nil > end > > local function tcp_connect(host, port, timeout) > @@ -1005,7 +1005,9 @@ end > local function tcp_server_loop(server, s, addr) > fiber.name(format("%s/%s:%s", server.name, addr.host, addr.port), {truncate = true}) > log.info("started") > - while socket_readable(s) do > + while true do > + fiber.testcancel() > + socket_readable(s) 4. Why cancel() is before readable()? > local sc, from = socket_accept(s) > if sc == nil then > local errno = s._errno > @@ -1335,7 +1339,7 @@ local function lsocket_tcp_accept(self) > if not errno_is_transient[errno] then > break > end > - until not socket_readable(self, deadline - fiber.clock()) > + end > return nil, socket_error(self) > end > > @@ -1390,7 +1394,9 @@ local function lsocket_tcp_receive(self, pattern, prefix) > elseif pattern == "*a" then > local result = { prefix } > local deadline = fiber.clock() + (self.timeout or TIMEOUT_INFINITY) > - repeat > + while deadline - fiber.clock() > 0 do 5. Same as 2. We allow 0 timeout. > + socket_readable(self, deadline - fiber.clock()) > + fiber.testcancel() > local data = socket_sysread(self) > if data == nil then > if not errno_is_transient[self._errno] then > diff --git a/test/box/net.box.result b/test/box/net.box.result > index 21cff4a11..4bafabdcb 100644 > --- a/test/box/net.box.result > +++ b/test/box/net.box.result > @@ -20,6 +20,9 @@ test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:: '") > --- > - true > ... > +fiber.wakeup(fiber.self()) 6. No link to the issue. Both in the commit message and before the test. > +--- > +... > test_run:cmd("setopt delimiter ';'") > --- > - true