[tarantool-patches] Re: [PATCH] Protect lua socket io against a spurious wakeup

From: Vladislav Shpilevoy <v.shpilevoy@xxxxxxxxxxxxx>
To: tarantool-patches@xxxxxxxxxxxxx, Georgy Kirichenko <georgy@xxxxxxxxxxxxx>
Date: Wed, 18 Jul 2018 15:25:16 +0300

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

Follow-Ups:
- [tarantool-patches] Re: [PATCH] Protect lua socket io against a spurious wakeup
  - From: Georgy Kirichenko

References:
- [tarantool-patches] [PATCH] Protect lua socket io against a spurious wakeup
  - From: Georgy Kirichenko

[tarantool-patches] Re: [PATCH] Protect lua socket io against a spurious wakeup

Other related posts: