fix: Avoid panic in ServerSentEventSender by keeping handler alive
#4821
Conversation
The goroutine launched by `ServerSentEventSender` can perform a write and flush after the calling http handler has exited, at this point the resources (e.g. `http.ResponseWriter`) are no longer safe to use. To work around this issue, heartbeats and sending events are now handled by the goroutine which signals its closure via a channel. This allows the calling handler to ensure it is kept alive until it's safe to exit. Fixes #4807
| case <-r.Context().Done(): | ||
| return |
There was a problem hiding this comment.
Why doesn't this prevent the write/flush after finish? We're not hijacking the connection so the context should still be cancelled on finish right?
There was a problem hiding this comment.
Because this code had the following race condition:
- Say
watchWorkspaceencountered an error and exited (this begins the process of all handlers and middleware exiting (r.Context()is not yet cancelled) - Timer is triggered
- We write to
rwwhich still succeeds as the teardown is still happening - Teardown finishes, context is cancelled, but we're currently not listening to this signal
- We hit
Flushafter teardown so we get a nil pointer deref
There was a problem hiding this comment.
Awesome explanation thank you
| @@ -174,8 +173,7 @@ func WebsocketCloseSprintf(format string, vars ...any) string { | |||
| return msg | |||
| } | |||
|
|
|||
| func ServerSentEventSender(rw http.ResponseWriter, r *http.Request) (func(ctx context.Context, sse codersdk.ServerSentEvent) error, error) { | |||
| var mu sync.Mutex | |||
| func ServerSentEventSender(rw http.ResponseWriter, r *http.Request) (sendEvent func(ctx context.Context, sse codersdk.ServerSentEvent) error, closed chan struct{}, err error) { | |||
There was a problem hiding this comment.
It seems like we can eliminate the error return from this as it's never set to a non-nil value
There was a problem hiding this comment.
Not really relevant for this PR, but that's a good point. I'd like to see it kept and remove the panic in favor of an error, though.
coderd/httpapi/httpapi.go
Outdated
| event := sseEvent{ | ||
| payload: buf.Bytes(), | ||
| errC: make(chan error, 1), | ||
| } |
There was a problem hiding this comment.
Did you have to move the writing to the goroutine to fix the bug or is this just a separate change you did? The old flow was simpler to understand as it didn't involve extra write channels and response channels, but if it helps to fix the bug then 👍
There was a problem hiding this comment.
It's not mandator per-se, but I think we'd want do it if we service-ified this implementation anyway. That said, asynchronous code will always introduce complexity somewhere, this change moves the core logic into one place which IMO simplifies it. It's easier to reason about where and in what order mutations/writes are happening.
This change also has the benefit of allowing us to listen to both r.Context() and ctx completion which we didn't before, which is more true to the given API (function signature).
There was a problem hiding this comment.
Oh yeah, forgot to mention that this logic change also stops writes after an error is encountered, allowing the caller (if errors are checked) to stop using the sender.
Previously we exited the goroutine on write error, meaning keepalives were no longer sent, so it makes sense to disable writes at this point.
There was a problem hiding this comment.
The asynchronous logic introduces channel reads which can result in the goroutine hanging waiting for data indefinitely. For example, if you wrote an event to the events channel and the sender goroutine got closed before reading that event, you would be permanently stuck waiting for a response from the response channel as far as I can tell.
I think this logic should be kept the way it was to avoid the complexities and extra bugs introduced by having it be asynchronous.
There was a problem hiding this comment.
wait nevermind I forgot it's an unbuffered channel
There was a problem hiding this comment.
case in point it's hard to reason with this logic since it's a chain of passing messages around when it could just be a simple write to writer. The goroutine could be refactored to use this same function for writing messages so the safety checks are all in the same place and duplication is reduced
There was a problem hiding this comment.
I don't see why that's a problem? The previous implementation was already asynchronous and required mutex locking and taking care not to use the resources after close. Here I've fixed those issues and ensured there are no hangs by listening on e.g. <-closed which is closed when there no longer is a listener for events. If you're considering a future developer making changes, I'd venture those changes would be "dangerous" in the previous implementation as well?
A motivation to keep this change is that AFAIK we'll be using SSEs more in the future, and we'll want to send different types of events on a single channel (due to browser limitations). At this point we'll need a service (sendEvent called from multiple places) and at that point the previous implementation of sendEvent becomes unsafe (same bug we're fixing here).
There was a problem hiding this comment.
If we're rewriting this soon like you say then I think this is fine. I still feel like a safe implementation can be achieved in a less complicated way and that we should investigate it in the future, but I won't block your PR for it since this is to fix a panic
There was a problem hiding this comment.
I honestly don't know when we're rewriting/refactoring this, @f0ssel may have some idea. Personally I don't feel channels are any more dangerous than mutexes (you have deadlocks and missing unlocks to think about there), but I guess this is in the eye of the beholder. I'll certainly consider your input if I'm the one rewriting this 👍🏻.
e16843c to
87a10aa
Compare
The goroutine launched by
ServerSentEventSendercan perform a writeand flush after the calling http handler has exited, at this point the
resources (e.g.
http.ResponseWriter) are no longer safe to use.To work around this issue, heartbeats and sending events are now handled
by the goroutine which signals its closure via a channel. This allows
the calling handler to ensure it is kept alive until it's safe to exit.
Fixes #4807
NOTE: This was a relatively quick fix, we may want to consider
rewriting the sender as a service to clean up the implementation.