[release/1.x] Release SSE response stream reference when GET request ends#1628
Open
halter73 wants to merge 2 commits into
Open
[release/1.x] Release SSE response stream reference when GET request ends#1628halter73 wants to merge 2 commits into
halter73 wants to merge 2 commits into
Conversation
StreamableHttpServerTransport held a reference to the Kestrel SSE response stream via _httpSseWriter for the entire session lifetime. Since the transport is only disposed when the session itself is disposed (via explicit DELETE or idle timeout), clients that disconnect without sending DELETE — which is typical for long-lived SSE connections — left the response stream pinned for the remainder of the idle timeout window. Because the Kestrel HTTP connection (and its associated memory pool buffers, Pipe readers/writers, socket send/receive buffers) is only collectible once all references to the response stream are released, this pinned ~20MiB of unmanaged memory per disconnected session until cleanup. Under steady connect/disconnect churn (e.g., IDE availability probes) this accumulated into sustained memory growth that eventually OOMKilled the container. Fix: release _httpSseWriter from within HandleGetRequestAsync's finally block so the reference is dropped as soon as the GET request exits, not just when the session is disposed. SendMessageAsync handles the now-nullable field via the existing _getHttpResponseCompleted gate, and any server-to-client messages sent after disconnect are still captured by the event store writer for replay via Last-Event-ID. Add a unit test that verifies _httpSseWriter is null once HandleGetRequestAsync returns. Relates to #766.
…ed; rewrite test - Remove _getHttpResponseCompleted and rely on _httpSseWriter being null as the signal that the GET response stream has been released. Null out _httpSseWriter in DisposeAsync so SendMessageAsync correctly skips writing post-dispose. - Replace the dispose-idempotency role of the old flag with a dedicated _disposed bool. - Move the explanatory comment in SendMessageAsync onto the event store branch (which intentionally still runs when the response stream is gone, to support Last-Event-ID replay). - Rewrite the unit test to assert observable behavior via a recording Stream instead of reflecting on private fields. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
Backport of #1519 to
release/1.x.Fixes a memory leak in
StreamableHttpServerTransportwhere the SSE response stream reference (and its associated Kestrel connection / memory pool buffers, ~20MiB per session) was retained until the session was disposed via explicit DELETE or idle timeout. Long-lived SSE clients that disconnect without sending DELETE would accumulate significant unmanaged memory.Includes both the original fix and the review fixup that replaces the redundant
_getHttpResponseCompletedflag with_disposedand rewrites the new unit test to assert observable behavior instead of using reflection.Original author: @joelmforsyth
/cc @jeffhandley