Skip to content
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

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

Ruben2424
Copy link
Contributor

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 Ruben2424 changed the title simplify traits and implementation simplify traits and error handling Jun 16, 2024
@Ruben2424
Copy link
Contributor Author

Ruben2424 commented Jun 16, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@Ruben2424 Ruben2424 requested a review from seanmonstar August 19, 2024 15:55
@seanmonstar
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor Author

Ruben2424 commented Sep 11, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants