Skip to content

fix: Close streaming connections deterministically where the platform allows#74

Merged
jsonbailey merged 8 commits into
mainfrom
jb/sdk-2668/eventsource-close-polyfill
Jul 22, 2026
Merged

fix: Close streaming connections deterministically where the platform allows#74
jsonbailey merged 8 commits into
mainfrom
jb/sdk-2668/eventsource-close-polyfill

Conversation

@jsonbailey

@jsonbailey jsonbailey commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

What

Fixes a streaming socket leak on both connection-teardown paths — explicit close()/interrupt() and automatic fault-driven reconnect. Previously the socket was released to the pool (release_conn) or the connection reference was simply dropped, so the socket lingered until GC rather than being closed.

The core difficulty: deterministically closing the socket requires first waking a reader that may be blocked mid-recv(), and the mechanism for that is platform- and version-specific:

  • POSIX + urllib3 ≥ 2.3 (which added HTTPResponse.shutdown()): resp.shutdown() (SHUT_RD) wakes the blocked reader, then resp.close() tears the socket down deterministically.
  • Windows, or urllib3 < 2.3 (no shutdown()): there is no public, non-deadlocking way to wake a blocked reader (shutdown() doesn't interrupt recv() on Windows; closing the socket doesn't reliably either, and going through resp.close()_fp.close() deadlocks on the reader's BufferedReader lock). So we fall back to release_conn() — the socket leaks until GC (the pre-existing baseline), but it never hangs.

So the per-connection closer is one predicate — "can we safely wake a blocked reader?":

if sys.platform != "win32" and hasattr(resp, "shutdown"):
    if resp.connection is not None:
        try: resp.shutdown()
        except Exception: ...
    resp.close()            # POSIX + urllib3>=2.3: deterministic close
else:
    resp.release_conn()     # Windows / old urllib3: leak, never hang

Also in this PR:

  • The reconnect path (sse_client.py _close_current_connection) now routes through the closer instead of dropping the connection reference (the common-case leak).
  • _HttpClientImpl.close() closes pooled connections before clear() (on urllib3 2.x, PoolManager.clear() alone does not FIN pooled sockets), for pools eventsource created.
  • A floor CI job (UV_RESOLUTION=lowest-direct, Linux + Windows, one Python) guards the minimum declared dependency versions — the regular jobs only run against the locked/latest versions, which is why the urllib3-1.26/2.0–2.2 deadlock corner would otherwise ship untested.

Why

Fixes the FDv1 streaming socket leak (SDK-2668). Net effect vs. the baseline leak: POSIX (urllib3 ≥ 2.3) is now deterministic on interrupt/reconnect/close; Windows and older urllib3 are equal to the baseline (leak, never hang). No breaking dependency change.

urllib3 support

Keeps urllib3 >= 1.26 (no floor bump). urllib3 without HTTPResponse.shutdown() (1.26, 2.0–2.2) transparently takes the release_conn fallback — the same path Windows takes — so those versions leak-but-never-hang rather than deadlocking.

Upstream

The Windows/old-urllib3 leak is only fully fixable upstream: HTTPResponse.shutdown() (added in urllib3#3527 for urllib3#2868) is POSIX-only. A follow-up urllib3 issue tracks making it platform-correct; when that lands, the fallback can be dropped and Windows becomes deterministic too.

Validation

  • Full Linux + Windows CI matrix green, plus both floor (lowest-direct) jobs — including urllib3 1.26 on Windows (both fallback conditions at once).
  • Local cross-version: urllib3 1.26.20 / 2.2.3 (the last version that deadlocked before this gate) / 2.3.0 / 2.7.0 all pass.
  • Concurrent-stop tests assert no hang on all platforms/versions, and reader-wakeup only where shutdown() is available (_CAN_WAKE_READER).

Make SSEClient.close()/interrupt() tear down the active streaming
connection deterministically, using a uniform closer gated on urllib3
capabilities:

- urllib3 2.x: resp.shutdown() wakes a reader blocked mid-read, then
  resp.close() sends the TCP FIN and releases the fd.
- urllib3 1.26.x: there is no resp.shutdown() and we do not reach into
  private socket attributes, so we fall back to resp.release_conn(). This
  never hangs, but the socket is not closed deterministically there --
  deterministic close requires urllib3 2.x (the fd is released via GC).

Revert PR #72's iterate-and-close of the pool in _HttpClientImpl.close()
back to a plain PoolManager.clear() for a self-created pool. The explicit
per-connection close() hung on urllib3 1.26.x when a reader was still
blocked mid-read on a connection; this fixes that regression. On 2.x the
active connection is already closed by the connection closer.

Also close the old ConnectionResult on a fault-driven reconnect in
_all_generator (both the normal-end and error paths), matching the async
client, so reconnects tear the previous connection down deterministically
rather than leaving it for garbage collection.

This corrects the pool-ownership/close work from 1.7.1 (SDK-2600) and
targets a 1.7.2 release.
The connection closer now unconditionally calls resp.shutdown() then resp.close() (2.x APIs) to deterministically close the streaming socket; the urllib3 floor is raised to >=2.0.0 and the 1.26 release_conn fallback and its version-gated tests are removed.
@jsonbailey
jsonbailey requested a review from a team as a code owner July 16, 2026 21:50
@jsonbailey jsonbailey changed the title fix: Close streaming connections deterministically on shutdown feat: Close streaming connections deterministically on shutdown and reconnect Jul 16, 2026
resp.shutdown() (SHUT_RD) wakes a blocked reader on POSIX but not Windows. Closing the connection's socket via the public resp.connection.close() wakes it on Windows too and bypasses the BufferedReader lock that resp.close() deadlocks on, using only public API and without touching the caller's pool.
Deterministic close (shutdown+close) only on POSIX with urllib3>=2.3 (which has HTTPResponse.shutdown); Windows and older urllib3 fall back to release_conn (leak, never hang). Keeps urllib3>=1.26 support, restores owned-pool connection close, and gates the reader-woke test assertion accordingly.
@jsonbailey jsonbailey changed the title feat: Close streaming connections deterministically on shutdown and reconnect fix: Close streaming connections deterministically where the platform allows Jul 22, 2026
Regular jobs run against locked/latest deps; this guards the minimum declared versions (notably urllib3 1.26, which lacks HTTPResponse.shutdown()) on both platforms. One Python, one lowest-direct resolution -- not a full version matrix.
Reuses the canonical test target (PYTEST_FLAGS incl. -W error::SyntaxWarning) instead of a hardcoded pytest invocation; UV_RESOLUTION=lowest-direct applies to both the uv sync and uv run pytest steps.
@jsonbailey
jsonbailey merged commit 90a7045 into main Jul 22, 2026
18 checks passed
@jsonbailey
jsonbailey deleted the jb/sdk-2668/eventsource-close-polyfill branch July 22, 2026 18:12
jsonbailey pushed a commit that referenced this pull request Jul 22, 2026
🤖 I have created a release *beep* *boop*
---


##
[1.7.2](1.7.1...1.7.2)
(2026-07-22)


### Bug Fixes

* Close streaming connections deterministically where the platform
allows
([#74](#74))
([90a7045](90a7045))

---
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]
> **Low Risk**
> Version and changelog-only release with no code changes in the diff.
> 
> **Overview**
> **Release 1.7.2** — version metadata is bumped from **1.7.1** to
**1.7.2** in `.release-please-manifest.json`,
`ld_eventsource/version.py`, and `pyproject.toml`.
> 
> `CHANGELOG.md` adds the **1.7.2** section documenting the bug fix from
[#74](#74):
closing streaming connections deterministically on platforms that
support it. No runtime or library code changes appear in this diff.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
993bd6b. 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>
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.

2 participants