Conversation
This comment has been minimized.
This comment has been minimized.
83efa31 to
535d56d
Compare
c5f10f2 to
868a2fc
Compare
| expired.set() | ||
| sock = conn.sock | ||
| if sock is not None: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Done in 37e5029. The connection classes now check the deadline before connecting, give the attempt only the remaining budget, and refuse a socket that arrives after expiry; http_send also re-checks expiry after every phase, so a response completing after the deadline is never returned as success. Test: test_deadline_applies_when_the_connection_is_slow (connect outlives the deadline; TimeoutError; no request reaches the server). Name resolution stays non-interruptible, which the docstring now says.
This comment has been minimized.
This comment has been minimized.
868a2fc to
37e5029
Compare
| Connections go directly to the URL's host; proxy environment variables | ||
| (``HTTP_PROXY``, ``HTTPS_PROXY``, ``NO_PROXY``) are not consulted. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Resolved in 886e893 by keeping the urllib transport: the opener is still built with build_opener, so the default ProxyHandler and its HTTP_PROXY/HTTPS_PROXY/NO_PROXY handling are unchanged from main.
|
|
||
| def connect(self) -> None: | ||
| _before_connect(self, self._deadline) | ||
| super().connect() |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
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).
| parts.hostname, | ||
| parts.port, |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Resolved in 886e893 by keeping the urllib transport, which handles bracketed IPv6 hosts itself; the direct http.client construction that stripped them is gone.
| timer.cancel() | ||
| conn.close() | ||
| # Whatever arrived after the deadline is not a delivery. | ||
| if deadline is not None and deadline.expired.is_set(): |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Done in 886e893. Expiry is now judged by the monotonic clock (_Deadline.expired), both in the connect path and after the request/error-body read; the timer only wakes blocked sockets. A late timer callback can no longer let a late response through.
| except Exception: # noqa: BLE001 - the body is best-effort detail only | ||
| detail = "" | ||
| return int(exc.code), str(exc.reason or ""), detail | ||
| conn.request(method, path, body=body, headers=headers) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Resolved in 886e893 by keeping urllib's Request.add_header, which merges differently cased header names with last-value-wins. test_custom_header_case_is_merged_not_duplicated asserts the server receives exactly one Content-Type.
| parts.hostname, | ||
| parts.port, | ||
| timeout=timeout, | ||
| context=ssl.create_default_context(), |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Resolved in 886e893 by keeping the urllib transport: no explicit SSL context is passed, so HTTPSConnection builds the stdlib default with ALPN http/1.1 as before.
This comment has been minimized.
This comment has been minimized.
- http_send: build the opener per request with handlers that create deadline-aware connections; a timer shuts the live socket down when the deadline expires and TimeoutError is raised, so a peer trickling bytes can no longer keep a request alive past timeout_ms - every TCP attempt is budgeted with the remaining time and registered with the deadline, so a stalled connect is aborted on time; expiry is judged by the monotonic clock after the request and after the error body read, so a late response is never reported as success - the urllib transport is otherwise unchanged (redirects refused, header merging, IPv6 hosts, proxy settings, default TLS context) - tests: trickled status line, trickled error body, slow name resolution, stalled connect, header case merge, unsupported scheme
37e5029 to
886e893
Compare
| timer = threading.Timer(deadline.seconds, deadline.fire) | ||
| timer.daemon = True | ||
| timer.start() | ||
| timed_out = f"request to {url} exceeded {timeout}s" |
There was a problem hiding this comment.
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.
| timed_out = f"request to {url} exceeded {timeout}s" | |
| timed_out = f"request exceeded {timeout}s" |
| _bind_deadline(self, deadline) | ||
|
|
||
| def connect(self) -> None: | ||
| super().connect() |
There was a problem hiding this comment.
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.
Codex AI reviewTwo issues remain: timeout failures can disclose endpoint secrets, and HTTPS TLS negotiation can exceed the new deadline. Reviewed commit |
Summary
Makes
HttpExporter.timeout_msa deadline for the whole request, as its docstring promised. Review on #720 measured that the value was applied per socket read, so a server trickling one byte per second finished atimeout_ms=2000request after 37 s.The transport stays on
urllibexactly as merged (redirects refused, header merging, IPv6 hosts, proxy settings, stdlib TLS context). The only addition is a deadline hook:http_sendbuilds its opener per request with handlers that create a connection subclass. Every TCP attempt is budgeted with the remaining time and registered with the deadline; a timer shuts the registered sockets down on expiry andTimeoutErroris raised. Expiry is judged by the monotonic clock after the request and after any error-body read, so a response that completes late is never reported as success.timeout=Nonestill means no limit, soOTelExporterandOpenSearchExporterare unchanged.Testing
test_timeout_is_a_whole_request_deadline: status line trickled one byte per 200 ms;timeout_ms=500raises at 0.5 s.test_deadline_applies_while_reading_an_error_body: immediate 500, trickled body;TimeoutError, notRuntimeError.test_deadline_applies_when_name_resolution_is_slow: no request is sent once the deadline has passed.test_deadline_interrupts_a_stalled_connect: a connect that never completes is aborted at the deadline (0.32 s on 300 ms), not at the socket timeout.test_custom_header_case_is_merged_not_duplicated,test_unsupported_url_scheme_is_rejected.hatch fmt --check,hatch run types:check, andhatch run test:allover the core, insight, otel, and testing packages pass.Notes