Tarantool development patches archive
 help / color / mirror / Atom feed
From: Alexander Turenko <alexander.turenko@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, 13 May 2020 17:21:25 +0300	[thread overview]
Message-ID: <20200513142125.cmki5dsthspszvl7@tkn_work_nb> (raw)
In-Reply-To: <8c0c398a-f043-0637-48f6-0d350b7a8c03@tarantool.org>

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.

  reply	other threads:[~2020-05-13 14:21 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
2020-05-06 15:38 ` Alexander Turenko
2020-05-07  7:29   ` lvasiliev
2020-05-13 14:21     ` Alexander Turenko [this message]
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=20200513142125.cmki5dsthspszvl7@tkn_work_nb \
    --to=alexander.turenko@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