`HTTPConnectionPool::isStale` decided whether a pooled keep-alive
connection was still usable with `poll(SELECT_READ, 0)`: readable meant
stale, on the assumption that an idle keep-alive connection has no data
pending. On a plain socket that holds, but on an HTTPS connection it
produces a false positive. `poll` reflects the raw TCP socket, so an
unread TLS post-handshake record (a session ticket or `KeyUpdate`) makes
an entirely live secure connection look readable, i.e. stale, and the
pool discards it and reconnects - at the cost of a fresh TLS handshake -
on nearly every borrow.
This ports the shared, TLS-aware helper `isSocketPeerClosed` (added in
`src/IO`) from the distributed cache fix and switches
`HTTPConnectionPool::isStale` to it:
- for a plain socket, a single non-blocking, non-destructive
`recv(..., MSG_PEEK | MSG_DONTWAIT)`: `0` means the peer sent a FIN
(closed); `> 0` means bytes are on the wire (alive); `EAGAIN` /
`EWOULDBLOCK` means a healthy idle connection; any other error (e.g.
`ECONNRESET`) is treated as closed/broken;
- for a TLS (`SecureStreamSocket`) socket, an `SSL_peek` on a temporarily
non-blocking socket. A raw `MSG_PEEK` cannot tell TLS records apart -
an unread `close_notify` looks the same as unread application data or a
session ticket, and in TLS 1.3 the record content type is encrypted.
`SSL_peek` decrypts just enough to distinguish real application data and
harmless post-handshake messages (a session ticket or `KeyUpdate`),
treated as alive, from a `close_notify` (`SSL_ERROR_ZERO_RETURN`) or a
broken session, treated as closed, without consuming application data.
A small `ssl` accessor is added to Poco's
`SecureSocketImpl` / `SecureStreamSocketImpl` to reach the underlying
OpenSSL `SSL` object.
`gtest_socket_peer_closed` covers both paths, including a
`socketpair`-backed TLS connection: an unread session ticket and pending
application data read as alive; a `close_notify` and an abrupt close read
as closed. The `close_notify` case also asserts that a raw `MSG_PEEK`
misfires there, which is exactly why the TLS-aware path is needed.
This is the public-repo counterpart, and the follow-up promised in, the
distributed cache fix https://github.com/ClickHouse/clickhouse-private/pull/64217
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
completeHandshake had the same issue as sendBytes/receiveBytes:
handleError could return ERR_SSL_WANT_READ (-1) when the timeout was
exhausted, and the negative value leaked to callers silently.
Add the same blocking-socket check. Add a test that connects an SSL
client to a plain TCP server -- the handshake stalls and must throw
instead of returning -1.
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
SecureSocketImpl::sendBytes and receiveBytes could return negative
ERR_SSL_WANT_READ/ERR_SSL_WANT_WRITE values to callers when the SSL
timeout was exhausted. For blocking sockets this means the operation
timed out, but the error propagated as a silent return code instead of
an exception. Callers like HTTPSession::write did not check for negative
returns from sendBytes, causing the error to reach BufferedStreamBuf
which converted it to ios_base::failure, losing all error context and
bypassing S3 retry logic.
Throw TimeoutException for blocking sockets when handleError returns
a negative value. Non-blocking callers still receive the return codes
they expect.
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Previously if you were using socket without timeout it wasn't able to
handle SSL_ERROR_WANT_READ/SSL_ERROR_WANT_WRITE, and even though sockets
without timeouts is an odd thing (but it is possible - [1]), it still
may be possible somewhere.
[1]: https://github.com/ClickHouse/ClickHouse/pull/65917
Signed-off-by: Azat Khuzhin <[email protected]>
* Replicate poco into base/poco/
* De-register poco submodule
* Build poco from ClickHouse
* Exclude poco from stylecheck
* Exclude poco from whitespace check
* Exclude poco from typo check
* Remove x bit from sources/headers (the style check complained)
* Exclude poco from duplicate include check
* Fix fasttest
* Remove contrib/poco-cmake/*
* Simplify poco build descriptions
* Remove poco stuff not used by ClickHouse
* Glob poco sources
* Exclude poco from clang-tidy
