-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Overload: Reset H2 server stream only use codec level reset mechanism #18895
Merged
Merged
Changes from 2 commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
3533da7
Use lower level reset mechanism for reset stream if saw Envoy level
KBaichoo 22c6521
s/sawEnvoyStreamComplete/envoyStreamComplete and make interface const.
KBaichoo 8008823
Revert Changes to Account interface, if doing OM reset don't defer.
KBaichoo c8f7bdc
onEnvoyStreamComplete to delay H1 codec destruction until Envoy level
KBaichoo ff20da9
Merge remote-tracking branch 'upstream/main' into rst-low-level
KBaichoo b6211fc
Iterator stability if for mock stream since we can remove observer
KBaichoo 3d320f0
merge main.
KBaichoo 2dc1eff
Used local_end_stream_ for http1.
KBaichoo 0b98a9c
Use encode_complete_ instead of local_end_stream_ since the latter i…
KBaichoo 4bb2619
Merge remote-tracking branch 'upstream/main' into rst-low-level
KBaichoo c3fa8ab
Revert onEnvoyStreamComplete.
KBaichoo f4c4199
Update comments.
KBaichoo 189d9b2
Revert unneeded mock method.
KBaichoo 84c517d
Don't null response_encoder_.
KBaichoo b75d8ac
Merge remote-tracking branch 'upstream/main' into rst-low-level
KBaichoo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
After some more offline discussion I'd suggested using local_end_stream_ rather than adding yet another way to communicate a stream is done. @KBaichoo thinks that's sketchy because of interplay between local_end_stream_ and the deferred reset code.
Pulling in @yanavlasov and @antoniovicente to 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.
interplay between local_end_stream and
runResetCallbacks(reason)
I think can be sketchy (sorry if I didn't convey that well)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 change feels brittle.
I think we need to dig deeper and make object lifetimes more clear. We need to eliminate all cases where an object remains registered after deletion.
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.
yeah that's my thought too. @KBaichoo if we're in a hurry to get this landed I'd be OK merging with a TODO to fix if you can help resolve the real lifetime issues right after, but if we're not in a rush I think we want to sort out the lifetime issues now in a less fragile way.
/wait
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.
Thanks for the pushback. I've dug deeper and have done the following:
doDeferredStreamDestroy
access the response encoder (codec stream) and remove the envoy level stream from callbacks, and notify the codec level stream it's done.