-
Notifications
You must be signed in to change notification settings - Fork 6
Fix webtransport conn not being served due to missing EnableStreamResetPartialDelivery
#392
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
Conversation
EnableStreamResetPartialDelivery
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes WebTransport connection issues by enabling EnableStreamResetPartialDelivery in QUIC configurations, which is a new requirement from webtransport-go. The change also improves error logging for WebTransport connections.
Changes:
- Added
EnableStreamResetPartialDelivery: trueto both client (Dial) and server (Listen) QUIC configurations - Enhanced error logging for WebTransport connection failures with more specific error messages
- Improved consistency in error log messages between WebTransport and QUIC connection handling
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| rhp/v4/quic/quic.go | Enables EnableStreamResetPartialDelivery for both client and server QUIC configs; adds error logging for ServeQUICConn and clarifies log messages |
| .changeset/enable_streamresetpartialdelivery_when_listening_for_quic_connections.md | Documents the change as a patch-level update |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.changeset/enable_streamresetpartialdelivery_when_listening_for_quic_connections.md
Show resolved
Hide resolved
|
Did you intentionally leave out the call to |
n8mgr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to call webtransport.ConfigureHTTP3Server as described in the issue?
|
@Alrighttt @n8mgr no that was a mistake. I added a regression test toe make sure it works and doesn't break itself again without us knowing. |
Alrighttt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
a2977a7 to
95df8ef
Compare
Seems like a new requirement for
webtransport-go.Relevant code can be found here.
Closes #391