From: lvasiliev <lvasiliev@tarantool.org> To: Ilya Kosarev <i.kosarev@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH] socket: fix error while closing socket.tcp_server Date: Wed, 6 May 2020 11:01:41 +0300 [thread overview] Message-ID: <b8bc5b11-d7e4-2bbe-7c33-2a622975e7a0@tarantool.org> (raw) In-Reply-To: <1588162321.664592164@f301.i.mail.ru> 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 > >
next prev parent reply other threads:[~2020-05-06 8:01 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 [this message] 2020-05-06 8:20 ` Ilya Kosarev 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=b8bc5b11-d7e4-2bbe-7c33-2a622975e7a0@tarantool.org \ --to=lvasiliev@tarantool.org \ --cc=i.kosarev@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