<HTML><BODY><div><div>Hi!</div><div> </div><div>Thanks for the answers.</div><div> </div><div>No new comments. LGTM<br> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Среда, 6 мая 2020, 11:01 +03:00 от lvasiliev <lvasiliev@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><style type="text/css"></style><div><div id="style_15887520751307141186_BODY">Hi! Thanks for the review.<br><br>On 29.04.2020 15:12, Ilya Kosarev wrote:<br>><br>> Hi!<br>><br>> Thanks for the patch.<br>><br>> LGTM, see 2 nits below.<br>><br>>> The tcp server starts in a separate fiber and when<br>>> the socket closes from another fiber the server's<br>>> fiber fails with an attempt to validate the server<br>>> socket. For check the socket status at server's<br>>> fiber a flag has been added and now server's fiber<br>>> will terminate correctly if the socket was closed.<br>> 1. I think this way it might sound a bit better:<br>> The tcp server starts in a separate fiber and when<br>> it’s socket is being closed from another fiber,<br>> server's fiber used to fail with an attempt to validate<br>> the server socket. To check socket status at server's<br>> fiber a flag has been added and now server's fiber<br>> terminates correctly when the socket is being closed.<br><br>Updated description:<br><br>The tcp server starts in a separate fiber.<br>When server's socket is closed from another fiber,<br>an exception will be thrown in server's loop from<br>check_socket function.<br>To check socket status at server's fiber a flag has<br>been added and now server's fiber terminates correctly<br>when the socket is being closed.</div></div></div></div></blockquote></div><div>Right, I think the last one is the most clear.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>>><br>>> Fixes: #4087<br>>> ---<br>>> @Changelog fix error while closing socket.tcp_server socket(gh-4087)<br>>><br>>> src/lua/socket.lua | 9 +++++++++<br>>> test/app/socket.result | 14 ++++++++++++++<br>>> test/app/socket.test.lua | 7 ++++++-<br>>> 3 files changed, 29 insertions(+), 1 deletion(-)<br>>><br>>> diff --git a/src/lua/socket.lua b/src/lua/socket.lua<br>>> index a334ad4..e453a53 100644<br>>> --- a/src/lua/socket.lua<br>>> +++ b/src/lua/socket.lua<br>>> @@ -102,6 +102,9 @@ local gc_socket_sentinel = ffi.new(gc_socket_t, { fd = -1 })<br>>><br>>> local function socket_close(socket)<br>>> local fd = check_socket(socket)<br>>> + if socket._server_socket_closed ~= nil then<br>>> + socket._server_socket_closed = true<br>>> + end<br>> 2. I think we have an option here to skip this check<br>> and simply always assign.<br>> socket._server_socket_closed = true<br>> In my opinion it looks a bit more obvious and breaks nothing.<br> From the code correctness point of view, this really doesn't<br>break anything. But IMO this is a property of a subset of sockets<br>(tcp server sockets) not all. In other words, a server socket<br>can be used in the context of a "base" socket, but a "base" socket<br>can't be used in the context of a server socket<br>(like a "base" and an "inheritor").</div></div></div></div></blockquote></div><div>You are right, I see. Let’s not mess up server socket field with general fields.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>>> socket._errno = nil<br>>> local r = ffi.C.coio_close(fd)<br>>> -- .fd is const to prevent tampering<br>>> @@ -1082,6 +1085,9 @@ local function tcp_server_loop(server, s, addr)<br>>> fiber.name(format("%s/%s:%s", server.name, addr.host, addr.port), {truncate = true})<br>>> log.info("started")<br>>> while socket_readable(s) do<br>>> + if s._server_socket_closed then<br>>> + break<br>>> + end<br>>> local sc, from = socket_accept(s)<br>>> if sc == nil then<br>>> local errno = s._errno<br>>> @@ -1216,6 +1222,9 @@ local function tcp_server(host, port, opts, timeout)<br>>> return nil<br>>> end<br>>> fiber.create(tcp_server_loop, server, s, addr)<br>>> + -- The _server_socket_closed flag is necessary for the server<br>>> + -- to stop correctly when closing the socket.<br>>> + s._server_socket_closed = false<br>>> return s, addr<br>>> end<br>>><br>>> diff --git a/test/app/socket.result b/test/app/socket.result<br>>> index 9f0ea0a..6206848 100644<br>>> --- a/test/app/socket.result<br>>> +++ b/test/app/socket.result<br>>> @@ -2914,3 +2914,17 @@ srv:close()<br>>> ---<br>>> - true<br>>> ...<br>>> +--<br>>> +-- gh-4087: fix error while closing socket.tcp_server<br>>> +--<br>>> +srv = socket.tcp_server('127.0.0.1', 0, function() end)<br>>> +---<br>>> +...<br>>> +srv:close()<br>>> +---<br>>> +- true<br>>> +...<br>>> +test_run:wait_log('default', 'stopped', 1024, 0.01)<br>>> +---<br>>> +- stopped<br>>> +...<br>>> diff --git a/test/app/socket.test.lua b/test/app/socket.test.lua<br>>> index d1fe7ad..a399fca 100644<br>>> --- a/test/app/socket.test.lua<br>>> +++ b/test/app/socket.test.lua<br>>> @@ -987,4 +987,9 @@ s:settimeout(1)<br>>> s:receive('*a')<br>>> s:close()<br>>> srv:close()<br>>> -<br>>> +--<br>>> +-- gh-4087: fix error while closing socket.tcp_server<br>>> +--<br>>> +srv = socket.tcp_server('127.0.0.1', 0, function() end)<br>>> +srv:close()<br>>> +test_run:wait_log('default', 'stopped', 1024, 0.01)<br>>> --<br>>> 2.7.4<br>>><br>><br>><br>> --<br>> Ilya Kosarev<br>><br>></div></div></div></div></blockquote> <div> </div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Ilya Kosarev</div></div></div><div> </div></div></BODY></HTML>