[Tarantool-patches] [PATCH v2] test: fix flaky socket test

[Tarantool-patches] [PATCH v2] test: fix flaky socket test

[Ilya Kosarev]

socket.test had a number of flaky problems:
- socket readiness expectation
- race conditions on socket shutdown in emulation test cases
- tcp_server stability in socket receive inconsistent behavior case
Now they are solved. Port randomization is improved.
Socket test is not fragile anymore.

Closes #4451, #4426, #4469
---
Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-4426-4451-fix-socket-test
Issues: https://github.com/tarantool/tarantool/issues/4426
        https://github.com/tarantool/tarantool/issues/4451
        https://github.com/tarantool/tarantool/issues/4469

Changes in v2:
- reconsider socket readiness expectation
- reduced conditions waiting time

 test/app/socket.result   | 80 +++++++++++++++++++++++++---------------
 test/app/socket.test.lua | 56 +++++++++++++++++++---------
 test/app/suite.ini       |  1 -
 3 files changed, 90 insertions(+), 47 deletions(-)

diff --git a/test/app/socket.result b/test/app/socket.result
index fd299424c96..0095b89b867 100644
--- a/test/app/socket.result
+++ b/test/app/socket.result
@@ -45,6 +45,9 @@ test_run:cmd("push filter '(error: .builtin/.*[.]lua):[0-9]+' to '\\1'")
 WAIT_COND_TIME = 10
 ---
 ...
+WAIT_TCP_CONNECT_TIME = 240
+---
+...
 socket('PF_INET', 'SOCK_STREAM', 'tcp121222');
 ---
 - null
@@ -107,7 +110,7 @@ s:nonblock(true)
 ---
 - true
 ...
-s:readable(.1)
+s:readable(WAIT_TCP_CONNECT_TIME)
 ---
 - true
 ...
@@ -308,7 +311,7 @@ sc:nonblock(true)
 ---
 - true
 ...
-sc:readable(.5)
+sc:readable(WAIT_TCP_CONNECT_TIME)
 ---
 - true
 ...
@@ -451,7 +454,7 @@ sc:sysconnect('127.0.0.1', s:name().port) or errno() == errno.EINPROGRESS
 ---
 - true
 ...
-sc:writable(10)
+sc:writable(WAIT_TCP_CONNECT_TIME)
 ---
 - true
 ...
@@ -754,7 +757,7 @@ string.match(tostring(sc), ', peer') == nil
 ---
 - true
 ...
-sc:writable()
+sc:writable(WAIT_TCP_CONNECT_TIME)
 ---
 - true
 ...
@@ -1074,6 +1077,15 @@ master:setsockopt('SOL_SOCKET', 'SO_REUSEADDR', true)
 ---
 - true
 ...
+seed = ''
+---
+...
+for d in string.gmatch(box.info.uuid, '%d') do  seed = seed .. d end
+---
+...
+math.randomseed(tonumber(seed))
+---
+...
 port = 32768 + math.random(0, 32767)
 ---
 ...
@@ -1822,8 +1834,14 @@ test_run:cmd("setopt delimiter ';'")
 ---
 - true
 ...
+socket_opened = true
 cfiber = fiber.create(function(sc, rch, wch)
-    while sc:send(wch:get()) and rch:put(sc:receive("*l")) do end
+    while socket_opened do
+        sc:send(wch:get())
+        local data = sc:receive("*l")
+        if not socket_opened then sc:close() end
+        rch:put(data)
+    end
 end, sc, rch, wch);
 ---
 ...
@@ -1936,6 +1954,9 @@ c:receive("*l")
 ---
 - 
 ...
+socket_opened = false
+---
+...
 wch:put("Fu")
 ---
 - true
@@ -1944,10 +1965,6 @@ c:send("354 Please type your message\n")
 ---
 - 29
 ...
-sc:close()
----
-- 1
-...
 c:receive("*l", "Line: ")
 ---
 - null
@@ -2816,7 +2833,7 @@ test_run:cmd("clear filter")
 ---
 - true
 ...
--- case: sicket receive inconsistent behavior
+-- case: socket receive inconsistent behavior
 chan = fiber.channel()
 ---
 ...
@@ -2826,41 +2843,46 @@ counter = 0
 fn = function(s) counter = 0; while true do s:write((tostring(counter)):rep(chan:get())); counter = counter + 1 end end
 ---
 ...
-srv = socket.tcp_server('0.0.0.0', 8888, fn)
+srv = nil
 ---
 ...
-s = socket.connect('localhost', 8888)
+test_run:cmd("setopt delimiter ';'")
 ---
+- true
 ...
-chan:put(5)
+test_run:wait_cond(function()
+    port = 32768 + math.random(0, 32767)
+    srv = socket.tcp_server('0.0.0.0', port, fn)
+    return srv ~= nil
+end, WAIT_COND_TIME);
 ---
 - true
 ...
-chan:put(5)
+receive1 = nil; receive2 = nil;
 ---
-- true
 ...
-s:receive(5)
+if srv ~= nil then
+    s = socket.connect('localhost', port)
+    chan:put(5)
+    chan:put(5)
+    receive1 = s:receive(5)
+    chan:put(5)
+    s:settimeout(1)
+    receive2 = s:receive('*a')
+    s:close()
+    srv:close()
+end;
 ---
-- '00000'
 ...
-chan:put(5)
+test_run:cmd("setopt delimiter ''");
 ---
 - true
 ...
-s:settimeout(1)
+receive1
 ---
-- 1
+- '00000'
 ...
-s:receive('*a')
+receive2
 ---
 - '1111122222'
 ...
-s:close()
----
-- 1
-...
-srv:close()
----
-- true
-...
diff --git a/test/app/socket.test.lua b/test/app/socket.test.lua
index c72d41763f4..be375a4215a 100644
--- a/test/app/socket.test.lua
+++ b/test/app/socket.test.lua
@@ -14,6 +14,7 @@ test_run = env.new()
 test_run:cmd("push filter '(error: .builtin/.*[.]lua):[0-9]+' to '\\1'")
 
 WAIT_COND_TIME = 10
+WAIT_TCP_CONNECT_TIME = 240
 
 socket('PF_INET', 'SOCK_STREAM', 'tcp121222');
 
@@ -39,7 +40,7 @@ s:nonblock(false)
 s:nonblock()
 s:nonblock(true)
 
-s:readable(.1)
+s:readable(WAIT_TCP_CONNECT_TIME)
 s:wait(.1)
 socket.iowait(s:fd(), 'RW')
 socket.iowait(s:fd(), 3)
@@ -100,7 +101,7 @@ sc = socket('PF_INET', 'SOCK_STREAM', 'tcp')
 sc:nonblock(false)
 sc:sysconnect('127.0.0.1', s:name().port)
 sc:nonblock(true)
-sc:readable(.5)
+sc:readable(WAIT_TCP_CONNECT_TIME)
 sc:sysread()
 string.match(tostring(sc), ', peer') ~= nil
 #sevres
@@ -140,7 +141,7 @@ s:listen(128)
 sc = socket('PF_INET', 'SOCK_STREAM', 'tcp')
 
 sc:sysconnect('127.0.0.1', s:name().port) or errno() == errno.EINPROGRESS
-sc:writable(10)
+sc:writable(WAIT_TCP_CONNECT_TIME)
 sc:write('Hello, world')
 
 sa, addr = s:accept()
@@ -243,7 +244,7 @@ sc:getsockopt('SOL_SOCKET', 'SO_ERROR')
 sc:nonblock(true)
 sc:sysconnect('127.0.0.1', 3458) or errno() == errno.EINPROGRESS or errno() == errno.ECONNREFUSED
 string.match(tostring(sc), ', peer') == nil
-sc:writable()
+sc:writable(WAIT_TCP_CONNECT_TIME)
 string.match(tostring(sc), ', peer') == nil
 socket_error = sc:getsockopt('SOL_SOCKET', 'SO_ERROR')
 socket_error == errno.ECONNREFUSED or socket_error == 0
@@ -343,6 +344,9 @@ s = nil
 -- random port
 master = socket('PF_INET', 'SOCK_STREAM', 'tcp')
 master:setsockopt('SOL_SOCKET', 'SO_REUSEADDR', true)
+seed = ''
+for d in string.gmatch(box.info.uuid, '%d') do  seed = seed .. d end
+math.randomseed(tonumber(seed))
 port = 32768 + math.random(0, 32767)
 -- SO_REUSEADDR allows to bind to the same source addr:port twice,
 -- so listen() can return EADDRINUSE and so we check it within
@@ -619,8 +623,14 @@ s:settimeout(100500)
 rch, wch = fiber.channel(1), fiber.channel(1)
 sc = socket.connect(host, port)
 test_run:cmd("setopt delimiter ';'")
+socket_opened = true
 cfiber = fiber.create(function(sc, rch, wch)
-    while sc:send(wch:get()) and rch:put(sc:receive("*l")) do end
+    while socket_opened do
+        sc:send(wch:get())
+        local data = sc:receive("*l")
+        if not socket_opened then sc:close() end
+        rch:put(data)
+    end
 end, sc, rch, wch);
 test_run:cmd("setopt delimiter ''");
 
@@ -651,9 +661,9 @@ rch:get()
 wch:put("DATA\n")
 c:receive(4)
 c:receive("*l")
+socket_opened = false
 wch:put("Fu")
 c:send("354 Please type your message\n")
-sc:close()
 c:receive("*l", "Line: ")
 c:receive()
 c:receive(10)
@@ -960,18 +970,30 @@ server:close()
 
 test_run:cmd("clear filter")
 
--- case: sicket receive inconsistent behavior
+-- case: socket receive inconsistent behavior
 chan = fiber.channel()
 counter = 0
 fn = function(s) counter = 0; while true do s:write((tostring(counter)):rep(chan:get())); counter = counter + 1 end end
-srv = socket.tcp_server('0.0.0.0', 8888, fn)
-s = socket.connect('localhost', 8888)
-chan:put(5)
-chan:put(5)
-s:receive(5)
-chan:put(5)
-s:settimeout(1)
-s:receive('*a')
-s:close()
-srv:close()
+srv = nil
+test_run:cmd("setopt delimiter ';'")
+test_run:wait_cond(function()
+    port = 32768 + math.random(0, 32767)
+    srv = socket.tcp_server('0.0.0.0', port, fn)
+    return srv ~= nil
+end, WAIT_COND_TIME);
+receive1 = nil; receive2 = nil;
+if srv ~= nil then
+    s = socket.connect('localhost', port)
+    chan:put(5)
+    chan:put(5)
+    receive1 = s:receive(5)
+    chan:put(5)
+    s:settimeout(1)
+    receive2 = s:receive('*a')
+    s:close()
+    srv:close()
+end;
+test_run:cmd("setopt delimiter ''");
+receive1
+receive2
 
diff --git a/test/app/suite.ini b/test/app/suite.ini
index 79432e29a76..dd802d98cf9 100644
--- a/test/app/suite.ini
+++ b/test/app/suite.ini
@@ -7,4 +7,3 @@ use_unix_sockets = True
 use_unix_sockets_iproto = True
 is_parallel = True
 pretest_clean = True
-fragile = socket.test.lua ; gh-4426 gh-4451
-- 
2.17.1

Re: [Tarantool-patches] [PATCH v2] test: fix flaky socket test

[Vladislav Shpilevoy]

Thanks for the fixes!

See 2 comments below.

> diff --git a/test/app/socket.result b/test/app/socket.result
> index fd299424c96..0095b89b867 100644
> --- a/test/app/socket.result
> +++ b/test/app/socket.result
> @@ -107,7 +110,7 @@ s:nonblock(true)
>  ---
>  - true
>  ...
> -s:readable(.1)
> +s:readable(WAIT_TCP_CONNECT_TIME)

1. How did you choose exactly these places to patch?
I see some other places, which also wait for an event,
but they still use a small timeout (in comparison with
240 secs). For example:

    socket.result line 233, 904, 919:
        s:readable(1)
        ---
        - true
        ...

    socket.result line 836, 848:
        s:readable(10)
        ---
        - true
        ...

    socket.result line 2870:
        s:settimeout(1)
        receive2 = s:receive('*a')

      Here :receive() has timeout 1 sec,
      not infinity.

> @@ -754,7 +757,7 @@ string.match(tostring(sc), ', peer') == nil
>  ---
>  - true
>  ...
> -sc:writable()
> +sc:writable(WAIT_TCP_CONNECT_TIME)

2. Here the timeout was infinity, but you
changed it to your constant. Is it to make
all the places consistent? That is strange,
because there are still other places calling
writable() without arguments or a settimeout()
invoked beforehand.