-
-
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
fix(http3): handle streamStateSendAndReceiveClosed in onStreamStateChange #4523
fix(http3): handle streamStateSendAndReceiveClosed in onStreamStateChange #4523
Conversation
…ange Signed-off-by: George MacRorie <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4523 +/- ##
==========================================
- Coverage 85.18% 85.14% -0.04%
==========================================
Files 154 154
Lines 14802 14794 -8
==========================================
- Hits 12608 12595 -13
- Misses 1687 1691 +4
- Partials 507 508 +1 ☔ View full report in Codecov by Sentry. |
Hi @GeorgeMac! Thank you for putting in the effort to debug this! Is there any way to properly test this, either as a unit or an end-to-end test. This seems very subtle, and I'm concerned we'll break this again in the future. I'm a bit confused about my own code now:
What do you think? |
I see both of what you're saying there @marten-seemann 👍 Correct me if I am wrong (slowly catching up), the purpose of the state tracking system is twofold:
What you're suggesting makes sense. Another idea could be to remove the callbacks in favour of a couple interfaces to represent the datagrammer and removal of the datagrammer from the parent connection. Something like: type streamCloser interface {
streamClose(quic.StreamID)
}
type sendRecieveError interface {
SetSendError(error)
SetRecieveError(error)
}
type stateTrackingStream struct {
quic.Stream
sendError error
receiveError error
errorer sendRecieveError
closer streamCloser
} Where the purpose of the stateTrackingStream becomes to continue proxying, but the moment the pair of send and recv error are non-nil, it calls
Happy to take a stab at whichever approach makes sense and write tests around it. |
Correct! Your idea works as well. No strong preference either way. I usually find callbacks a tiny bit easier to test, but the difference is quite minimal. |
…arer and errorSetter
@marten-seemann I pushed up a take on what I described. See what you think. Happy to adjust or go back to callbacks. |
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.
This is great! nice work @GeorgeMac. It simplifies the logic. A nice improvement to this part of the code.
Thank you @MarcoPolo 🙏 |
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.
Thank you @GeorgeMac!
Thanks for the great feedback! |
Fixes #4513
This updates the
onStreamStateChange
to handle thestreamStateSendAndReceiveClosed
state.Prior to this, when the client failed to observe this, the datagram previously tracked in the streams map was left behind. This caused the memory leak observed in the issue above. Adding this case causes the entry to be deleted and memory in our project is stable.