From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: tarantool-patches@freelists.org, Georgy Kirichenko <georgy@tarantool.org> Subject: [tarantool-patches] Re: [PATCH] Fix lua socket polling in case of a spurious wakeup Date: Wed, 18 Jul 2018 20:06:01 +0300 [thread overview] Message-ID: <e37d6392-c9a5-f9d3-5f19-e35f0f0476af@tarantool.org> (raw) In-Reply-To: <b91fde1b13a9f668766713a3089a5ddda1801318.1531932194.git.georgy@tarantool.org> Thanks for the fixes! Please, use [PATCH v2] prefix when sending second version of a patch. See 2 comments below. On 18/07/2018 19:46, Georgy Kirichenko wrote: > socket_writable/socket_readable may return before timeout is exceeded > with the false status in cause of a spurious wakeup and this should not be > treated as an EOF or an error. > > Fixed #3344 > --- > 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 | 57 ++++++++++++++++---------------- > test/app/socket.result | 70 ++++++++++++++++++++++++++++++++++++++++ > test/app/socket.test.lua | 29 +++++++++++++++++ > 3 files changed, 129 insertions(+), 27 deletions(-) > > diff --git a/src/lua/socket.lua b/src/lua/socket.lua > index 06306eae2..6ebcea055 100644 > --- a/src/lua/socket.lua > +++ b/src/lua/socket.lua > @@ -337,11 +337,15 @@ local function do_wait(self, what, timeout) > end 1. The function 'do_wait' now sets _errno to timeout even if it was actually not timeout but spurious wakeup. I think, we should not set any errors in such a case. Actually, do_wait is called from only three functions, so we can set _errno in them when the timeout really occurs. > > local function socket_readable(self, timeout) > - return do_wait(self, 1, timeout) ~= 0 > + local result = do_wait(self, 1, timeout) ~= 0 2. Function socket_readable now can return even if the socket actually is not readable because of spurious wakeup. It can lead to block on read() as I understand, if we are trying to read non-readable thing. And breaks logic of this function. I think, that here we should check do_wait result to actually contain 'R'. If not 'R' and the timeout still is not depleted, we should call do_wait again. > + fiber.testcancel() > + return result > end > > local function socket_writable(self, timeout) > - return do_wait(self, 2, timeout) ~= 0 > + local result = do_wait(self, 2, timeout) ~= 0 > + fiber.testcancel() > + return result > end > > local function socket_wait(self, timeout)
next prev parent reply other threads:[~2018-07-18 17:06 UTC|newest] Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-07-18 16:46 [tarantool-patches] " Georgy Kirichenko 2018-07-18 17:06 ` Vladislav Shpilevoy [this message] 2018-08-02 15:44 Georgy Kirichenko 2018-08-06 14:27 ` [tarantool-patches] " Vladislav Shpilevoy
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=e37d6392-c9a5-f9d3-5f19-e35f0f0476af@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=georgy@tarantool.org \ --cc=tarantool-patches@freelists.org \ --subject='[tarantool-patches] Re: [PATCH] Fix lua socket polling in case of a spurious wakeup' \ /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
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox