fix: add logging to empty exception handlers in _base_client.py - #3565
fix: add logging to empty exception handlers in _base_client.py#3565Sahith59 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
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.
These handlers live inside __del__ finalizers, not regular call paths. Logging here is problematic for two reasons: 1. asyncio.get_running_loop() raises RuntimeError whenever __del__ runs outside an active event loop, which is the normal case during GC and interpreter shutdown. Logging that as an error is misleading. 2. exc_info=True dumps raw tracebacks that may contain transport/proxy exception text not sanitized by SensitiveHeadersFilter. Keeping pass and documenting why is the correct approach for finalizers.
c79c98c to
c702652
Compare
|
You're right Justin...!, good catch. I missed that these live inside del finalizers, not regular call paths. The async case is the obvious one: asyncio.get_running_loop() raises RuntimeError whenever the finalizer runs outside an active loop, which is basically every normal GC/shutdown scenario. Logging that with exc_info=True would spam error-looking output on clean exits and also leak traceback text that SensitiveHeadersFilter doesn't cover. Reverted both to pass and added short inline comments explaining the rationale so the next person reading this doesn't make the same mistake. Updated the branch and rebased on main. |
2 participants
Fixes #3428
Found an empty exception handler
except: passpattern in _base_client.py. Empty exception handlers silently swallow errors, which can make debugging very difficult.This PR replaces
passwith a debug log message when closing the client connections.