<HTML><BODY><div class="js-helper js-readmsg-msg">
<style type="text/css"></style>
<div>
<base target="_self" href="https://e.mail.ru/">
<div id="style_15756431540843867601_BODY"><div class="class_1575677490">
<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></span><br><p>4. I run socket.test to simulate "high load" in parallel with itself<br>(while ./test-run.py -j20 `for r in {1..64} ; do echo socket ; done` 2>/dev/null ; do l=$(($l+1)) ; echo ======== $l ============= ; done).<br>Using fiber.time() as random seed leads to the same results produced<br>on parallel tasks. Using instance uuid as a seed seems to be a good<br>decision.</p><p>5. Well, considering the new seed generation, 10s seem to be enough.</p><p>6. If we close socket in the main fiber, we might attempt to use closed<br>socket in background fiber.</p><p>7. Of course it shouldn't be nil and test will fail in this case. We<br>just want to be sure that test won't hang even if server is still nil.<br>Otherwise we will hang on chan:put(), which is not nice.</p>Sent v2 considering mentioned drawbacks.<br><br><blockquote style="border-left:1px solid #0857A6;margin:10px;padding:0 0 0 10px;">
Пятница, 6 декабря 2019, 0:31 +03:00 от Vladislav Shpilevoy <<a href="mailto:v.shpilevoy@tarantool.org">v.shpilevoy@tarantool.org</a>>:<br>
<br>
<div id="">
<div class="js-helper_mailru_css_attribute_postfix js-readmsg-msg_mailru_css_attribute_postfix">
<style></style>
<div>
<div id="style_15755814790108045663_BODY_mailru_css_attribute_postfix">Hi! Thanks for the patch!<br>
<br>
See 7 comments below.<br>
<br>
On 04/12/2019 14:43, Ilya Kosarev wrote:<br>
> socket.test had a number of flaky problems:<br>
> - socket readableness expectation<br>
> - race conditions on socket shutdown in emulation test cases<br>
> - tcp_server stability in socket receive inconsistent behavior case<br>
> Now they are solved. Port randomization is improved.<br>
> 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" rel=" noopener noreferrer">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" rel=" noopener noreferrer">https://github.com/tarantool/tarantool/issues/4426</a><br>
> <a href="https://github.com/tarantool/tarantool/issues/4451" target="_blank" rel=" noopener noreferrer">https://github.com/tarantool/tarantool/issues/4451</a><br>
> <a href="https://github.com/tarantool/tarantool/issues/4469" target="_blank" rel=" noopener noreferrer">https://github.com/tarantool/tarantool/issues/4469</a><br>
> <br>
> test/app/socket.result | 84 +++++++++++++++++++++++++---------------<br>
> test/app/socket.test.lua | 60 +++++++++++++++++++---------<br>
> test/app/suite.ini | 1 -<br>
> 3 files changed, 94 insertions(+), 51 deletions(-)<br>
> <br>
> diff --git a/test/app/socket.result b/test/app/socket.result<br>
> index fd299424c9..42dde8f375 100644<br>
> --- a/test/app/socket.result<br>
> +++ b/test/app/socket.result<br>
> @@ -45,6 +45,9 @@ test_run:cmd("push filter '(error: .builtin/.*[.]lua):[0-9]+' to '\\1'")<br>
> WAIT_COND_TIME = 10<br>
1. This is now unused.<br>
<br>
> ---<br>
> ...<br>
> +WAIT_TCP_CONNECT_TIME = 240<br>
> +---<br>
> +...<br>
> socket('PF_INET', 'SOCK_STREAM', 'tcp121222');<br>
> ---<br>
> - null<br>
> @@ -107,7 +110,7 @@ s:nonblock(true)<br>
> ---<br>
> - true<br>
> ...<br>
> -s:readable(.1)<br>
> +s:readable(WAIT_TCP_CONNECT_TIME)<br>
> ---<br>
> - true<br>
> ...<br>
> @@ -227,7 +230,7 @@ s:syswrite(ffi.cast('const char *', ping), #ping)<br>
> ---<br>
> - 6<br>
> ...<br>
> -s:readable(1)<br>
> +s:readable(WAIT_TCP_CONNECT_TIME)<br>
<br>
2. I understand, why it is 'connect time' in the previous<br>
hunk. But here the socket is already connected. So it is<br>
not connect time.<br>
<br>
On the other hand, there are places right after connect,<br>
which don't use this timeout. For example, line 457<br>
in socket.result file:<br>
<br>
sc:sysconnect('127.0.0.1', s:name().port) or errno() == errno.EINPROGRESS<br>
---<br>
- true<br>
...<br>
sc:writable(10)<br>
---<br>
- true<br>
...<br>
<br>
Here it is 10 seconds, while in other places you started<br>
using 240.<br>
<br>
> ---<br>
> - true<br>
> ...<br>
> @@ -830,7 +833,7 @@ sc:sendto('127.0.0.1', s:name().port, 'Hello, world')<br>
> ---<br>
> - 12<br>
> ...<br>
> -s:readable(10)<br>
> +s:readable(WAIT_TCP_CONNECT_TIME)<br>
> ---<br>
> - true<br>
> ...<br>
<br>
3. There is no connect time. This is DGRAM socket with UDP.<br>
No connections.<br>
<br>
The same below. Moreover, there are now 2 waits for<br>
'connect time' on the same socket.<br>
<br>
> @@ -842,7 +845,7 @@ sc:sendto('127.0.0.1', s:name().port, 'Hello, world, 2')<br>
> ---<br>
> - 15<br>
> ...<br>
> -s:readable(10)<br>
> +s:readable(WAIT_TCP_CONNECT_TIME)<br>
> ---<br>
> - true<br>
> ...<br>
> @@ -898,7 +901,7 @@ sc:sendto('127.0.0.1', s:name().port, 'Hello, World!')<br>
> ---<br>
> - 13<br>
> ...<br>
> -s:readable(1)<br>
> +s:readable(WAIT_TCP_CONNECT_TIME)<br>
> ---<br>
> - true<br>
> ...<br>
> @@ -1074,6 +1077,15 @@ master:setsockopt('SOL_SOCKET', 'SO_REUSEADDR', true)<br>
> ---<br>
> - true<br>
> ...<br>
> +seed = ''<br>
> +---<br>
> +...<br>
> +for d in string.gmatch(box.info.uuid, '%d') do seed = seed .. d end<br>
<br>
4. Well, this is probably the most intricate way to<br>
get a seed. Have you considered fiber.time()? And why do<br>
you need it? Isn't it initialized already?<br>
<br>
> +---<br>
> +...<br>
> +math.randomseed(tonumber(seed))<br>
> +---<br>
> +...<br>
> port = 32768 + math.random(0, 32767)<br>
> ---<br>
> ...<br>
> @@ -1092,7 +1104,7 @@ test_run:wait_cond(function()<br>
> return false, master:error()<br>
> end<br>
> return true<br>
> -end, WAIT_COND_TIME);<br>
> +end, 100);<br>
<br>
5. Why? 10 seconds was not enough to try a few random<br>
ports?<br>
<br>
> ---<br>
> - true<br>
> ...<br>
> @@ -1822,8 +1834,14 @@ test_run:cmd("setopt delimiter ';'")<br>
> ---<br>
> - true<br>
> ...<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>
> end, sc, rch, wch);<br>
<br>
6. What was a problem here? In the commit message you said<br>
'race conditions'. Where? You just moved socket close to<br>
the background fiber from the main fiber.<br>
<br>
> ---<br>
> ...<br>
> @@ -1936,6 +1954,9 @@ c:receive("*l")<br>
> ---<br>
> - <br>
> ...<br>
> +socket_opened = false<br>
> +---<br>
> +...<br>
> wch:put("Fu")<br>
> ---<br>
> - true<br>
> @@ -1944,10 +1965,6 @@ c:send("354 Please type your message\n")<br>
> ---<br>
> - 29<br>
> ...<br>
> -sc:close()<br>
> ----<br>
> -- 1<br>
> -...<br>
> c:receive("*l", "Line: ")<br>
> ---<br>
> - null<br>
> @@ -2816,7 +2833,7 @@ test_run:cmd("clear filter")<br>
> ---<br>
> - true<br>
> ...<br>
> --- case: sicket receive inconsistent behavior<br>
> +-- case: socket receive inconsistent behavior<br>
> chan = fiber.channel()<br>
> ---<br>
> ...<br>
> @@ -2826,41 +2843,46 @@ counter = 0<br>
> fn = function(s) counter = 0; while true do s:write((tostring(counter)):rep(chan:get())); counter = counter + 1 end end<br>
> ---<br>
> ...<br>
> -srv = socket.tcp_server('0.0.0.0', 8888, fn)<br>
> +srv = nil<br>
> ---<br>
> ...<br>
> -s = socket.connect('localhost', 8888)<br>
> +test_run:cmd("setopt delimiter ';'")<br>
> ---<br>
> +- true<br>
> ...<br>
> -chan:put(5)<br>
> +test_run:wait_cond(function()<br>
> + port = 32768 + math.random(0, 32767)<br>
> + srv = socket.tcp_server('0.0.0.0', port, fn)<br>
> + return srv ~= nil<br>
> +end, 100);<br>
> ---<br>
> - true<br>
> ...<br>
> -chan:put(5)<br>
> +receive1 = nil; receive2 = nil;<br>
> ---<br>
> -- true<br>
> ...<br>
> -s:receive(5)<br>
> +if srv ~= nil then<br>
> + s = socket.connect('localhost', port)<br>
> + chan:put(5)<br>
> + chan:put(5)<br>
> + receive1 = s:receive(5)<br>
> + chan:put(5)<br>
> + s:settimeout(1)<br>
> + receive2 = s:receive('*a')<br>
> + s:close()<br>
> + srv:close()<br>
> +end;<br>
<br>
7. How is it possible, that the server still is nil here?<br>
At least one of 32k ports should have been free.<br>
<br>
> ---<br>
> -- '00000'<br>
> ...<br>
> -chan:put(5)<br>
> +test_run:cmd("setopt delimiter ''");<br>
> ---<br>
> - true<br>
> ...<br>
> -s:settimeout(1)<br>
> +receive1<br>
> ---<br>
> -- 1<br>
> +- '00000'<br>
> ...<br>
> -s:receive('*a')<br>
> +receive2<br>
> ---<br>
> - '1111122222'<br>
> ...<br>
> -s:close()<br>
> ----<br>
> -- 1<br>
> -...<br>
> -srv:close()<br>
> ----<br>
> -- true<br>
> -...<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
<br>-- <br>Ilya Kosarev<br>
</div></div>
<base target="_self" href="https://e.mail.ru/">
</div>
</div></BODY></HTML>