Include the peer address in deferred connect errors - #115718
Open
valerypetrov wants to merge 29 commits into
Open
Include the peer address in deferred connect errors#115718valerypetrov wants to merge 29 commits into
valerypetrov wants to merge 29 commits into
Conversation
Non-blocking connect failures discovered via poll()/SO_ERROR (the normal path for refused connections to remote hosts, e.g. replica part fetches) threw "Connection refused" without the peer address, unlike every other failure path in SocketImpl::connect. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
Workflow [PR], commit [b5736ba] AI ReviewSummaryThis PR moves deferred-connect endpoint attribution to the actual connect site, threads the HTTPS proxy tunnel through the caller-resolved proxy address, and mirrors the same endpoint naming in the native client's manual non-blocking connect loop. I did not find a remaining blocker or major correctness issue in the current code, and the earlier misattribution / dropped-endpoint findings are fixed in the current head. Missing context / blind spots
Final Verdict✅ No blocking review findings. |
Contributor
Build profile diff (arm_release)Comparing ✅ No significant changes. Binary sizes
Only the stripped binary is compared: the official master build keeps debug symbols while PR builds strip them, so the other binaries differ by construction. Compile time of recompiled translation units397 translation units recompiled, 2406 s compile time in total, 397 of them have a recent master baseline. |
Adding the address inside Poco::Net::SocketImpl::connect also hit the native client, whose own handler already appends the endpoint, so errors read "Connection refused: host:port (host:port)." and 02550_benchmark_connections_credentials failed on the doubled text. Restore SocketImpl and extend the message in the HTTP connection pool instead: it covers the url() and part-fetch paths this is about, keeps the exception type, and leaves client-facing messages as they were. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PcH1uX3QLcLes3XfidQqsC
Poco::Exception's message setters are protected, so the previous version did not compile. Rethrow ConnectionRefusedException carrying the endpoint instead. Catching the NetException base would have been broader but would also swallow SSLException, which the caller has to keep telling apart from a per-address routing failure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PcH1uX3QLcLes3XfidQqsC
The synchronous ECONNREFUSED path in Poco::Net::SocketImpl::connect throws ConnectionRefusedException(address), so rewriting it produced "Connection refused: host:port: host:port". Only the deferred poll()/SO_ERROR path arrives with a bare message, so rethrow unchanged unless the message is empty, and guard the no-duplication property in gtest_connection_pool. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PcH1uX3QLcLes3XfidQqsC
doConnect rebuilt the message from the request host, so a direct connect lost the resolved peer IP the PR promises and a proxied one named the target instead of the proxy it was really dialling. Use getResolvedAddress(), or the proxy endpoint when the proxy is not bypassed, and share the bypass mirror with prepareNewConnection. The other deferred SO_ERROR failures keep Poco's own text, which the comment and the changelog now say instead of implying otherwise. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PcH1uX3QLcLes3XfidQqsC
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PcH1uX3QLcLes3XfidQqsC
It is file-local; the missing prototype fails the -Wmissing-prototypes build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PcH1uX3QLcLes3XfidQqsC
connectEndpoint() formats host:port through a bracket-aware helper for both the proxy and direct branches, matching SocketAddress::toString(); the bare-text check uses contains() for the tidy build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PcH1uX3QLcLes3XfidQqsC
Session::reconnect resolves the proxy hostname inside connect(), so a deferred SO_ERROR failure reconstructed the endpoint from proxy_config.host and could not tell which of several proxy records refused. Stash the SocketAddress each connect attempt dials and prefer it in connectEndpoint(); cleared before every attempt so pre-connect DNS failures cannot leak a stale address. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
HTTPSClientSession::connect ignores the SocketAddress reconnect() resolved on the tunnel branch: proxyConnect() built a fresh session from the proxy hostname and resolved it again, so with several A/AAAA records the deferred error could name a different record than the one actually dialled. proxyConnect() now takes the caller's resolved address and pins it on the inner session via setResolvedHost, so the tunnel dials exactly the address the stash recorded; proxyTunnel() and other callers keep the old behavior. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
test_disk_over_web_server asserted the old bare 'Connection refused' text and now expects the address the refusal names. Adds the asked-for tunnel regression: an HTTPS request through an HTTP proxy whose resolvable name has a dead port, asserting the deferred error carries the resolved proxy address rather than the configured name, which a regression back to a bare proxyConnect() would report instead.
Two same-process resolutions of one hostname always agree, so a DNS-level test alone cannot tell 'reuse the caller's resolved address' from 'resolve the proxy again'. The new gtest makes the two disagree on purpose: the proxy hostname resolves to a live listener on 127.0.0.1 while the address passed to HTTPSClientSession::connect is 127.0.0.99, so honoring the pinned address is refused at TCP connect and a regression to bare proxyConnect() reaches the listener and fails in the CONNECT exchange or TLS handshake instead - the failure modes cannot be confused. The integration side adds the asked-for multi-record proxy hostname (two /etc/hosts A records, both with a dead port) asserting the deferred error names one concrete record and not the name. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PooledConnection::connect() stashes dialled_address before any TCP connect can fail, and both catch handlers in doConnect() only rewrap errors that occur after such a connect attempt was made (a bare ConnectionRefusedException, or a NetException carrying ENETUNREACH/ EHOSTUNREACH/EHOSTDOWN). The only earlier throw point, SocketAddress's DNS resolution, raises DNSException subclasses that match neither catch's type check, so dialled_address is always set by the time connectEndpoint() runs and the proxy/resolved-host fallbacks - and the formatHostAndPort() helper that served them - never execute. The integration test added by this PR already asserts the fallback output must never appear. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A borrowed connection keeps only a weak pointer to its endpoint pool, and that pool can disappear while the connection is still in use: `dropCache()` clears every endpoint, and the periodic `wipeExpired()` sweep erases pools that hold no stored connection. `PooledConnection::reconnect` then dials on its own instead of asking the pool for a fresh connection, and that fallback went straight to `Session::reconnect`, so a deferred connect error came back with Poco's bare text and no address - exactly what the rest of this change set out to fix. Route the fallback through `doConnect` as well. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The rewrite of bare deferred connect errors lived only inside PooledConnection::doConnect, so pool-backed sessions named their peer while callers that construct a session directly and go through HTTPClientSession::sendRequest -> reconnect -> connect - S3 credentials renewal, JWT provider, BuzzHouse - kept emitting bare "Connection refused" / "No route to host" without an address on deferred failures. Move the rewrite into reconnect itself: it is the one place that knows both branches of the address it dials, so every carrier of HTTPClientSession and HTTPSClientSession benefits alike, and PooledConnection::doConnect shrinks to reconnect plus socket inode accounting. Immediate failures already carry the peer and are left untouched, so no message grows a repeated endpoint.
ProxyTunnelDialsTheCallerResolvedAddress used localhost as the proxy name, which stays green if a dual-stack resolver picks ::1: the buggy path then dials an unbound [::1] port, still raises ConnectionRefusedException and passes. Configure the literal 127.0.0.1 where the listener is bound instead, so a regression always reaches the live listener and fails later in the CONNECT exchange or TLS handshake. Add DirectSessionReconnectNamesThePeer for sessions built without the pool (S3 credentials, JWT provider, BuzzHouse): their sendRequest must end up with the endpoint named exactly once, whether Poco reported it natively on the immediate path or reconnect restored it on the deferred one.
Contributor
Author
|
Update on the review threads:
All 24 ConnectionPool gtests pass locally. |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PcH1uX3QLcLes3XfidQqsC
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PcH1uX3QLcLes3XfidQqsC
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PcH1uX3QLcLes3XfidQqsC
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PcH1uX3QLcLes3XfidQqsC
The reconnect-level rewrap could not tell a deferred connect failure from the same bare shape raised after a successful dial: a proxy blackholing the CONNECT exchange threw a bare TimeoutException from receiveBytes on the inner tunnel session, the outer session's socket was still unattached, and the failure was relabeled 'connect timed out: <proxy>'. SocketImpl::connect now names the endpoint on its deferred SO_ERROR path (matching its other two paths), and reconnect passes everything through untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PcH1uX3QLcLes3XfidQqsC
The deferred and immediate connect paths now name the resolved address, so the benchmark's wrapped message gained a middle segment; the test normalizes it away and its reference stays unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PcH1uX3QLcLes3XfidQqsC
Connection::connect walks several resolved addresses with its own connectNB + poll + socketError loop, and reported the failure without saying which address it was. It now passes the dialled address the same way SocketImpl::connect does. The wrap below no longer appends the connection description when the message already contains it, so a numeric host stops rendering the endpoint twice. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PcH1uX3QLcLes3XfidQqsC
SocketImpl::error(int, const std::string &) is protected, so naming the address from Connection.cpp's own non-blocking connect loop did not compile. Add a public throwSocketError() that pairs the socketError() read with that throw inside the class, keeping the protected overload protected and dropping the static-accessed-through-instance NOLINT. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PcH1uX3QLcLes3XfidQqsC
Suppressing the trailing (host:port) when the message already named the address broke 03025_clickhouse_host_env, which matches exactly that suffix and passes on master. Naming the endpoint inside the Poco message was the point of this PR, but the suffix is a stable part of the client's connect error that callers match on, so it stays unconditional and the mild repetition is accepted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PcH1uX3QLcLes3XfidQqsC
POCO_ETIMEDOUT is the one case in SocketImpl::error's switch that throws without arg, so routing the client's own non-blocking connect loop through error(err, arg) lost the address on exactly the timeout it was added to name. Mirror connect()'s deferred branch and synthesize the TimeoutException with the endpoint. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PcH1uX3QLcLes3XfidQqsC
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes: #48205
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Connect errors detected after a non-blocking connect (for example, while fetching a part from another replica) now name the peer:
Connection refused,Network is unreachable,No route to host, andHost is down.Documentation entry for user-facing changes
Poco::Net::SocketImpl::connect(address, timeout)reports connect failures in two ways: an immediate failure of::connectalready includedaddress.toString(), but an error discovered afterEINPROGRESSviapoll()+SO_ERROR— the normal outcome when the peer is a remote host, as in replicated part fetches — reaches the caller througherror(err), with no address argument at all. This is why logs showed bareConnection refusedwith no hint of which replica was unreachable. The message now becomesConnection refused: <ip>:<port>.The address reported is the endpoint that was actually dialled: the address the resolver picked for this connection (so the resolved IP is shown, which also helps diagnose stale DNS entries, as noted in the issue), or the proxy endpoint when a proxy is in use and not bypassed. An immediate failure already carries the address, so those messages are left untouched rather than repeating it.
Scope.
Connection refusedis rethrown as aConnectionRefusedExceptioncarrying the endpoint. The other deferredSO_ERRORfailures lose the address on the same path and arrive as plainNetExceptions with Poco's bare text (Network is unreachable,No route to host,Host is down); those are restated asNetExceptions with the endpoint appended.NetExceptionsubclasses such asSSLException, and immediate failures whose text already names the peer, pass through untouched.The integration test uses two instances because the deferred-error path only triggers for non-loopback connects, and covers both an IP literal and a hostname, so the resolved address is asserted rather than the name from the URL.
Workflow [PR]
Sync PR [sync-upstream/pr/115718]