Skip to content

deps: h3-0.0.8 (s2n-quic-h3) #2629

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

Merged
merged 1 commit into from
May 16, 2025
Merged

Conversation

junkurihara
Copy link
Contributor

Release Summary:

This change supports h3 v0.0.8 that has breaking changes on its API. This was implemented by referring to h3-quinn v0.10.0, specifically by following the commit efdc2d8bfda0d86c0efdd36867ee6183f92d54d6.

Resolved issues:

resolves #2628 (dependabot PR)

Description of changes:

This change updates quic/s2n-quic-h3/src/s2n_quic.rs as the upstream h3 traits changes the manner of error handling.

Call-outs:

I believe the change does not need to be updated at this point since it just follows the h3's API changes.

However, I am not sure if changes L53-L58 and L74-L79 (cases when a connection closes with no error) are suitable with underlying s2n-quic layer.

Testing:

Existing tests (but there is no test in s2n-quic-h3 as well as h3-quinn).

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@boquan-fang
Copy link
Contributor

Hi Jun, thanks for cutting this PR to us. We will take a look and determine how to include h3 updates to our codebase.

@boquan-fang boquan-fang requested a review from maddeleine May 12, 2025 17:38
@boquan-fang
Copy link
Contributor

Hello Jun, we have assigned a reviewer for this PR and we will give a review for this PR later. To get started, this PR doesn't pass our CI, since it is not compiled properly. You can find the specific error in the CI run.

@junkurihara
Copy link
Contributor Author

@boquan-fang Hi, thanks!
As you mentioned, from the results of CI, I found that I forgot to update s2n-quic-qns package according to h3 reexports. So I just fixed. I hope this passes all required tests.

@maddeleine
Copy link
Contributor

Also our CI is currently broken. So qns/interop-report isn't expected to pass right now.

@junkurihara
Copy link
Contributor Author

Thanks for the comments! Most of commented part just followed the structure of h3-quinn, which actually doesn't fit to s2n-quic-h3. So I will update the branch and respond to comments shortly.

@junkurihara
Copy link
Contributor Author

Hi. I just pushed a commit responding to the comments. I hope everything has been resolved now.

@boquan-fang boquan-fang requested a review from maddeleine May 15, 2025 16:29
Copy link
Contributor

@maddeleine maddeleine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for doing this! Hopefully h3 stops doing breaking changes, because that was a big refactor. Can you rebase your PR to get the newest commits? I just fixed our CI so we should be able to get the tests passing now on your PR.

@junkurihara
Copy link
Contributor Author

@maddeleine Thank you for reviewing! I just rebased my branch to the HEAD of main, and squashed several commits :-)

@maddeleine maddeleine merged commit a5921e5 into aws:main May 16, 2025
119 checks passed
@junkurihara junkurihara deleted the deps/h3-0.0.8 branch May 16, 2025 17:36
dougch pushed a commit that referenced this pull request May 19, 2025
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