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

[lvasiliev]

Hi! Thanks for the review.

On 29.04.2020 15:12, Ilya Kosarev wrote:
> 
> 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.

Updated description:

The tcp server starts in a separate fiber.
When server's socket is closed from another fiber,
an exception will be thrown in server's loop from
check_socket function.
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.
 From the code correctness point of view, this really doesn't
break anything. But IMO this is a property of a subset of sockets
(tcp server sockets) not all. In other words, a server socket
can be used in the context of a "base" socket, but a "base" socket
can't be used in the context of a server socket
(like a "base" and an "inheritor").
>>       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
>   
>