GO_AWAY support with graceful shutdown #117
Conversation
…gration signals internally
| subscriber | ||
| .iter_mut() | ||
| .for_each(|s| s.handle_go_away().unwrap_or(())); | ||
| } |
There was a problem hiding this comment.
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.
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.
run_send independently 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.
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"
This is correct. To unsubscribe from all tracks the current design have one way, which is dropping the Subscribed optimistically 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.
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?
Even I want to keep the Subscribed state 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.
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.
2 participants
No description provided.