Skip to content

fix: best-effort aclose() of httpx AsyncClient instead of leaving transports to GC#72

Merged
acrogenesis merged 1 commit into
mainfrom
fix/async-client-leak-aclose
Jun 12, 2026
Merged

fix: best-effort aclose() of httpx AsyncClient instead of leaving transports to GC#72
acrogenesis merged 1 commit into
mainfrom
fix/async-client-leak-aclose

Conversation

@acrogenesis

Copy link
Copy Markdown
Member

Description

pygqlc orphaned httpx.AsyncClient instances on three paths, leaving their transports to Python's cyclic GC. The orphaned _SelectorTransport.__del__ -> _sock.close() finalizers then ran during GC sweeps on arbitrary threads — prod logs literally show Exception ignored while calling deallocator <_SelectorTransport.__del__> — which contributed to false TMPRL1101 deadlock detections in Temporal workers. valiot/python-tooling#151 mitigated the worker side with gc.freeze; this PR fixes the leak at its source.

The three leak sites

  1. _get_async_client — when it decided the cached client's event loop was closed, it did self._async_client = None without closing ("Intentionally don't try to close the old client…").
  2. async_execute retry — on "Event loop is closed" during post, it dropped self._async_client = None without close and recreated.
  3. _close() (called by __del__) — sync context; set _async_client = None with no close attempt even when a loop was running.

The fix (one helper, three call sites)

  • New private _drop_async_client(): swaps the client out and best-effort await client.aclose(), swallowing (and WARN-logging) failures when the client's original loop is genuinely gone. Used at sites 1 and 2, and by async_cleanup (which keeps its semantics but loses its duplicated probe logic).
  • _close() (site 3, sync): if this thread has a running loop, schedule aclose() on it via loop.call_soon_threadsafe(lambda: loop.create_task(client.aclose()))__del__ can run on any thread. No usable loop → GC fallback, exactly as before.

Bonus bug found while testing

The loop-closed "probe" called await self._async_client.get_timeout()a method that does not exist on httpx.AsyncClient. The resulting AttributeError matched the "has no attribute" branch, so:

  • _get_async_client recreated the client on every call (and leaked the old one every time — this is why the leak was so loud in prod), and
  • async_cleanup always took the "let GC handle it" branch and never closed anything.

With this PR, recreation now closes the old client first and async_cleanup actually closes. I deliberately did not touch the probe itself (client-reuse behavior is unchanged: still recreates per call, now leak-free); fixing reuse properly is a separate concern. Also removed the unused _close_async_client private method, superseded by the helper.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

New hermetic regression suite tests/pygqlc/gql_client/test_async_client_lifecycle.py (no real sockets, no sleeps; singleton-bypass fixture pattern from test_subscribe.py):

  • _get_async_client acloses the stale client when replacing it (loop-closed branch simulated)
  • async_execute's "Event loop is closed" retry acloses the stale client before retrying
  • _close() under a running loop schedules aclose() (real asyncio loop); without a loop it falls back silently
  • _drop_async_client swallows aclose() exceptions and still drops the client
  • async_cleanup actually closes a live client

Red-first verified: 4/6 fail against pre-fix code (the other 2 guard preserved fallback behavior). Full suite: 37 passed locally (live-API tests skip-error without env secrets, as usual). uvx ruff format --check clean; bandit (-s B101 -ll) exit 0; uv lock --upgrade run and uv.lock committed per check-outdated policy.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (CHANGELOG 3.8.1, version bumped in pygqlc/__version__.py)
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

🤖 Generated with Claude Code

…nsports to GC

Three paths dropped the async client without closing it, orphaning its
transports to the cyclic GC whose _SelectorTransport.__del__ socket
finalizers then ran on arbitrary threads (false TMPRL1101 deadlocks in
Temporal workers — valiot/python-tooling#151):

1. _get_async_client's loop-closed branch
2. async_execute's 'Event loop is closed' retry
3. _close() / __del__

A new _drop_async_client() helper awaits aclose() best-effort (swallowing
failures when the original loop is gone) and serves sites 1, 2 and
async_cleanup; _close() schedules aclose() on the running loop via
call_soon_threadsafe when one exists. Also fixes async_cleanup never
actually closing (its get_timeout() probe hit a nonexistent method) and
removes the unused _close_async_client.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@palantir-valiot palantir-valiot Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall Assessment

This PR fixes three leak paths for httpx.AsyncClient instances in GraphQLClient. The implementation introduces a _drop_async_client() helper for best-effort aclose() and updates _close() (sync, called by __del__) to schedule aclose() via call_soon_threadsafe when a running loop exists. New hermetic tests cover the three leak sites plus error swallowing. Version and changelog are updated per repo conventions. No blocking bugs found.

Findings

No actionable findings.

Notes

  • The broken get_timeout() probe in _get_async_client (line 1097) is deliberately left untouched per the PR description; client-reuse behavior is unchanged and the leak is now closed regardless.
  • The broad except Exception in _drop_async_client and _close is intentional for best-effort cleanup when the original loop is gone; the docstring explains the rationale.
  • Test coverage for the lifecycle fix is comprehensive and follows the repo's "hermetic, no sockets, no sleeps" pattern.

@acrogenesis acrogenesis merged commit cb88649 into main Jun 12, 2026
7 checks passed
@acrogenesis acrogenesis deleted the fix/async-client-leak-aclose branch June 12, 2026 04:28
palantir-valiot Bot pushed a commit that referenced this pull request Jun 12, 2026
…nsports to GC (#72)

Three paths dropped the async client without closing it, orphaning its
transports to the cyclic GC whose _SelectorTransport.__del__ socket
finalizers then ran on arbitrary threads (false TMPRL1101 deadlocks in
Temporal workers — valiot/python-tooling#151):

1. _get_async_client's loop-closed branch
2. async_execute's 'Event loop is closed' retry
3. _close() / __del__

A new _drop_async_client() helper awaits aclose() best-effort (swallowing
failures when the original loop is gone) and serves sites 1, 2 and
async_cleanup; _close() schedules aclose() on the running loop via
call_soon_threadsafe when one exists. Also fixes async_cleanup never
actually closing (its get_timeout() probe hit a nonexistent method) and
removes the unused _close_async_client.

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.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.

1 participant