From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 07298469710 for ; Wed, 6 May 2020 11:01:42 +0300 (MSK) References: <1588162321.664592164@f301.i.mail.ru> From: lvasiliev Message-ID: Date: Wed, 6 May 2020 11:01:41 +0300 MIME-Version: 1.0 In-Reply-To: <1588162321.664592164@f301.i.mail.ru> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [Tarantool-patches] [PATCH] socket: fix error while closing socket.tcp_server List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ilya Kosarev Cc: tarantool-patches@dev.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. >> >> 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 > >