fix: unblock Repository producers when a subscriber disconnects mid-replay#63
Draft
kinyoklion wants to merge 3 commits into
Draft
fix: unblock Repository producers when a subscriber disconnects mid-replay#63kinyoklion wants to merge 3 commits into
kinyoklion wants to merge 3 commits into
Conversation
…eplay When a Repository is registered for a channel, the server calls Replay on each new subscription and drains the returned channel from the per-connection handler goroutine. If the subscriber disconnects while the handler is still consuming that batch, the handler breaks out of its read loop and abandons the channel without draining it -- and it cannot close it, because it holds the receiving end. Replay itself has no cancellation hook (two strings in, a bare channel out), so the producer goroutine blocks forever on its next `out <- event` send, leaking the goroutine and its payload until the process exits. This fixes it at two levels: 1. Background drain (no API change, works for every Repository): when the handler exits its read loop while still mid-batch, it drains the remaining events in a throwaway goroutine so the producer unblocks as fast as it can emit and releases in milliseconds. 2. RepositoryWithContext (optional extension): a Repository may implement ReplayWithContext(ctx, channel, id), which the server prefers when present, passing the subscribing request's context. That context is cancelled on disconnect, so an adopting producer can select on ctx.Done() and stop promptly and cleanly. Repositories that implement only Replay are unaffected and continue to be served via the drain safety net. Replay remains for backward compatibility; the new interface embeds Repository and is selected by type assertion, so no existing consumer or exported API breaks. Tests reproduce the leak (a producer blocked on send is stranded on disconnect) and verify it is fixed for both the plain-Replay drain path and the ReplayWithContext path, across clean FIN and abrupt RST, plus normal delivery, context-cancellation observation, method preference, empty batches, multiple concurrent subscriptions, and server close during replay. Full suite passes under -race.
…t helper Factor the background drain into drainReplayedEvents, and also drain the batch channel in Server.run() when the initial trySend to a subscriber fails (the subscriber was already closed and will never consume the batch, so its producer would otherwise block forever). This broadens the safety net beyond the mid-batch-consume case already covered after the handler's read loop.
Match the existing channel-drain convention (stream.go) with a //nolint:revive directive; golangci-lint run is clean.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Problem
eventsourceis an SSE server. When aRepositoryis registered for a channel,Server.run()callsrepo.Replay(channel, lastEventID) <-chan Eventon each new subscription, wraps the returned channel as aneventBatch, and hands it to the per-connection HTTP handler goroutine, which drains it and writes each event to the client.The handler detects client disconnect immediately via
req.Context().Done()(both a clean FIN and an abrupt reset fire it). But on disconnect it doesbreak ReadLoopand abandons the batch channel without draining it — and it cannot close it, because it holds the receiving end.Repository.Replayhas no context/cancellation hook (two strings in, a bare channel out), so the producer goroutine insideReplayblocks forever on its nextout <- event: a clean shutdown and a dirty one are indistinguishable to it, because "my send never completes" is all the API exposes.Consequence: a mid-replay disconnect strands the
Replayproducer goroutine and its payload until the process exits. (Once events are flowing, disconnects are already caught on the write path viaenc.Encodeerrors — only the initial producer to handler hop is blind.)Fix — two levels
1. Background drain (no API change, covers every Repository). When the handler exits its read loop while still consuming a batch (
readBatchChnon-nil), it drains the remaining events in a throwaway goroutine (go func(){ for range ch {} }()). The producer unblocks as fast as it can emit and releases in milliseconds. This alone fixes the forever-leak for anyRepository, including third-party ones.2.
RepositoryWithContext(optional extension, clean propagation). ARepositorymay additionally implement:The
Servertype-asserts the registered repository for it and, when present, calls it with the subscription's request context, so the producer canselect { case out <- event: case <-ctx.Done(): return }and abort promptly and cleanly.Replayremains required (the new interface embedsRepository); repositories that implement onlyReplayare served exactly as before via the drain safety net.Together: #1 guarantees no producer blocks past the handler's exit even for repos that never adopt the context; #2 lets adopters stop immediately on disconnect.
Backward compatibility
No exported symbol changed or was removed.
Repository.Replayis unchanged;RepositoryWithContextis new and optional and is selected purely by type assertion.SliceRepositoryand any existing consumer continue to work unmodified.Tests
server_replay_disconnect_test.go:Replaydrain path and theReplayWithContextpath.SetLinger(0)) disconnects.ReplayWithContextobservesctx.Done()on disconnect, and is preferred overReplaywhen both are implemented.Full suite passes under
go test -race ./...;gofmtandgo vetclean.