Skip to content

fix: unblock Repository producers when a subscriber disconnects mid-replay#63

Draft
kinyoklion wants to merge 3 commits into
mainfrom
rlamb/eventsource-replay-disconnect
Draft

fix: unblock Repository producers when a subscriber disconnects mid-replay#63
kinyoklion wants to merge 3 commits into
mainfrom
rlamb/eventsource-replay-disconnect

Conversation

@kinyoklion

Copy link
Copy Markdown
Member

Problem

eventsource is an SSE server. When a Repository is registered for a channel, Server.run() calls repo.Replay(channel, lastEventID) <-chan Event on each new subscription, wraps the returned channel as an eventBatch, 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 does break ReadLoop and abandons the batch channel without draining it — and it cannot close it, because it holds the receiving end. Repository.Replay has no context/cancellation hook (two strings in, a bare channel out), so the producer goroutine inside Replay blocks forever on its next out <- 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 Replay producer goroutine and its payload until the process exits. (Once events are flowing, disconnects are already caught on the write path via enc.Encode errors — 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 (readBatchCh non-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 any Repository, including third-party ones.

2. RepositoryWithContext (optional extension, clean propagation). A Repository may additionally implement:

ReplayWithContext(ctx context.Context, channel, id string) <-chan Event

The Server type-asserts the registered repository for it and, when present, calls it with the subscription's request context, so the producer can select { case out <- event: case <-ctx.Done(): return } and abort promptly and cleanly. Replay remains required (the new interface embeds Repository); repositories that implement only Replay are 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.Replay is unchanged; RepositoryWithContext is new and optional and is selected purely by type assertion. SliceRepository and any existing consumer continue to work unmodified.

Tests

server_replay_disconnect_test.go:

  • Repro/regression: a producer blocked on a channel send is stranded on disconnect before the fix; after the fix it unblocks well under the deadline — verified for both the plain-Replay drain path and the ReplayWithContext path.
  • Clean FIN and abrupt RST (SetLinger(0)) disconnects.
  • Normal in-order delivery still works (plain and context repos) when the subscriber stays connected.
  • ReplayWithContext observes ctx.Done() on disconnect, and is preferred over Replay when both are implemented.
  • Empty batch (nil channel and already-closed channel) does not hang and normal publishing still works afterward.
  • Multiple concurrent subscriptions all unblock.
  • Server close during replay does not strand the producer.

Full suite passes under go test -race ./...; gofmt and go vet clean.

…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant