<HTML><BODY><div><div>Hi!</div><div> </div><div>Thanks for the patch.</div><div> </div><div>LGTM, see 2 nits below.<br> </div><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_15877312100268332610_BODY">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.</div></div></div></div></blockquote></div><div>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,</div><div>server's fiber used to fail with an attempt to validate</div><div>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.</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_soc9ket_closed ~= nil then<br>+ socket._server_socket_closed = true<br>+ end</div></div></div></div></blockquote></div><div>2. I think we have an option here to skip this check</div><div>and simply always assign.</div><div>socket._server_socket_closed = true</div><div>In my opinion it looks a bit more obvious and breaks nothing.</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> </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><style type="text/css"></style></BODY></HTML>