soreceive_stream: issues with O_NONBLOCK

[Andre Oppermann]

On 03.07.2011 17:00, Mikolaj Golub wrote:
> Hi,

Hi Mikolaj,

> Trying soreceive_stream I found that many applications (like firefox, pidgin,
> gnus) might hang in soreceive_stream/sbwait.

Many thanks for testing soreceive_stream.

> It was shown up that the issue was with O_NONBLOCK connections -- they blocked
> in recv() when should not have been.
 >
> This can be checked with this simple test:
>
> http://people.freebsd.org/~trociny/test_nonblock.c
>
> In soreceive_stream we have the following code that looks wrong:
>
>     1968         /* Socket buffer is empty and we shall not block. */
>     1969         if (sb->sb_cc == 0&&
>     1970             ((sb->sb_flags&  SS_NBIO) || (flags&  (MSG_DONTWAIT|MSG_NBIO)))) {
>     1971                 error = EAGAIN;
>     1972                 goto out;
>     1973         }
>
> It should check so->so_state agains SS_NBIO, not sb->sb_flags. But just

Indeed.

> changing this is not enough. This check is called too early -- before checking
> that socket state is not SBS_CANTRCVMORE. As a result, if the peer closes the
> connection recv() returns EAGAIN instead of 0. See this example:
>
> http://people.freebsd.org/~trociny/test_close.c
>
> So I moved the "nonblock" check below SBS_CANTRCVMORE check and ended up with
> this patch:
>
> http://people.freebsd.org/~trociny/uipc_socket.c.soreceive_stream.patch
>
> It works for me fine.

OK.

> Also, this part looks wrong:
>
>     1958         /* We will never ever get anything unless we are connected. */
>     1959         if (!(so->so_state&  (SS_ISCONNECTED|SS_ISDISCONNECTED))) {
>     1960                 /* When disconnecting there may be still some data left. */
>     1961                 if (sb->sb_cc>  0)
>     1962                         goto deliver;
>     1963                 if (!(so->so_state&  SS_ISDISCONNECTED))
>     1964                         error = ENOTCONN;
>     1965                 goto out;
>     1966         }
>
> Why we check in 1959 that state is not SS_ISDISCONNECTED? If it is valid then
> the check at 1963 is useless becase it will be always true. Shouldn't it be
> something like below?
 >
> 	if (!(so->so_state&  (SS_ISCONNECTED|SS_ISCONNECTING))) {
>   		/* When disconnecting there may be still some data left. */
>   		if (sb->sb_cc>  0)
>   			goto deliver;
> 		error = ENOTCONN;
>   		goto out;
>   	}
>
> (I don't see why we souldn't set ENOTCONN if the state is SS_ISDISCONNECTED).

ENOTCONN is only valid in the beginning of the connection.  When we reach
SS_INDISCONNECTED the connection was connected before and we have to return
a 0-read to signal EOF.  It can be simplified by immediately setting the
error and going out.  The other cases can't ever be true.

Please try this patch:
  http://people.freebsd.org/~andre/soreceive_stream.diff-20110707

I can't fully test it myself due to being 1000km from my development
machine and having only limited hw access for rebooting.

> And the last :-). Currently, to try soreceive_stream one need to rebuild kernel
> with TCP_SORECEIVE_STREAM and then set tunable net.inet.tcp.soreceive_stream.
> Why do we need TCP_SORECEIVE_STREAM option? Wouldn't tunable be enough? It
> would simplify trying soreceive_stream by users and we might have more
> testing/feedback.

Done. See r223839.

-- 
Andre