simplify traits and error handling

[Ruben2424]

This PR implements parts of the proposed changes from #177. Most of the code is from #200.

This replaces poll_accept_recv and poll_accept_bidi with one poll_incoming method to accept unidirectional and bidirectional streams. (See #177 for the reasons)

This also removes Option from the poll_incoming. h3_quinn always returned Some(...).

Once this is merged I will do follow up PRs to explore error types for connection/stream errors and to improve connection closure and error handling.

[Ruben2424]

Some h3spec test are failing. This is probably not because of h3. When there is a high CPU load i could reproduce it locally. According to Wireshark h3 does respond with the right Error Code even if the h3spec tests are failing. It also seamed only to happen when sending a grease stream.

I think the issue is, that h3 spec does not accept the STOP_SENDING quic frame for the request stream. But I am not sure why.

edit: without the changes from this PR this seems not to happen.

[Ruben2424]

Tests are passing with the newest version of h3spec. But i got this error sometimes with the newest version too. Should this block the PR?

[Ruben2424]

Now that the CI is green i think this is ready for a review now.

[seanmonstar]

I see the background issue mentions a bit about quinn, I'm curious about other perspectives.

@camshaft if you have a moment, would you care to share if you've seen one way or the other be better? Have users of s2n-quic made better use of accepting a PeerStream, or the separate methods?

[camshaft]

Have users of s2n-quic made better use of accepting a PeerStream, or the separate methods?

I've seen uses of both - I think it just depends on how your application's accept loop is structured. A lot of them aren't set up to handle recv streams and assume bidirectional only. In these cases, it's easier to just call the accept_bidirectional method. On the other hand, if you do it this way you do need to remember to actively poll both queue types or configure the advertised stream limits for the type you're not planning on handling.

I think there's also value in being able to apply back pressure differently depending on stream type. With a unified accept method, you lose a bit of control to do that. For example, you could have an application that wants to round-robin between each stream type. This is easy to implement with split methods - you just call one after the other. With a unified method, you can't make any guarantees on which stream type comes next.

[Ruben2424]

I've seen uses of both

If both is possible with s2n-quic i would go for it. If it turns out to be not useful, we can change it back again. What do you think?

I think it just depends on how your application's accept loop is structured. A lot of them aren't set up to handle recv streams and assume bidirectional only. In these cases, it's easier to just call the accept_bidirectional method. On the other hand, if you do it this way you do need to remember to actively poll both queue types or configure the advertised stream limits for the type you're not planning on handling.

Both stream types are necessary for h3 and one method will imo reduce the complexity because in h3, users only have one future, to poll, in order to accept streams atm. Additionally this will reduce the possible ways a quic implementation can give errors to h3 reducing the trait complexity.

I think there's also value in being able to apply back pressure differently depending on stream type.

Can this be useful for http3?

[Ruben2424]

@seanmonstar a few Months have passed. Have you thought about this? Is this the right way to go?

[Ruben2424]

I will close that PR for now. If it becomes relevant again i can make a follow up PR. #271 improved error handeling without combining accept_bi and accept_recv.