-
Notifications
You must be signed in to change notification settings - Fork 27
GO_AWAY support with graceful shutdown #117
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
Open
itzmanish
wants to merge
2
commits into
cloudflare:main
Choose a base branch
from
itzmanish:feat/go-away
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
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 hidden or 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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
Oops, something went wrong.
Oops, something went wrong.
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.
It's not clear to me exactly what's supposed to be happening here or why this is being handled the point where we typically just log the outgoing message and then encode it for the wire. It looks like this handler is supposed to do some subscription cleanup, but I'm not sure this is where we really want to be doing it.
I'm reading this as saying "whenever we are just about to put a GOAWAY on the wire, first unsubscribe from all tracks we're subscribed to, and then put it on the wire"
I don't read the spec as saying that GOAWAY itself should impact subscription state. I can see a case for wanting to ensure a subscriber client unsubscribes from all active subscriptions prior to sending GOAWAY, but this seems like a very intrusive and awkward way to enforce that. I wonder if it might be better to error if a client tries to send a GOAWAY while it still has open subscriptions?
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.
run_sendindependently runs in seperate thread which means we can know if server is sending a go_away only by either intercepting the message or take signal_rx in the parameters.This is correct. To unsubscribe from all tracks the current design have one way, which is dropping the
Subscribedoptimistically which then sends unsubscribe to the downstream and then send go_away. The client makes sure before dropping the connection that publish_done is responded for all unsubscribe message.Even I want to keep the
Subscribedstate and just put a flag that it's closed and send unsubscribe and when it gets publish done only then the state gets cleared but this requires changing a lot of code. In fact the client side code always first send unsubscribe and then when gets publish_done it closes the state.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, I think for this initial implementation it would be better to punt on strictly enforcing the unsubscribes at this layer and just make it possible to send GOAWAY without any surprising side effects. It'd be nice to provide guardrails for applications, but it sounds like doing that correctly would be a lot more work right now. I'm OK saying that applications should ultimately be responsible for correctly managing their session state (including unsubscribing from tracks before sending GOAWAY). Given that we're already talking about session teardown in this case the worst that can happen is that an application will do something wrong and end up accidentally ending a session with a PROTOCOL_VIOLATION instead.