Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,16 @@
from __future__ import annotations

import datetime
import http.client
import json
import re
import socket
import threading
import time
import urllib.error
import urllib.request
from typing import Any
from urllib.parse import urlsplit


_SQL_IDENTIFIER = re.compile(r"^[a-zA-Z_][a-zA-Z0-9_]*$")
Expand All @@ -36,7 +41,179 @@ def redirect_request( # type: ignore[override] # stdlib signature has no hints
return None


_OPENER = urllib.request.build_opener(_NoRedirect())
class _Deadline:
"""A whole-request time budget shared by the timer and the connection.

``expired()`` is judged by the monotonic clock, so a response that lands
after the budget is never accepted even if the timer callback runs late.
The timer's job is only to wake a blocked socket.
"""

def __init__(self, seconds: float) -> None:
self.seconds = seconds
self.expires_at = time.monotonic() + seconds
self._fired = threading.Event()
self._lock = threading.Lock()
self._sockets: list[socket.socket] = []

def expired(self) -> bool:
return self._fired.is_set() or time.monotonic() >= self.expires_at

def remaining(self) -> float:
# Never hand the socket layer zero or a negative value: those mean
# non-blocking / blocking, not "no time left".
return max(self.expires_at - time.monotonic(), 0.001)

def register(self, sock: socket.socket) -> None:
"""Make ``sock`` reachable by ``fire`` (an in-flight connect or the live socket)."""
with self._lock:
self._sockets.append(sock)
already_fired = self._fired.is_set()
if already_fired:
_shutdown(sock)

def fire(self) -> None:
self._fired.set()
with self._lock:
sockets = list(self._sockets)
for sock in sockets:
_shutdown(sock)


def _shutdown(sock: socket.socket) -> None:
try:
sock.shutdown(socket.SHUT_RDWR)
except OSError:
pass


def _connect_within(
deadline: _Deadline,
address: tuple[str, int],
timeout: float | None,
source_address: tuple[str, int] | None = None,
) -> socket.socket:
"""``socket.create_connection`` with the remaining budget per attempt.

Every candidate socket is registered with the deadline before connecting,
so the timer can abort an attempt that is still waiting for the peer. Name
resolution itself cannot be interrupted.
"""
del timeout # the deadline, not the connection's static timeout, rules here
host, port = address
if deadline.expired():
msg = "deadline expired before connecting"
raise TimeoutError(msg)
last_error: OSError | None = None
for family, kind, proto, _, sockaddr in socket.getaddrinfo(
host, port, 0, socket.SOCK_STREAM
):
if deadline.expired():
msg = "deadline expired while connecting"
raise TimeoutError(msg)
sock = socket.socket(family, kind, proto)
try:
sock.settimeout(deadline.remaining())
if source_address:
sock.bind(source_address)
deadline.register(sock)
sock.connect(sockaddr)
except OSError as exc:
sock.close()
last_error = exc
continue
return sock
if last_error is not None:
raise last_error
msg = f"getaddrinfo returned no addresses for {host!r}"
raise OSError(msg)


def _bind_deadline(
conn: http.client.HTTPConnection, deadline: _Deadline | None
) -> None:
# ``_create_connection`` is the connection's socket factory; swapping it
# keeps the stdlib connect (TLS wrapping, ALPN, tunnelling) intact while
# every TCP attempt is budgeted and interruptible.
if deadline is not None and hasattr(conn, "_create_connection"):
conn._create_connection = ( # type: ignore[attr-defined] # stdlib hook
lambda address, timeout=None, source_address=None: _connect_within(
deadline, address, timeout, source_address
)
)


def _after_connect(
conn: http.client.HTTPConnection, deadline: _Deadline | None
) -> None:
if deadline is None:
return
if conn.sock is not None:
deadline.register(conn.sock) # the (possibly TLS-wrapped) live socket
if deadline.expired():
conn.close()
msg = "deadline expired while connecting"
raise TimeoutError(msg)


class _HTTPConnection(http.client.HTTPConnection):
def __init__(self, *args: Any, deadline: _Deadline | None, **kwargs: Any) -> None:
super().__init__(*args, **kwargs)
self._deadline = deadline
_bind_deadline(self, deadline)

def connect(self) -> None:
super().connect()

This comment was marked as outdated.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 886e893. Each TCP attempt now gets only the remaining budget and is registered with the deadline before connecting, so the timer aborts a stalled attempt at the deadline rather than after the per-address socket timeout. test_deadline_interrupts_a_stalled_connect asserts the elapsed time (0.32 s on a 300 ms deadline).

_after_connect(self, self._deadline)


class _HTTPSConnection(http.client.HTTPSConnection):
def __init__(self, *args: Any, deadline: _Deadline | None, **kwargs: Any) -> None:
super().__init__(*args, **kwargs)
self._deadline = deadline
_bind_deadline(self, deadline)

def connect(self) -> None:
super().connect()

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.

Codex AI review · Finding arf_v1_qv4pswss7gpvxjidl2ga6gprg3

[P2] Make the TLS handshake interruptible

super().connect() calls SSLContext.wrap_socket(..., do_handshake_on_connect=True). CPython detaches the registered raw socket before starting that blocking handshake, while _after_connect() cannot register the SSLSocket until it returns. If TCP or proxy setup used most of the budget, expiry shuts only a detached socket and TLS may wait for the old per-socket timeout. Wrap with automatic handshaking disabled, register the wrapped socket and set its remaining timeout, then call do_handshake(); cover this with a stalled HTTPS-handshake test.

_after_connect(self, self._deadline)


class _HTTPHandler(urllib.request.HTTPHandler):
def __init__(self, deadline: _Deadline | None) -> None:
super().__init__()
self._deadline = deadline

def http_open(self, req: urllib.request.Request) -> http.client.HTTPResponse:
deadline = self._deadline

def factory(*args: Any, **kwargs: Any) -> _HTTPConnection:
return _HTTPConnection(*args, deadline=deadline, **kwargs)

return self.do_open(factory, req) # type: ignore[arg-type] # factory, not class


class _HTTPSHandler(urllib.request.HTTPSHandler):
def __init__(self, deadline: _Deadline | None) -> None:
super().__init__()
self._deadline = deadline

def https_open(self, req: urllib.request.Request) -> http.client.HTTPResponse:
deadline = self._deadline

def factory(*args: Any, **kwargs: Any) -> _HTTPSConnection:
return _HTTPSConnection(*args, deadline=deadline, **kwargs)

# No context is configured on this handler, so the connection builds the
# stdlib default (certificate verification, ALPN http/1.1).
return self.do_open(factory, req) # type: ignore[arg-type] # factory, not class


def _opener(deadline: _Deadline | None) -> urllib.request.OpenerDirector:
# build_opener keeps the default handlers (proxy discovery, header merging,
# IPv6 hosts, error handling) and swaps in ours where classes overlap.
return urllib.request.build_opener(
_NoRedirect(), _HTTPHandler(deadline), _HTTPSHandler(deadline)
)


def compact_dumps(value: Any) -> str:
Expand Down Expand Up @@ -89,17 +266,53 @@ def http_send(
message. Redirects are not followed: a 3xx is returned like any other
failure. ``error_text`` is the first ``_MAX_ERROR_BODY_BYTES`` of a non-2xx
response body and empty on success; a success body is never read. Network
errors and timeouts propagate.
errors propagate.

``timeout`` (seconds) is a deadline for the whole request: connecting,
sending, and receiving the status, headers and any error body. On expiry
the live socket is shut down and ``TimeoutError`` is raised, even against
a peer that keeps the connection alive by trickling bytes, and a response
that completes after the deadline is never reported as success. Name
resolution cannot be interrupted. ``None`` means no limit.
"""
scheme = urlsplit(url).scheme
if scheme not in ("http", "https"):
msg = f"Unsupported URL scheme {scheme!r} in {url!r} (need http or https)"
raise ValueError(msg)
request = urllib.request.Request(url, data=body, method=method)
for key, value in headers.items():
request.add_header(key, value)

deadline = _Deadline(timeout) if timeout is not None else None
timer: threading.Timer | None = None
if deadline is not None:
timer = threading.Timer(deadline.seconds, deadline.fire)
timer.daemon = True
timer.start()
timed_out = f"request to {url} exceeded {timeout}s"

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.

Codex AI review · Finding arf_v1_5hxswa2fcrh4k4ojmtj35vr25m

[P1] Redact the endpoint from timeout errors

_ExportScheduler logs exporter exceptions verbatim, so a routine timeout now writes the complete configured URL—including userinfo or signed query parameters—to application logs. Omit it or format only a safely redacted origin, and add a regression test using a secret-bearing URL.

Suggested change
timed_out = f"request to {url} exceeded {timeout}s"
timed_out = f"request exceeded {timeout}s"

try:
with _OPENER.open(request, timeout=timeout) as response: # noqa: S310
return int(response.status), str(response.reason or ""), ""
except urllib.error.HTTPError as exc:
try:
detail = exc.read(_MAX_ERROR_BODY_BYTES).decode("utf-8", errors="replace")
except Exception: # noqa: BLE001 - the body is best-effort detail only
detail = ""
return int(exc.code), str(exc.reason or ""), detail
with _opener(deadline).open(request, timeout=timeout) as response: # noqa: S310
status = int(response.status)
reason = str(response.reason or "")
detail = ""
except urllib.error.HTTPError as exc:
status = int(exc.code)
reason = str(exc.reason or "")
try:
detail = exc.read(_MAX_ERROR_BODY_BYTES).decode(
"utf-8", errors="replace"
)
except Exception: # noqa: BLE001 - the body is best-effort detail only
detail = ""
except Exception as exc:
if deadline is not None and deadline.expired():
raise TimeoutError(timed_out) from exc
raise
finally:
if timer is not None:
timer.cancel()
# Whatever arrived after the deadline is not a delivery.
if deadline is not None and deadline.expired():
raise TimeoutError(timed_out)
return status, reason, detail
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,11 @@ class HttpMethod(StrEnum):
class HttpExporter:
"""Sends each record as a JSON body to any HTTP endpoint.

The endpoint must answer 2xx; any other status raises. ``timeout_ms``
bounds the whole request (default 10 seconds). ``max_record_size_bytes``
The endpoint must answer 2xx; any other status raises. ``timeout_ms`` is a
deadline for the whole request (connect, send, response headers and any
error body; default 10 seconds); on expiry the connection is shut down and
``TimeoutError`` is raised. Name resolution is not interruptible.
``max_record_size_bytes``
has no default because a generic endpoint has no known limit.
"""

Expand Down
Loading
Loading