Skip to content

Commit 955d36d

Browse files
committed
fix: serialize NO_PROXY sanitization across concurrent client constructions
Addresses Codex P2: when two SDK default clients are constructed concurrently while NO_PROXY contains a newline, one call can enter the context after another already sanitized the process-wide env, record no original value, and then the first call restores the invalid value before the second call's super().__init__() reaches httpx's env proxy parsing. That leaves the second client exposed to the same InvalidURL this change is trying to prevent. Added a module-level threading.Lock (_no_proxy_sanitizer_lock) that wraps the entire sanitize-construct-restore window in _sanitized_no_proxy, so concurrent client constructions are serialized and each call sees a consistent environment. Added test_concurrent_client_construction_serializes_sanitization that spawns 10 threads constructing clients with a newline NO_PROXY and verifies no errors and the original value is restored.
1 parent 332dd5d commit 955d36d

2 files changed

Lines changed: 58 additions & 14 deletions

File tree

‎src/openai/_base_client.py‎

Lines changed: 23 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
import logging
1212
import platform
1313
import warnings
14+
import threading
1415
import contextlib
1516
import email.utils
1617
from types import TracebackType
@@ -833,6 +834,9 @@ def _idempotency_key(self) -> str:
833834
return f"stainless-python-retry-{uuid.uuid4()}"
834835

835836

837+
_no_proxy_sanitizer_lock = threading.Lock()
838+
839+
836840
@contextlib.contextmanager
837841
def _sanitized_no_proxy() -> Iterator[None]:
838842
"""Temporarily normalize line separators in NO_PROXY/no_proxy for the
@@ -846,21 +850,26 @@ def _sanitized_no_proxy() -> Iterator[None]:
846850
sanitized value to be visible for that window and then restore the original
847851
afterwards — this avoids permanently mutating process-global state for
848852
unrelated clients.
853+
854+
A module-level lock serializes concurrent client constructions so that one
855+
call cannot restore the original (invalid) value while another call's
856+
``super().__init__()`` is still reading the environment.
849857
"""
850-
originals: dict[str, str] = {}
851-
try:
852-
for key in ("NO_PROXY", "no_proxy"):
853-
val = os.environ.get(key)
854-
if val and any(c in val for c in "\n\r"):
855-
originals[key] = val
856-
# splitlines() handles \n, \r, \r\n, and other Unicode line
857-
# separators uniformly.
858-
parts = [part.strip() for part in val.splitlines()]
859-
os.environ[key] = ",".join(p for p in parts if p)
860-
yield
861-
finally:
862-
for key, val in originals.items():
863-
os.environ[key] = val
858+
with _no_proxy_sanitizer_lock:
859+
originals: dict[str, str] = {}
860+
try:
861+
for key in ("NO_PROXY", "no_proxy"):
862+
val = os.environ.get(key)
863+
if val and any(c in val for c in "\n\r"):
864+
originals[key] = val
865+
# splitlines() handles \n, \r, \r\n, and other Unicode line
866+
# separators uniformly.
867+
parts = [part.strip() for part in val.splitlines()]
868+
os.environ[key] = ",".join(p for p in parts if p)
869+
yield
870+
finally:
871+
for key, val in originals.items():
872+
os.environ[key] = val
864873

865874

866875
class _DefaultHttpxClient(httpx.Client):

‎tests/test_no_proxy_sanitize.py‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@
77

88
from __future__ import annotations
99

10+
import os
11+
1012
import pytest
1113

1214

@@ -197,3 +199,36 @@ def test_aiohttp_client_trust_env_false_skips_sanitization(monkeypatch: pytest.M
197199

198200
assert os.environ.get("NO_PROXY") == "localhost\n127.0.0.1"
199201
assert client._mounts == {}
202+
203+
204+
def test_concurrent_client_construction_serializes_sanitization(monkeypatch: pytest.MonkeyPatch) -> None:
205+
"""Concurrent client constructions must not race on the env mutation.
206+
207+
Without the lock, one call could restore the original (invalid) NO_PROXY
208+
value while another call's ``super().__init__()`` is still reading the
209+
environment, exposing the second client to InvalidURL. The lock
210+
serializes the sanitize-construct-restore window so each call sees a
211+
consistent environment.
212+
"""
213+
import threading
214+
215+
from openai._base_client import _DefaultHttpxClient
216+
217+
_set_no_proxy(monkeypatch, "localhost\n127.0.0.1")
218+
errors: list[Exception] = []
219+
220+
def construct() -> None:
221+
try:
222+
_DefaultHttpxClient()
223+
except Exception as exc:
224+
errors.append(exc)
225+
226+
threads = [threading.Thread(target=construct) for _ in range(10)]
227+
for t in threads:
228+
t.start()
229+
for t in threads:
230+
t.join()
231+
232+
assert not errors, f"Concurrent constructions failed: {errors}"
233+
# The original (invalid) value must be restored after all constructions
234+
assert os.environ.get("NO_PROXY") == "localhost\n127.0.0.1"

0 commit comments

Comments
 (0)