<HTML><BODY>Hi!<br><br>Thanks for the comments!<br><br>5 answers are below.<br><br><span style="font-family: Arial, Tahoma, Verdana, sans-serif;" data-mce-style="font-family: Arial, Tahoma, Verdana, sans-serif;">Sent v3 considering discussed questions.</span><br><br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">
Воскресенье, 8 декабря 2019, 18:52 +03:00 от Vladislav Shpilevoy <v.shpilevoy@tarantool.org>:<br><br><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15758203512012208388_BODY">Hi! Thanks for the patch!<br><br>
Please, keep my comments near your answers. Because<br>
I don't remember all my comments, and for each your<br>
answer I need to lookup my original email to remember,<br>
what was the problem.<br><br>
This is usually simpler both to you and a reviewer,<br>
when your answers are inlined into the original email.<br><br>
Also don't drop the comments. Here is my comment 2:<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>
And it is still actual. Why is it called 'CONNECT_TIME' if<br>
it has nothing to do with connect in most of the places?<br>
The problem is in the name. I would introduce one timeout<br>
for conds, connects, readable, etc. You even already have<br>
it - COND_TIME.</div></div></div></div></blockquote>2. Ok, I see the problem here. I will carefully reconsider such<br>places to make them consistent.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15758203512012208388_BODY"><br><br>
On 06/12/2019 17:01, Ilya Kosarev wrote:<br>
> Hi!<br>
> <br>
> Thanks for your review.<br>
> <br>
> 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><br>
I tried this example on your branch and got that:<br><br>
=========================================================================<br>
[009] --- app/socket.result Sun Dec 8 15:40:58 2019<br>
[009] +++ app/socket.reject Sun Dec 8 16:09:23 2019<br>
[009] @@ -2670,13 +2670,17 @@<br>
[009] -- case: recvfrom reads first 512 bytes from the message with tcp<br>
[009] received_message, from = receiving_socket:recvfrom()<br>
[009] ---<br>
[009] +- error: '[string "received_message, from = receiving_socket:rec..."]:1: attempt to<br>
[009] + index global ''receiving_socket'' (a nil value)'<br>
[009] ...<br>
[009] e = receiving_socket:errno()<br>
[009] ---<br>
[009] +- error: '[string "e = receiving_socket:errno() "]:1: attempt to index global ''receiving_socket''<br>
[009] + (a nil value)'<br>
[009] ...<br>
[009] received_message == message:sub(1, 512) -- expected true<br>
[009] ---<br>
[009] -- true<br>
[009] +- false<br>
[009] ...<br>
[009] received_message:len() == 512 -- expected true<br>
[009] ---<br>
[009] @@ -2684,7 +2688,7 @@<br>
[009] ...<br>
[009] received_message<br>
[009] ---<br>
[009] -- xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx<br>
[009] +- yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy<br>
[009] ...<br>
[009] received_message:len()<br>
[009] ---<br>
[009] @@ -2692,11 +2696,13 @@<br>
[009] ...<br>
[009] from == nil -- expected true<br>
[009] ---<br>
[009] -- true<br>
[009] +- false<br>
[009] ...<br>
[009] from<br>
[009] ---<br>
[009] -- null<br>
[009] +- host: 127.0.0.1<br>
[009] + family: AF_INET<br>
[009] + port: 57721<br>
[009] ...<br>
[009] e == 0 -- expected true<br>
[009] ---<br>
[009] @@ -2709,33 +2715,39 @@<br>
[009] -- case: recvfrom does not discard the message tail with tcp<br>
[009] received_message, from = receiving_socket:recvfrom()<br>
[009] ---<br>
[009] +- error: '[string "received_message, from = receiving_socket:rec..."]:1: attempt to<br>
[009] + index global ''receiving_socket'' (a nil value)'<br>
[009] ...<br>
[009] e = receiving_socket:errno()<br>
[009] ---<br>
[009] +- error: '[string "e = receiving_socket:errno() "]:1: attempt to index global ''receiving_socket''<br>
[009] + (a nil value)'<br>
[009] ...<br>
[009] received_message == message:sub(513, 513) -- expected true<br>
[009] ---<br>
[009] -- true<br>
[009] +- false<br>
[009] ...<br>
[009] received_message:len() == 1 -- expected true<br>
[009] ---<br>
[009] -- true<br>
[009] +- false<br>
[009] ...<br>
[009] received_message<br>
[009] ---<br>
[009] -- x<br>
[009] +- yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy<br>
[009] ...<br>
[009] received_message:len()<br>
[009] ---<br>
[009] -- 1<br>
[009] +- 512<br>
[009] ...<br>
[009] from == nil -- expected true<br>
[009] ---<br>
[009] -- true<br>
[009] +- false<br>
[009] ...<br>
[009] from<br>
[009] ---<br>
[009] -- null<br>
[009] +- host: 127.0.0.1<br>
[009] + family: AF_INET<br>
[009] + port: 57721<br>
[009] ...<br>
[009] e == 0 -- expected true<br>
[009] ---<br>
[009] @@ -2747,7 +2759,8 @@<br>
[009] ...<br>
[009] receiving_socket:close()<br>
[009] ---<br>
[009] -- true<br>
[009] +- error: '[string "return receiving_socket:close() "]:1: attempt to index global ''receiving_socket''<br>
[009] + (a nil value)'<br>
[009] ...<br>
[009] sending_socket:close()<br>
[009] ---<br>
[009] <br>
=========================================================================<br><br>
Sometimes I get this:<br><br>
=========================================================================<br>
[008] app/socket.test.lua [ fail ]<br>
[008] <br>
[008] Test failed! Result content mismatch:<br>
[008] --- app/socket.result Sun Dec 8 15:40:58 2019<br>
[008] +++ app/socket.reject Sun Dec 8 16:11:27 2019<br>
[008] @@ -2340,23 +2340,24 @@<br>
[008] ...<br>
[008] received_message == '' -- expected true<br>
[008] ---<br>
[008] -- true<br>
[008] +- false<br>
[008] ...<br>
[008] received_message<br>
[008] ---<br>
[008] -- <br>
[008] +- null<br>
[008] ...<br>
[008] from ~= nil -- expected true<br>
[008] ---<br>
[008] -- true<br>
[008] +- false<br>
[008] ...<br>
[008] from.host == '127.0.0.1' -- expected true<br>
[008] ---<br>
[008] -- true<br>
[008] +- error: '[string "return from.host == ''127.0.0.1'' -- expected t..."]:1: attempt<br>
[008] + to index global ''from'' (a nil value)'<br>
[008] ...<br>
[008] e == 0 -- expected true<br>
[008] ---<br>
[008] -- true<br>
[008] +- false<br>
[008] ...<br>
[008] -- case: recv, no datagram, explicit size<br>
[008] received_message = receiving_socket:recv(512)<br>
[008] @@ -2386,23 +2387,25 @@<br>
[008] ...<br>
[008] received_message == nil -- expected true<br>
[008] ---<br>
[008] -- true<br>
[008] +- false<br>
[008] ...<br>
[008] received_message<br>
[008] ---<br>
[008] -- null<br>
[008] +- <br>
[008] ...<br>
[008] from == nil -- expected true<br>
[008] ---<br>
[008] -- true<br>
[008] +- false<br>
[008] ...<br>
[008] from<br>
[008] ---<br>
[008] -- null<br>
[008] +- host: 127.0.0.1<br>
[008] + family: AF_INET<br>
[008] + port: 59867<br>
[008] ...<br>
[008] e == errno.EAGAIN -- expected true<br>
[008] ---<br>
[008] -- true<br>
[008] +- false<br>
[008] ...<br>
[008] -- case: recv, zero datagram, explicit size<br>
[008] sendto_zero(sending_socket, '127.0.0.1', receiving_socket_port)<br>
=========================================================================<br><br>
So socket.test.lua still is unstable. The command with 64<br>
socket.test.lua gives me an error in 100% cases.</div></div></div></div></blockquote>4. Ok, I guess these are mac os specific issues. I haven't got mac os<br>machine access yet, however, I hope to get it soon and fix these problems.<br>I still tried to stabilize these cases; if it don't work, I will reconsider them later.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15758203512012208388_BODY"><br><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.<br><br>
I thought that it is strange, because UUIDs are also generated<br>
randomly, and by the same random generator as is used in Lua.<br>
But now I see why math.random() gives you the same numbers<br>
even though UUIDs are different: Lua math.random() has own<br>
random number generator. Not from the standard library.<br><br>
> 5. Well, considering the new seed generation, 10s seem to be enough.<br><br>
So you could skip this strange extraction of a number from<br>
UUID, but increase the timeout? I vote for it. Because the<br>
failing example with 64 instances of socket.test.lua is very<br>
artificial, and even then it will work in case the timeout<br>
is big enough, right? UUID parsing adds unnecessary complexity<br>
and raises questions like I asked.</div></div></div></div></blockquote>5. Ok, I agree.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15758203512012208388_BODY"><br><br>
> 6. If we close socket in the main fiber, we might attempt to use closed<br>
> socket in background fiber.<br><br>
And what? We don't care anymore for the background<br>
fiber after the socket is not needed anymore. Ok, it will<br>
stumble into errors in that fiber, but how does it affect<br>
further execution of the test?</div></div></div></div></blockquote>6. It affects further test execution, as far as it leads<br>to the following error:<br><pre style="box-sizing: border-box; font-family: SFMono-Regular, Consolas, 'Liberation Mono', Menlo, monospace; font-size: 11.9px; margin-top: 0px; margin-bottom: 16px; overflow-wrap: normal; padding: 16px; overflow: auto; line-height: 1.45; background-color: #f6f8fa; border-radius: 3px; color: #24292e;" data-mce-style="box-sizing: border-box; font-family: SFMono-Regular, Consolas, 'Liberation Mono', Menlo, monospace; font-size: 11.9px; margin-top: 0px; margin-bottom: 16px; overflow-wrap: normal; padding: 16px; overflow: auto; line-height: 1.45; background-color: #f6f8fa; border-radius: 3px; color: #24292e;"><code style="box-sizing: border-box; font-family: SFMono-Regular, Consolas, 'Liberation Mono', Menlo, monospace; font-size: 11.9px; padding: 0px; margin: 0px; background: initial; border-radius: 3px; word-break: normal; border: 0px; display: inline; overflow: visible; line-height: inherit; overflow-wrap: normal;" data-mce-style="box-sizing: border-box; font-family: SFMono-Regular, Consolas, 'Liberation Mono', Menlo, monospace; font-size: 11.9px; padding: 0px; margin: 0px; background: initial; border-radius: 3px; word-break: normal; border: 0px; display: inline; overflow: visible; line-height: inherit; overflow-wrap: normal;">[015] c:receive("*l", "Line: ")
[015] ---
[015] - null
[015] -- closed
[015] -- 'Line: Fu'
[015] +- Connection reset by peer</code></pre><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15758203512012208388_BODY"><br><br>
> <br>
> 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.<br><br>
If it will always be nil in case of an error, we won't notice the<br>
problem. Please, drop it. Increase a timeout somewhere if necessary,<br>
but consideration of a test passed, if in fact it didn't, is worse.</div></div></div></div></blockquote>7. We don't consider the test passed because of this construction.<br> We will notice the problem, as far as <span style="font-family: "DejaVu Sans Mono"; font-size: 9pt;">test_run:wait_cond()<br></span>will return false instead of true, and also <span style="font-family: "DejaVu Sans Mono"; font-size: 9pt;" data-mce-style="font-family: 'DejaVu Sans Mono'; font-size: 9pt;"><span style="font-family: "DejaVu Sans Mono"; font-size: 9pt;" data-mce-style="font-family: 'DejaVu Sans Mono'; font-size: 9pt;">receive1 & <span style="font-size: 9pt;">receive2<br></span></span></span>values will be nil instead of <span style="font-family: "DejaVu Sans Mono"; font-size: 9pt;" data-mce-style="font-family: 'DejaVu Sans Mono'; font-size: 9pt;"><span style="font-family: "DejaVu Sans Mono"; font-size: 9pt;" data-mce-style="font-family: 'DejaVu Sans Mono'; font-size: 9pt;">'00000' & <span style="font-size: 9pt;">'1111122222'.<br></span></span></span>We just don't want to hang on chan:put and wait for extra 120 sec.<br>for test to fail, that is why it is hidden under if clause.<br><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_15758203512012208388_BODY"><br></div></div></div></div></blockquote>
<br>
<br>-- <br>Ilya Kosarev<br></BODY></HTML>