[tarantool-patches] Re: [PATCH 03/11] sio: remove exceptions
Vladimir Davydov
vdavydov.dev at gmail.com
Wed Dec 5 11:42:41 MSK 2018
On Wed, Dec 05, 2018 at 12:29:10AM +0300, Vladislav Shpilevoy wrote:
>
> > > /**
> > > * Receive a message on a socket. A wrapper for recvfrom(), but
> > > - * throws exception on error except EAGAIN, EINTR, EWOULDBLOCK.
> > > + * sets diagnostics on error except EAGAIN, EINTR, EWOULDBLOCK.
> > > + * @param fd Socket to receive from.
> > > + * @param buf Buffer to save message.
> > > + * @param len Size of @a buf.
> > > + * @param flags recvfrom() flags.
> > > + * @param[out] src_addr Source address.
> > > + * @param[in, out] addrlen Size of @a src_addr.
> > > + * @param[out] is_error_fatal Set to true, if an error occured and
> > > + * it was not EAGAIN, EINTR and EWOULDBLOCK.
> > > + *
> > > + * @param How many bytes are received, or -1 on error.
> > > */
> > > ssize_t
> > > sio_recvfrom(int fd, void *buf, size_t len, int flags,
> > > - struct sockaddr *src_addr, socklen_t *addrlen);
> > > + struct sockaddr *src_addr, socklen_t *addrlen,
> > > + bool *is_error_fatal);
> >
> > This new API doesn't look good to me. Let's instead set diag and return
> > -1 on every error and introduce a helper to check if a particular error
> > code means that the socket would block (the name might need to be
> > refined):> sio_wouldblock(errno)
> >
> > ?
> >
>
> The only purpose of the out flag was to hide from a user details
> of how exactly to check if an error is not critical: via errno, or
> via checking diag_is_empty(diag_get()) or via any other way. So a
> user wouldn't need to include errno or use complex constructions
> like with diag.
>
> But as you wish. Lets find another way.
>
> Any names having 'wouldblock', 'block' or describing other concrete
> behaviour can not be used. At least, sio_bind/listen can set EADDRINUSE,
> which is not about blocking.
>
> Also, notion of 'critical' is different for various functions. I've
> grouped errors in 4 groups:
>
> EINPROGRESS - sio_connect
>
> EADDRINUSE - sio_bind, sio_listen
>
> EAGAIN - sio_accept, sio_write,
> EWOULDBLOCK sio_writev, sio_readn_ahead,
> EINTR sio_sendto, sio_recvfrom
>
> ECONNRESET - sio_read
> EAGAIN
> EWOULDBLOCK
> EINTR
>
> I can not implement one function for checking of any
> error via errno because
>
> * EADDRINUSE is not critical for bind/listen, but
> critical for connect;
>
> * EAGAIN, EINTR are not critical for IO, but critical
> for connect;
>
> * ECONNRESET is not critical for sio_read, but critical
> for sio_sendto.
>
> The list is not complete I think, and I do not want to fix some
> crutches in a single error checking function each time when
> something changed in any other sio function.
OK, I see. I'll think what we can do about it.
>
> I can not use the simplest way !diag_is_empty(diag_get()) as well
> because as I discovered, iproto thread does not clean diag in
> some places after processing an error. For example,
> iproto_connection_resume on error leaves an error in diag and
> the error lives in the thread until next error. The same in
> iproto_connection_on_input(), where catch() block does not
> clears diag.
>
> I could use diag if I fixed these places, but I do not want
> to rely on cleanness of external code.
>
> I decided to return from some functions a special code that
> can be checked by sio_is_error_fatal().
>
> See v2 thread for a new version of the commit. I do not paste
> diff here, since in fact the patch is rewritten completely.
More information about the Tarantool-patches
mailing list