Performance optimization-- cache the encoding step - #2
Closed
jkodumal wants to merge 2 commits into
Closed
Conversation
|
lgtm |
Author
|
Unclear that we need this. |
kinyoklion
added a commit
that referenced
this pull request
Jul 28, 2026
…eplay (#63) ## 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: ```go 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.
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
2 participants
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.
If there are many subscribers for an event, it may be more efficient to pre-compute the encoding.