Re: [Tarantool-patches] [PATCH] socket: fix error while closing socket.tcp_server

["Ilya Kosarev" <i.kosarev@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 3444 bytes --]


Hi!
 
Thanks for the patch.
 
LGTM, see 2 nits below.
 
>The tcp server starts in a separate fiber and when
>the socket closes from another fiber the server's
>fiber fails with an attempt to validate the server
>socket. For check the socket status at server's
>fiber a flag has been added and now server's fiber
>will terminate correctly if the socket was closed.
1. I think this way it might sound a bit better:
The tcp server starts in a separate fiber and when
it’s socket is being closed from another fiber,
server's fiber used to fail with an attempt to validate
the server socket. To check socket status at server's
fiber a flag has been added and now server's fiber
terminates correctly when the socket is being closed.
>
>Fixes: #4087
>---
>@Changelog fix error while closing socket.tcp_server socket(gh-4087)
>
> src/lua/socket.lua | 9 +++++++++
> test/app/socket.result | 14 ++++++++++++++
> test/app/socket.test.lua | 7 ++++++-
> 3 files changed, 29 insertions(+), 1 deletion(-)
>
>diff --git a/src/lua/socket.lua b/src/lua/socket.lua
>index a334ad4..e453a53 100644
>--- a/src/lua/socket.lua
>+++ b/src/lua/socket.lua
>@@ -102,6 +102,9 @@ local gc_socket_sentinel = ffi.new(gc_socket_t, { fd = -1 })
> 
> local function socket_close(socket)
>     local fd = check_socket(socket)
>+ if socket._server_soc9ket_closed ~= nil then
>+ socket._server_socket_closed = true
>+ end
2. I think we have an option here to skip this check
and simply always assign.
socket._server_socket_closed = true
In my opinion it looks a bit more obvious and breaks nothing.
>     socket._errno = nil
>     local r = ffi.C.coio_close(fd)
>     -- .fd is const to prevent tampering
>@@ -1082,6 +1085,9 @@ local function tcp_server_loop(server, s, addr)
>     fiber.name(format("%s/%s:%s", server.name, addr.host, addr.port), {truncate = true})
>     log.info("started")
>     while socket_readable(s) do
>+ if s._server_socket_closed then
>+ break
>+ end
>         local sc, from = socket_accept(s)
>         if sc == nil then
>             local errno = s._errno
>@@ -1216,6 +1222,9 @@ local function tcp_server(host, port, opts, timeout)
>         return nil
>     end
>     fiber.create(tcp_server_loop, server, s, addr)
>+ -- The _server_socket_closed flag is necessary for the server
>+ -- to stop correctly when closing the socket.
>+ s._server_socket_closed = false
>     return s, addr
> end
> 
>diff --git a/test/app/socket.result b/test/app/socket.result
>index 9f0ea0a..6206848 100644
>--- a/test/app/socket.result
>+++ b/test/app/socket.result
>@@ -2914,3 +2914,17 @@ srv:close()
> ---
> - true
> ...
>+--
>+-- gh-4087: fix error while closing socket.tcp_server
>+--
>+srv = socket.tcp_server('127.0.0.1', 0, function() end)
>+---
>+...
>+srv:close()
>+---
>+- true
>+...
>+test_run:wait_log('default', 'stopped', 1024, 0.01)
>+---
>+- stopped
>+...
>diff --git a/test/app/socket.test.lua b/test/app/socket.test.lua
>index d1fe7ad..a399fca 100644
>--- a/test/app/socket.test.lua
>+++ b/test/app/socket.test.lua
>@@ -987,4 +987,9 @@ s:settimeout(1)
> s:receive('*a')
> s:close()
> srv:close()
>-
>+--
>+-- gh-4087: fix error while closing socket.tcp_server
>+--
>+srv = socket.tcp_server('127.0.0.1', 0, function() end)
>+srv:close()
>+test_run:wait_log('default', 'stopped', 1024, 0.01)
>--
>2.7.4
>  
 
 
--
Ilya Kosarev
 

[-- Attachment #2: Type: text/html, Size: 5007 bytes --]