[tarantool-patches] Re: [PATCH] Protect lua socket io against a spurious wakeup
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Wed Jul 18 15:25:16 MSK 2018
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...\"]:<line>: '")
> ---
> - 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
More information about the Tarantool-patches
mailing list