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

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

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


Hi!
 
Thanks for the answers.
 
No new comments. LGTM
  
>Среда, 6 мая 2020, 11:01 +03:00 от lvasiliev <lvasiliev@tarantool.org>:
> 
>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.
Right, I think the last one is the most clear.
>>>
>>> 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_socket_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").
You are right, I see. Let’s not mess up server socket field with general fields.
>>>      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
>>
>> 
 
 
--
Ilya Kosarev
 

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