-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
pass frame / stream type parsing errors to the hijacker callbacks #3429
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3429 +/- ##
==========================================
+ Coverage 85.66% 85.67% +0.01%
==========================================
Files 136 136
Lines 9948 9957 +9
==========================================
+ Hits 8521 8530 +9
- Misses 1049 1050 +1
+ Partials 378 377 -1
Continue to review full report at Codecov.
|
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.
Granted, I'm not familiar with this codebase, this change seems relatively straightforward and it makes sense that a hijacker would have more context than the general library.
One question not specific to this PR but in general is that it seems like there are UniStreamHijacker
tests but no matching StreamHijacker
tests. Any specific reason?
Please use #3391 if you like. |
9afa9fb
to
225e880
Compare
When a stream is reset, we might not have received the frame / stream type yet. The callback might be able to identify if it was a stream intended for that application by analyzing the stream reset error.
225e880
to
96c0dac
Compare
@hareku Thank you. I had actually missed your PR. I've merged it now, and rebased this PR. @MarcoPolo Thanks for pointing this is out in the review. This actually helped uncover a bug, which I fixed in 5cb2e82. |
When a stream is reset, we might not have received the frame / stream type yet. The callback might be able to identify if it was a stream intended for that application by analyzing the stream reset error.
This will allow us to resolve marten-seemann/go-libp2p-webtransport#3. The WebTransport implementation can determine if the stream reset error code is in the WebTransport error code range.
@ydnar @hareku Thoughts?