Re: [tarantool-patches] Re: [PATCH 03/11] sio: remove exceptions

[Vladimir Davydov <vdavydov.dev@gmail.com>]

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.