<HTML><BODY><span style="font-family: Arial, Tahoma, Verdana, sans-serif;" data-mce-style="font-family: Arial, Tahoma, Verdana, sans-serif;">Hi!</span><br style="font-family: Arial, Tahoma, Verdana, sans-serif;" data-mce-style="font-family: Arial, Tahoma, Verdana, sans-serif;"><br style="font-family: Arial, Tahoma, Verdana, sans-serif;" data-mce-style="font-family: Arial, Tahoma, Verdana, sans-serif;"><span style="font-family: Arial, Tahoma, Verdana, sans-serif;" data-mce-style="font-family: Arial, Tahoma, Verdana, sans-serif;">Thanks for your review!<br><br></span><p style="font-family: Arial, Tahoma, Verdana, sans-serif;" data-mce-style="font-family: Arial, Tahoma, Verdana, sans-serif;">Sent v5 including all proposed enhancements.</p><br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">
Среда, 25 декабря 2019, 4:29 +03:00 от Alexander Turenko <alexander.turenko@tarantool.org>:<br>
<br>
<div id="">
<div class="js-helper js-readmsg-msg">
<style type="text/css"></style>
<div>
<div id="style_15772373771611988711_BODY">Thanks for the patch!<br>
<br>
I really appreciate the work in direction toward stable tests. Unstable<br>
CI sometimes hides real problems, so actually it is direction to make<br>
tarantool itself more stable.<br>
<br>
All changes look correct. There are places however that we can polish a<br>
bit. Please, consider my comments below.<br>
<br>
How I tested it:<br>
<br>
$ (cd test && ./test-run.py -j 30 --force $(yes app/socket | head -n 1000))<br>
<br>
Observed the following fails:<br>
<br>
Case 1.<br>
<br>
[008] Test failed! Result content mismatch:<br>
[008] --- app/socket.result Tue Dec 24 15:55:40 2019<br>
[008] +++ app/socket.reject Tue Dec 24 15:56:24 2019<br>
[008] @@ -1371,7 +1371,7 @@<br>
[008] ...<br>
[008] cnt<br>
[008] ---<br>
[008] -- 2<br>
[008] +- 3<br>
[008] ...<br>
[008] client:write('hi')<br>
[008] ---<br>
<br>
Another instance is connected to our server. I found that it is<br>
socket.connect() from 'case: socket receive inconsistent behavior' (and<br>
this is the reason of case 3 below). The code the the following:<br>
<br>
| srv = socket.tcp_server('0.0.0.0', 0, fn)<br>
| s = socket.connect('localhost', srv:name().port)<br>
<br>
localhost may be resolved into ::1 (ipv6) and 127.0.0.1 (ipv4). Linux<br>
allows to bind the same port number on different addresses.<br>
getaddrinfo() on Linux give ipv6 addr/port first (usually, depends<br>
on settings) and if another process listens on ::1 on the same port as<br>
we bound on ipv4, we'll connect to this process.<br>
<br>
The change that eliminates the fail above for me:<br>
<br>
@@ -979,7 +983,7 @@ chan = fiber.channel()<br>
counter = 0<br>
fn = function(s) counter = 0; while true do s:write((tostring(counter)):rep(chan:get())); counter = counter + 1 end end<br>
srv = socket.tcp_server('0.0.0.0', 0, fn)<br>
-s = socket.connect('localhost', srv:name().port)<br>
+s = socket.connect('127.0.0.1', srv:name().port)<br>
chan:put(5)<br>
chan:put(5)<br>
s:receive(5)<br>
<br>
(CCed Maria as author of the test case.)<br>
<br>
However it is possible that there are other similar cases. I suggest to<br>
verify the test against them. Please, beware of changing negative cases<br>
(when we expect that a timeout will occur): setting large timeout on<br>
them would significantly increase the test run time (we'll always wait<br>
100 seconds at the point).<br>
<br>
BTW, why not to listen on 127.0.0.1 rather then on all interfaces<br>
(0.0.0.0)?<br>
<br>
BTW, unix sockets may help to don't bother with clashes of different<br>
kinds.<br>
<br>
----<br>
<br>
[018] --- app/socket.result Tue Dec 24 15:58:20 2019<br>
[018] +++ app/socket.reject Tue Dec 24 15:59:29 2019<br>
[018] @@ -2822,7 +2822,7 @@<br>
[018] ...<br>
[018] client:read(1, 5) == ''<br>
[018] ---<br>
[018] -- true<br>
[018] +- false<br>
[018] ...<br>
[018] server:close()<br>
[018] ---<br>
<br>
I printed client_.errno. It is 110 (ETIMEDOUT). It seems it worth to<br>
replace 5 with WAIT_COND_TIME (or, as TIMEOUT as I proposed to name it<br>
below). Maybe there are other such places.<br>
<br>
----<br>
<br>
Case 3.<br>
<br>
Test hung! Result content mismatch:<br>
--- app/socket.result Wed Dec 25 01:04:26 2019<br>
+++ var/003_app/socket.result Wed Dec 25 01:05:22 2019<br>
@@ -2857,34 +2857,3 @@<br>
---<br>
...<br>
chan:put(5)<br>
----<br>
-- true<br>
-...<br>
-chan:put(5)<br>
----<br>
-- true<br>
-...<br>
-s:receive(5)<br>
----<br>
-- '00000'<br>
-...<br>
-chan:put(5)<br>
----<br>
-- true<br>
-...<br>
-s:settimeout(1)<br>
----<br>
-- 1<br>
-...<br>
-s:receive('*a')<br>
----<br>
-- '1111122222'<br>
-...<br>
-s:close()<br>
----<br>
-- 1<br>
-...<br>
-srv:close()<br>
----<br>
-- true<br>
-...<br>
<br>
Explained above (see case 1).<br>
<br>
----<br>
<br>
[017] --- app/socket.result Wed Dec 25 01:18:29 2019<br>
[017] +++ app/socket.reject Wed Dec 25 01:19:30 2019<br>
[017] @@ -40,7 +40,7 @@<br>
[017] ...<br>
[017] test_run:cmd("push filter '(error: .builtin/.*[.]lua):[0-9]+' to '\\1'")<br>
[017] ---<br>
[017] -- true<br>
[017] +- Hello, world<br>
<br>
It seems we connected to some of our socket server rather then test-run.<br>
Looks like the same ipv4 / ipv6 problem.<br>
<br>
After the following patch for test-run I don't see it anymore (don't<br>
sure I made enough runs, however):<br>
<br>
diff --git a/lib/test_suite.py b/lib/test_suite.py<br>
index 84b99f4..0a137c3 100644<br>
--- a/lib/test_suite.py<br>
+++ b/lib/test_suite.py<br>
@@ -197,7 +197,7 @@ class TestSuite:<br>
def start_server(self, server):<br>
# create inspector daemon for cluster tests<br>
inspector = TarantoolInspector(<br>
- 'localhost', server.inspector_port<br>
+ '127.0.0.1', server.inspector_port<br>
)<br>
inspector.start()<br>
# fixme: remove this string if we fix all legacy tests<br>
diff --git a/test_run.lua b/test_run.lua<br>
index e37b559..fc1b833 100644<br>
--- a/test_run.lua<br>
+++ b/test_run.lua<br>
@@ -458,7 +458,7 @@ local inspector_methods = {<br>
local function inspector_new(host, port)<br>
local inspector = {}<br>
<br>
- inspector.host = host or 'localhost'<br>
+ inspector.host = host or '127.0.0.1'<br>
inspector.port = port or tonumber(os.getenv('INSPECTOR'))<br>
if inspector.port == nil then<br>
error('Inspector not started')<br>
<br>
----<br>
<br>
On Mon, Dec 23, 2019 at 11:48:33PM +0300, Ilya Kosarev wrote:<br>
> socket.test had a number of flaky problems:<br>
> - socket readiness expectation<br>
> - race conditions on socket shutdown in emulation test cases<br>
> - UDP datagrams losses on mac os<br>
> - excessive random port searches<br>
> Now they are solved. Socket test is not fragile anymore.<br>
> <br>
> Closes #4451, #4426, #4469<br>
> ---<br>
> Branch: <a href="https://github.com/tarantool/tarantool/tree/i.kosarev/gh-4426-4451-fix-socket-test" target="_blank">https://github.com/tarantool/tarantool/tree/i.kosarev/gh-4426-4451-fix-socket-test</a><br>
> Issues: <a href="https://github.com/tarantool/tarantool/issues/4426" target="_blank">https://github.com/tarantool/tarantool/issues/4426</a><br>
> <a href="https://github.com/tarantool/tarantool/issues/4451" target="_blank">https://github.com/tarantool/tarantool/issues/4451</a><br>
> <a href="https://github.com/tarantool/tarantool/issues/4469" target="_blank">https://github.com/tarantool/tarantool/issues/4469</a><br>
> <br>
> Changes in v2:<br>
> - reconsidered socket readiness expectation<br>
> - reduced conditions waiting time<br>
> <br>
> Changes in v3:<br>
> - reconsidered expectations to unify them<br>
> - simplified randomization<br>
> <br>
> Changes in v4:<br>
> - left infinite timeouts alone<br>
> - wrapped UDP datagrams awaiting with wait_cond<br>
<br>
That's my bad. As I see, other parts of the test follows the following<br>
pattern for waiting UDP packets:<br>
<br>
| s:readable(TIMEOUT)<br>
| s:recv()<br>
<br>
test_run:wait_cond() is correct solution too, but :readable() looks a<br>
bit simpler. Anyway, let's use one that you like more.<br>
<br>
(It is the only change I propose, which I didn't verify.)<br>
<br>
> - replaced manual port randomization with zero bind<br>
<br>
Nice idea!<br>
<br>
> - removed extra wait_cond wrappers<br>
> <br>
> test/app/socket.result | 104 ++++++++++++++++++++++-----------------<br>
> test/app/socket.test.lua | 94 ++++++++++++++++++++---------------<br>
> test/app/suite.ini | 1 -<br>
> 3 files changed, 114 insertions(+), 85 deletions(-)<br>
<br>
> diff --git a/test/app/socket.test.lua b/test/app/socket.test.lua<br>
> index c72d41763f4..4ed6c0f25f7 100644<br>
> --- a/test/app/socket.test.lua<br>
> +++ b/test/app/socket.test.lua<br>
> @@ -13,7 +13,7 @@ env = require('test_run')<br>
> test_run = env.new()<br>
> test_run:cmd("push filter '(error: .builtin/.*[.]lua):[0-9]+' to '\\1'")<br>
> <br>
> -WAIT_COND_TIME = 10<br>
> +WAIT_COND_TIME = 100<br>
<br>
Nit: I would name it just TIMEOUT, since it is not only for<br>
test_run:wait_cond() now.<br>
<br>
> @@ -619,8 +608,14 @@ s:settimeout(100500)<br>
> rch, wch = fiber.channel(1), fiber.channel(1)<br>
> sc = socket.connect(host, port)<br>
> test_run:cmd("setopt delimiter ';'")<br>
> +socket_opened = true<br>
> cfiber = fiber.create(function(sc, rch, wch)<br>
> - while sc:send(wch:get()) and rch:put(sc:receive("*l")) do end<br>
> + while socket_opened do<br>
> + sc:send(wch:get())<br>
> + local data = sc:receive("*l")<br>
> + if not socket_opened then sc:close() end<br>
> + rch:put(data)<br>
> + end<br>
<br>
I would use existing channel to signal the fiber to close the socket: it<br>
looks simpler then using a channel and a variable both. Proposed diff:<br>
<br>
diff --git a/test/app/socket.test.lua b/test/app/socket.test.lua<br>
index 4ed6c0f25..0fe22709a 100644<br>
--- a/test/app/socket.test.lua<br>
+++ b/test/app/socket.test.lua<br>
@@ -608,13 +608,16 @@ s:settimeout(100500)<br>
rch, wch = fiber.channel(1), fiber.channel(1)<br>
sc = socket.connect(host, port)<br>
test_run:cmd("setopt delimiter ';'")<br>
-socket_opened = true<br>
cfiber = fiber.create(function(sc, rch, wch)<br>
- while socket_opened do<br>
- sc:send(wch:get())<br>
- local data = sc:receive("*l")<br>
- if not socket_opened then sc:close() end<br>
- rch:put(data)<br>
+ while true do<br>
+ local outgoing_data = wch:get()<br>
+ if outgoing_data == nil then<br>
+ sc:close()<br>
+ break<br>
+ end<br>
+ sc:send(outgoing_data)<br>
+ local incoming_data = sc:receive("*l")<br>
+ rch:put(incoming_data)<br>
end<br>
end, sc, rch, wch);<br>
test_run:cmd("setopt delimiter ''");<br>
@@ -646,9 +649,10 @@ rch:get()<br>
wch:put("DATA\n")<br>
c:receive(4)<br>
c:receive("*l")<br>
-socket_opened = false<br>
+rch:get()<br>
wch:put("Fu")<br>
c:send("354 Please type your message\n")<br>
+wch:put(nil) -- EOF<br>
c:receive("*l", "Line: ")<br>
c:receive()<br>
c:receive(10)<br>
<br>
> end, sc, rch, wch);<br>
> test_run:cmd("setopt delimiter ''");<br>
> <br>
> @@ -651,9 +646,9 @@ rch:get()<br>
> wch:put("DATA\n")<br>
> c:receive(4)<br>
> c:receive("*l")<br>
> +socket_opened = false<br>
> wch:put("Fu")<br>
> c:send("354 Please type your message\n")<br>
> -sc:close()<br>
> c:receive("*l", "Line: ")<br>
> c:receive()<br>
> c:receive(10)<br>
<br>
> @@ -778,7 +773,9 @@ e == errno.EAGAIN -- expected true<br>
> <br>
> -- case: recv, zero datagram<br>
> sendto_zero(sending_socket, '127.0.0.1', receiving_socket_port)<br>
> -received_message = receiving_socket:recv()<br>
> +received_message = test_run:wait_cond(function() \<br>
> + return receiving_socket:recv() \<br>
> +end, WAIT_COND_TIME)<br>
<br>
Proposed to use :readable() (see above).<br>
<br>
> --- case: sicket receive inconsistent behavior<br>
> +-- case: socket receive inconsistent behavior<br>
> chan = fiber.channel()<br>
> counter = 0<br>
> fn = function(s) counter = 0; while true do s:write((tostring(counter)):rep(chan:get())); counter = counter + 1 end end<br>
> -srv = socket.tcp_server('0.0.0.0', 8888, fn)<br>
> -s = socket.connect('localhost', 8888)<br>
> +srv = socket.tcp_server('0.0.0.0', 0, fn)<br>
> +s = socket.connect('localhost', srv:name().port)<br>
<br>
Proposed to connect to 127.0.0.1 (see above).<br>
<br>
> chan:put(5)<br>
> chan:put(5)<br>
> s:receive(5)<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
<br>-- <br>Ilya Kosarev<br></BODY></HTML>