-
Notifications
You must be signed in to change notification settings - Fork 82
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
simplify traits and error handling #244
base: master
Are you sure you want to change the base?
Conversation
Some h3spec test are failing. This is probably not because of h3. 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: |
Tests are passing with the newest version of h3spec. But i got this error sometimes with the newest version too. |
Now that the CI is green i think this is ready for a review now. |
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? |
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 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. |
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?
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.
Can this be useful for http3? |
This PR implements parts of the proposed changes from #177. Most of the code is from #200.
This replaces
poll_accept_recv
andpoll_accept_bidi
with onepoll_incoming
method to accept unidirectional and bidirectional streams. (See #177 for the reasons)This also removes
Option
from thepoll_incoming
. h3_quinn always returnedSome(...)
.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.