fix: Close streaming connection pool on FDv1 data source shutdown#456
Draft
jsonbailey wants to merge 2 commits into
Draft
fix: Close streaming connection pool on FDv1 data source shutdown#456jsonbailey wants to merge 2 commits into
jsonbailey wants to merge 2 commits into
Conversation
The legacy FDv1 StreamingUpdateProcessor passed its own urllib3 PoolManager to the eventsource SSEClient but never retained or closed it. SSEClient.close() only releases the active connection back to the pool and does not close a caller-supplied pool. Under urllib3 >= 1.26.16 / 2.x, PoolManager.clear() no longer closes sockets synchronously (teardown deferred to GC), so the streaming socket lingered ESTABLISHED after LDClient.close(). Retain the pool and force-close it on shutdown, mirroring the FDv2 data source.
joker23
approved these changes
Jul 10, 2026
Aligns FDv1 and FDv2 to log pool-close failures at warning (with traceback) rather than debug, so a failure to deterministically close the streaming socket is visible in production.
Contributor
Author
|
Putting this in draft as I do some more testing and don't want this to get merged by accident. |
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.
Summary
The legacy FDv1 streaming data source now retains and force-closes its urllib3
PoolManageron shutdown, so the streaming TCP socket is torn down promptly instead of lingering until GC. This mirrors the fix already present in the FDv2 data source (ldclient/impl/datasourcev2/streaming.py).Root cause
StreamingUpdateProcessor(ldclient/impl/datasource/streaming.py) created its ownPoolManager, passed it to the eventsourceSSEClient, but never retained or closed it.SSEClient.close()only releases the active connection back to the pool and does not close a caller-supplied pool (eventsource only closes pools it created — correct ownership). Under urllib3 >= 1.26.16 / 2.x,PoolManager.clear()no longer closes sockets synchronously (teardown is deferred toweakref.finalizeon GC), so the streaming socket lingered ESTABLISHED afterLDClient.close().Fix
self._sse_poolwhen building theSSEClient._close_pool_manager(pool)helper that iteratespool.pools, closes eachHTTPConnectionPool, then callspool.clear()— wrapped in try/except with debug logging. Guarded onpool is Noneand idempotent, so it is safe to call from both the run thread and a stopping thread.self._sse.close()in bothrun()(tail) and__stop_with_error_info().This mirrors the FDv2 data source's approach. A small local copy of the helper was chosen over importing from the FDv2 module to avoid backward coupling (legacy depending on v2); the helper is ~15 lines.
Test
Added
test_stop_closes_underlying_pooltoldclient/testing/impl/datasource/test_streaming.py, modeled on the FDv2test_streaming_closes_underlying_pool_on_fallback. It injects a tracking pool + fake SSE client, callsstop(), and asserts the SSE client is closed, the pool is cleared, and the underlying connection pool is closed.Notes
Should ship bundled with the launchdarkly-eventsource 1.7.1 bump (the SDK already pins
>=1.7.1onmain).Closes SDK-2668
Note
Low Risk
Localized shutdown/cleanup change with idempotent pool teardown and unit test coverage; no auth, flag evaluation, or data-path changes.
Overview
FDv1
StreamingUpdateProcessornow keeps the urllib3PoolManagerused for SSE and force-closes it on shutdown, so the streaming TCP socket is torn down when the client stops instead of staying open until GC (urllib3 ≥ 1.26.16 / 2.x behavior whenSSEClient.close()only returns the connection to the pool).A local
_close_pool_managercloses eachHTTPConnectionPoolin the manager thenclear()s it; it runs afterSSEClient.close()in both therun()loop exit andstop()/__stop_with_error_info(), and is safe to call from either thread. FDv2 streaming only bumps pool-close failure logs from debug to warning to match v1.Adds
test_stop_closes_underlying_poolto assertstop()closes the fake SSE client, clears the pool, and closes the underlying connection pool.Reviewed by Cursor Bugbot for commit f45e997. Bugbot is set up for automated code reviews on this repo. Configure here.