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

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

[Leonid Vasiliev]

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.

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
     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

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

[lvasiliev]

On 24.04.2020 15:26, Leonid Vasiliev wrote:
> 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.
> 
> Fixes: #4087
> ---
> @Changelog fix error while closing socket.tcp_server socket(gh-4087)
https://github.com/tarantool/tarantool/issues/4087
https://github.com/tarantool/tarantool/tree/lvasiliev/gh-4087-fix-socket-stuff

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

[Ilya Kosarev]

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


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.
>
>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.
>     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
 

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

Re: [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
>   
> 

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

[Ilya Kosarev]

[-- 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 --]

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

[Alexander Turenko]

> @@ -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

Can we use the same check `<s>._gc_socket.fd >= 0` as in check_socket()
(we can wrap it to a function like socket_is_closed())?

It seems that using the same way for determining whether a socket is
closed owuld simplify the code a bit.

WBR, Alexander Turenko.

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

[lvasiliev]

Hi! Thank you for the review.

On 06.05.2020 18:38, Alexander Turenko wrote:
>> @@ -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
> 
> Can we use the same check `<s>._gc_socket.fd >= 0` as in check_socket()
> (we can wrap it to a function like socket_is_closed())?
> 
> It seems that using the same way for determining whether a socket is
> closed owuld simplify the code a bit.
> 
> WBR, Alexander Turenko.
> 
If I understand correctly, in this case we must not only check whether
the socket is closed or not, but also check whether it was closed
without errors by checking errno. In fact, we come to my first patchset,
where I disable throwing an exception from the check_socket(). Disabling
throwing an exception is the right way because now our API doesn't
correspond to the documentation (I will create a ticket for this). But
fixing a socket check leads to big changes in the code that are not
related to the problem and will not pass the review. I can wrap the flag
check in a method, but then all sockets should have a flag and it's look
like overkill (this will duplicate the functionality of the
check_socket() function after fix).
In addition, we don't check EV_ERROR and it seems to me that this can
also lead to the fact that the check you proposed may be insufficient.

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

[Alexander Turenko]

TL;DR: I would ask you a case that will show the difference you mention.

The same, but with more words, below.

WBR, Alexander Turenko.

> > > @@ -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
> >
> > Can we use the same check `<s>._gc_socket.fd >= 0` as in check_socket()
> > (we can wrap it to a function like socket_is_closed())?
> >
> > It seems that using the same way for determining whether a socket is
> > closed owuld simplify the code a bit.
> >
> > WBR, Alexander Turenko.
> >
> If I understand correctly, in this case we must not only check whether
> the socket is closed or not, but also check whether it was closed
> without errors by checking errno. In fact, we come to my first patchset,

Execution path is the following:

 | tcp_server()
 |     local s, addr = tcp_server_bind(...)
 |     if not s then <...> end
 |     <s is correct socket here>
 |     fiber.create(tcp_server_loop, server, s, addr)
 |         while socket_readable(s) do -- it is yield
 |             <a check should be added here>
 |             local sc, from = socket_accept(s)
 |             <...>
 |         end

What can appear with <s> when we're in yield? It can be closed or
mangled in some bad way by a user (say, `socket._gc_socket = {}`). The
latter is invalid usage, so we should handle only closing the socket.

The next question is what condition we should use to check whether a
socket is closed. Look at check_socket() and socket_close() functions
(marked your new code with '+' marks):

 | local function check_socket(socket)
 |     local gc_socket = type(socket) == 'table' and socket._gc_socket
 |     if ffi.istype(gc_socket_t, gc_socket) then
 |         local fd = gc_socket.fd
 |         if fd >= 0 then
 |             return fd
 |         else
 |             error("attempt to use closed socket")
 |         end
 |     else
 |         local msg = "Usage: socket:method()"
 |         if socket ~= nil then msg = msg .. ", called with non-socket" end
 |         error(msg)
 |     end
 | end

 | 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
 |     socket._errno = nil
 |     local r = ffi.C.coio_close(fd)
 |     -- .fd is const to prevent tampering
 |     ffi.copy(socket._gc_socket, gc_socket_sentinel, ffi.sizeof(gc_socket_t))
 |     if r ~= 0 then
 |         socket._errno = boxerrno()
 |         return false
 |     end
 |     return true
 | end

check_socket() use `<s>._gc_socket.fd >= 0` to determine whether socket
is closed. I don't see a reason to add another way to determine it. But
I would extract the check into a function:

 | local function socket_is_closed(socket)
 |     return socket._gc_socket.fd < 0
 | end

If I miss some case, please, show reproducer code, which reveals it.

> where I disable throwing an exception from the check_socket(). Disabling

Does not look the same: check_socket() is used everywhere in the API
functions.

> throwing an exception is the right way because now our API doesn't
> correspond to the documentation (I will create a ticket for this). But
> fixing a socket check leads to big changes in the code that are not
> related to the problem and will not pass the review. I can wrap the flag

We discussed Leonid's question externally. It is about raising an error
for a closed socket handle. I would consider closed socket as an illegal
parameter and raise an error as for any other illegal parameter error.
At least until someone will show that a correct code may pass a closed
socket to a socket module function / a handle method due to some
external influence (when other end of socket becomes closed or so).

Our documentation usually miss to mention that an illegal parameter
error is raised, not returned. This is not good, but it is usual
convention for our modules in fact.

> check in a method, but then all sockets should have a flag and it's look
> like overkill (this will duplicate the functionality of the
> check_socket() function after fix).

Since check_socket() change you propose (don't raise an error) is
questionable I cannot take it as the argument.

> In addition, we don't check EV_ERROR and it seems to me that this can
> also lead to the fact that the check you proposed may be insufficient.

To be honest, I don't get how it is related to the question we discuss,
but anyway.

From http://pod.tst.eu/http://cvs.schmorp.de/libev/ev.pod

 | EV_ERROR
 |
 | An unspecified error has occurred, the watcher has been stopped. This
 | might happen because the watcher could not be properly started
 | because libev ran out of memory, a file descriptor was found to be
 | closed or any other problem. Libev considers these application bugs.
 |                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 |
 | You best act on it by reporting the problem and somehow coping with
 | the watcher being stopped. Note that well-written programs should not
 | receive an error ever, so when your watcher receives it, this usually
 | indicates a bug in your program.
 |
 | Libev will usually signal a few "dummy" events together with an
 | error, for example it might indicate that a fd is readable or
 | writable, and if your callbacks is well-written it can just attempt
 |                                                        ^^^^^^^^^^^^
 | the operation and cope with the error from read() or write(). This
 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 | will not work in multi-threaded programs, though, as the fd could
 | already be closed and reused for another thing, so beware.

So, first: it should not appear at all. Second, we can just read() /
write() and look at the return value + errno.

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

[lvasiliev]

Hi! Thanks for the review.

On 13.05.2020 17:21, Alexander Turenko wrote:
> TL;DR: I would ask you a case that will show the difference you mention.
> 
> The same, but with more words, below.
> 
> WBR, Alexander Turenko.
> 
I couldn't find a valid way to close the socket outside. So, you
won this fight :). I sent a new version of the patchset.
>>>> @@ -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
>>>
>>> Can we use the same check `<s>._gc_socket.fd >= 0` as in check_socket()
>>> (we can wrap it to a function like socket_is_closed())?
>>>
>>> It seems that using the same way for determining whether a socket is
>>> closed owuld simplify the code a bit.
>>>
>>> WBR, Alexander Turenko.
>>>
>> If I understand correctly, in this case we must not only check whether
>> the socket is closed or not, but also check whether it was closed
>> without errors by checking errno. In fact, we come to my first patchset,
> 
> Execution path is the following:
> 
>   | tcp_server()
>   |     local s, addr = tcp_server_bind(...)
>   |     if not s then <...> end
>   |     <s is correct socket here>
>   |     fiber.create(tcp_server_loop, server, s, addr)
>   |         while socket_readable(s) do -- it is yield
>   |             <a check should be added here>
>   |             local sc, from = socket_accept(s)
>   |             <...>
>   |         end
> 
> What can appear with <s> when we're in yield? It can be closed or
> mangled in some bad way by a user (say, `socket._gc_socket = {}`). The
> latter is invalid usage, so we should handle only closing the socket.
> 
> The next question is what condition we should use to check whether a
> socket is closed. Look at check_socket() and socket_close() functions
> (marked your new code with '+' marks):
> 
>   | local function check_socket(socket)
>   |     local gc_socket = type(socket) == 'table' and socket._gc_socket
>   |     if ffi.istype(gc_socket_t, gc_socket) then
>   |         local fd = gc_socket.fd
>   |         if fd >= 0 then
>   |             return fd
>   |         else
>   |             error("attempt to use closed socket")
>   |         end
>   |     else
>   |         local msg = "Usage: socket:method()"
>   |         if socket ~= nil then msg = msg .. ", called with non-socket" end
>   |         error(msg)
>   |     end
>   | end
> 
>   | 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
>   |     socket._errno = nil
>   |     local r = ffi.C.coio_close(fd)
>   |     -- .fd is const to prevent tampering
>   |     ffi.copy(socket._gc_socket, gc_socket_sentinel, ffi.sizeof(gc_socket_t))
>   |     if r ~= 0 then
>   |         socket._errno = boxerrno()
>   |         return false
>   |     end
>   |     return true
>   | end
> 
> check_socket() use `<s>._gc_socket.fd >= 0` to determine whether socket
> is closed. I don't see a reason to add another way to determine it. But
> I would extract the check into a function:
> 
>   | local function socket_is_closed(socket)
>   |     return socket._gc_socket.fd < 0
>   | end
> 
> If I miss some case, please, show reproducer code, which reveals it.
> 
>> where I disable throwing an exception from the check_socket(). Disabling
> 
> Does not look the same: check_socket() is used everywhere in the API
> functions.
I was thinking of something like that you can return nil in case of
a closed socket and throw exception in case of an invalid argument.
> 
>> throwing an exception is the right way because now our API doesn't
>> correspond to the documentation (I will create a ticket for this). But
>> fixing a socket check leads to big changes in the code that are not
>> related to the problem and will not pass the review. I can wrap the flag
> 
> We discussed Leonid's question externally. It is about raising an error
> for a closed socket handle. I would consider closed socket as an illegal
> parameter and raise an error as for any other illegal parameter error.
> At least until someone will show that a correct code may pass a closed
> socket to a socket module function / a handle method due to some
> external influence (when other end of socket becomes closed or so).
> 
> Our documentation usually miss to mention that an illegal parameter
> error is raised, not returned. This is not good, but it is usual
> convention for our modules in fact.
> 
For closed socket a separate exception used without usual "Usage ...".
I suggest not discussing the problem of API documentation here. I was 
wrong when started.
>> check in a method, but then all sockets should have a flag and it's look
>> like overkill (this will duplicate the functionality of the
>> check_socket() function after fix).
> 
> Since check_socket() change you propose (don't raise an error) is
> questionable I cannot take it as the argument.
Ok.
> 
>> In addition, we don't check EV_ERROR and it seems to me that this can
>> also lead to the fact that the check you proposed may be insufficient.
> 
> To be honest, I don't get how it is related to the question we discuss,
> but anyway.
> 
>  From http://pod.tst.eu/http://cvs.schmorp.de/libev/ev.pod
> 
>   | EV_ERROR
>   |
>   | An unspecified error has occurred, the watcher has been stopped. This
>   | might happen because the watcher could not be properly started
>   | because libev ran out of memory, a file descriptor was found to be
>   | closed or any other problem. Libev considers these application bugs.
>   |                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>   |
>   | You best act on it by reporting the problem and somehow coping with
>   | the watcher being stopped. Note that well-written programs should not
>   | receive an error ever, so when your watcher receives it, this usually
>   | indicates a bug in your program.
>   |
>   | Libev will usually signal a few "dummy" events together with an
>   | error, for example it might indicate that a fd is readable or
>   | writable, and if your callbacks is well-written it can just attempt
>   |                                                        ^^^^^^^^^^^^
>   | the operation and cope with the error from read() or write(). This
>   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>   | will not work in multi-threaded programs, though, as the fd could
>   | already be closed and reused for another thing, so beware.
> 
> So, first: it should not appear at all. Second, we can just read() /
> write() and look at the return value + errno.
>