Repository navigation
fix: Close the response body when a stream stops - #114
Merged
Merged
Conversation
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.
kinyoklion
commented
Oct 12, 2026
|
|
||
| sleepNotifier.notify(); // in case we're sleeping in a reconnect delay, wake us up | ||
| } | ||
| if (oldReadingThread == Thread.currentThread()) { |
Member
Author
There was a problem hiding this comment.
This is outside the synchronized block because it closing the connection is a potentially blocking call.
kinyoklion
marked this pull request as ready for review
October 12, 2026 17:21
tanderson-ld
approved these changes
Oct 13, 2026
kinyoklion
pushed a commit
that referenced
this pull request
Oct 13, 2026
🤖 I have created a release *beep* *boop* --- ## [5.0.1](5.0.0...5.0.1) (2026-10-13) ### Bug Fixes * Close the response body when a stream stops ([#114](#114)) ([0cf16fe](0cf16fe)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > **Release 5.0.1** bumps the project version from **5.0.0** to **5.0.1** in `gradle.properties` and `.release-please-manifest.json`, and adds the **5.0.1** section to `CHANGELOG.md`. > > The release notes document a **bug fix** ([#114](#114)): the HTTP **response body is closed when the SSE stream stops**, addressing a resource-leak path in long-lived connections. This PR does not include application code changes—only the Release Please version and changelog updates for that already-merged fix. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 284c6d0. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
kinyoklion
added a commit
to launchdarkly/java-core
that referenced
this pull request
Oct 13, 2026
## Summary After a stream read timeout on an HTTP/2 connection, OkHttp keeps the connection in the pool and treats it as healthy until a 1 second ping deadline passes. After a long-lived stream fails, the first reconnect comes 0.5 to 1 second later. So when the network silently drops the stream connection, the reconnect opens a new stream on the same dead connection and waits for another 5 minute read timeout. Recovery takes about 10 minutes instead of 5. The stream's OkHttp client now uses a connection pool that keeps no idle connections, so each reconnect opens a new connection. This applies to the FDv1 streaming data source and the FDv2 streaming synchronizer. The stream client has its own pool, so polling and event delivery are unchanged. This also updates okhttp-eventsource to 5.0.1, which closes the response body when a stream stops (launchdarkly/okhttp-eventsource#114). Without that, a restart that the SDK starts itself, such as after an FDv2 `goodbye` or invalid data, leaves the abandoned HTTP/2 call holding the old connection, so this pool setting cannot close it. New tests count TCP connections across a reconnect and fail without this change. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Overview** > Fixes slow recovery when an SSE stream dies on HTTP/2 by forcing **each stream reconnect to use a fresh TCP connection** instead of reusing a pooled connection OkHttp may still treat as healthy for ~1s after a read timeout. > > **StreamProcessor** (FDv1) and **StreamingSynchronizerImpl** (FDv2) now configure the eventsource OkHttp client with `ConnectionPool(0, 1, SECONDS)` **after** `httpProperties` applies its default pool, so idle connections are not kept for streaming only. Polling and other HTTP clients are unchanged. > > Bumps **okhttp-eventsource** to `5.0.1` (pairs with upstream fix for abandoned HTTP/2 calls on SDK-initiated restarts). Adds integration tests that count TCP connections across a reconnect and expect two connections. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 332fbe0. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
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.
Summary
EventSourcestops a stream by cancelling the OkHttp call, but when the stream stops on the reading thread (for example,interrupt()called inside theanyEvents()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
ConnectStrategyimplementations now also see their input stream closed when a stream stops; thegetInputStream()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).
EventSourcenow tracks the connectInputStreamand, on the reading thread only, runsdiscardEventParser()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
mockwebserverfor tests:EventSourceStreamReleaseTestasserts closer-then-close ordering acrossinterrupt/stop/close(same or other thread), andHttpConnectStrategyHttp2Testverifies 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.