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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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

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.