Skip to content

fix: Close the response body when a stream stops - #114

Open
kinyoklion wants to merge 2 commits into
mainfrom
rlamb/close-response-on-abandon
Open

kinyoklion wants to merge 2 commits into
mainfrom
rlamb/close-response-on-abandon

Conversation

@kinyoklion

@kinyoklion kinyoklion commented Oct 9, 2026 •

Copy link
Copy Markdown
Member

Summary

EventSource stops a stream by cancelling the OkHttp call, but when the stream stops on the reading thread (for example, interrupt() called inside the anyEvents() loop), it never closes the response body. OkHttp requires the body to be closed. On HTTP/2, cancelling only resets the stream, so the call stays attached to the connection: the connection is never released, the reconnected stream is multiplexed onto it, and if that connection later dies silently, the next reconnect can reuse it and wait for another read timeout.

The reading thread now closes the input stream whenever it stops using a stream, after the connection closer has run. The close stays on the reading thread because the stream is not safe to close while another thread reads from it. Custom ConnectStrategy implementations now also see their input stream closed when a stream stops; the getInputStream() Javadoc says so.

New tests use real HTTP/2 connections (MockWebServer, added as a test dependency) and fail without this change.


Note

Overview
Fixes a leak where stopping an SSE stream cancelled the OkHttp request but left the response body open. On HTTP/2, that kept the connection tied to the active response and could break reconnect behavior (stale connections, read timeouts).

EventSource now tracks the connect InputStream and, on the reading thread only, runs discardEventParser() after the connection closer—clearing the parser and closing the stream so the HTTP response is released. The same path runs on errors and end-of-stream. ConnectStrategy.Result#getInputStream() Javadoc documents this contract for custom strategies.

Adds mockwebserver for tests: EventSourceStreamReleaseTest asserts closer-then-close ordering across interrupt/stop/close (same or other thread), and HttpConnectStrategyHttp2Test verifies the OkHttp connection pool goes idle and reconnects on a fresh connection when needed.

Reviewed by Cursor Bugbot for commit 3114c80. Bugbot is set up for automated code reviews on this repo. Configure here.

EventSource stopped a stream by cancelling the request, but it did not
close the response body when the stream stopped on the reading thread, for
example when interrupt() is called inside the anyEvents() loop. On HTTP/2,
cancelling only resets the stream, so the request stays attached to the
connection and the connection is never released.

The reading thread now closes the input stream whenever it stops using a
stream, after the connection closer has run.

sleepNotifier.notify(); // in case we're sleeping in a reconnect delay, wake us up
}
if (oldReadingThread == Thread.currentThread()) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is outside the synchronized block because it closing the connection is a potentially blocking call.

@kinyoklion
kinyoklion marked this pull request as ready for review October 12, 2026 17:21
@kinyoklion
kinyoklion requested a review from a team as a code owner October 12, 2026 17:21

This branch has not been deployed

No deployments
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