Tarantool development patches archive
 help / color / mirror / Atom feed
From: "Ilya Kosarev" <i.kosarev@tarantool.org>
To: lvasiliev <lvasiliev@tarantool.org>
Cc: tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH] socket: fix error while closing socket.tcp_server
Date: Wed, 06 May 2020 11:20:42 +0300	[thread overview]
Message-ID: <1588753242.784725060@f158.i.mail.ru> (raw)
In-Reply-To: <b8bc5b11-d7e4-2bbe-7c33-2a622975e7a0@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 --]

  reply	other threads:[~2020-05-06  8:20 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-24 12:26 Leonid Vasiliev
2020-04-24 14:03 ` lvasiliev
2020-04-29 12:12 ` Ilya Kosarev
2020-05-06  8:01   ` lvasiliev
2020-05-06  8:20     ` Ilya Kosarev [this message]
2020-05-06 15:38 ` Alexander Turenko
2020-05-07  7:29   ` lvasiliev
2020-05-13 14:21     ` Alexander Turenko
2020-05-19 19:52       ` lvasiliev

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1588753242.784725060@f158.i.mail.ru \
    --to=i.kosarev@tarantool.org \
    --cc=lvasiliev@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH] socket: fix error while closing socket.tcp_server' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox