Skip to content

fix: add logging to empty exception handlers in _base_client.py - #3565

Open
Sahith59 wants to merge 1 commit into
openai:mainfrom
Sahith59:fix-empty-exception-handler
Open

fix: add logging to empty exception handlers in _base_client.py#3565
Sahith59 wants to merge 1 commit into
openai:mainfrom
Sahith59:fix-empty-exception-handler

Conversation

@Sahith59

@Sahith59 Sahith59 commented Aug 1, 2026

Copy link
Copy Markdown

Fixes #3428

Found an empty exception handler except: pass pattern in _base_client.py. Empty exception handlers silently swallow errors, which can make debugging very difficult.

This PR replaces pass with a debug log message when closing the client connections.

@Sahith59
Sahith59 requested a review from a team as a code owner August 1, 2026 22:36

@jbeckwith-oai jbeckwith-oai 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.

Blocking: these handlers are inside __del__ finalizers, not ordinary operational code, and the swallowed exceptions include expected teardown control flow. In particular, AsyncHttpxClientWrapper.__del__ calls asyncio.get_running_loop(), which normally raises when GC/interpreter shutdown runs outside an active event loop. This change turns that routine path into a full traceback whenever SDK debug logging is enabled. Reproduction on this head:

OPENAI_LOG=debug PYTHONPATH=src python -c "from openai import AsyncOpenAI; AsyncOpenAI(api_key=\"test\")"

It emits Failed to close async client plus RuntimeError: no running event loop during otherwise successful process exit. Logging from a finalizer is also not teardown-safe, and exc_info=True introduces a new disclosure surface because SensitiveHeadersFilter only sanitizes structured header arguments—not arbitrary transport/proxy exception text and tracebacks.

Please keep this best-effort fallback silent and document why, or at minimum distinguish the expected no-loop/finalizing cases and emit only a sanitized message when logging is known to be safe. Add focused lifecycle tests for normal GC outside a loop, an actual close failure, and interpreter teardown. Explicit close() / aclose() remains the reliable path where callers can observe cleanup failures.

Validation on exact head: Ruff passed and tests/test_client.py passed (198 passed, 2 skipped); the problem is the newly introduced finalizer/logging behavior, not a syntax or test regression.

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.

Empty exception handler in _base_client.py

2 participants