Fix stale errno handling when recv returns 0 - #562
Conversation
When recv returns 0 on a clean peer close, errno is not required to be updated. Avoid surfacing a stale value by reporting ECONNRESET on POSIX and WSAECONNRESET on Windows. Add a regression test covering the stale errno case. Fixes ClickHouse#487
|
|
|
Hi @puretechteam, could you please sign the CLA? Also, please consider fixing your PR description. |
| throw std::system_error(WSAECONNRESET, getErrorCategory(), "connection closed by peer"); | ||
| #else | ||
| throw std::system_error(ECONNRESET, getErrorCategory(), "connection closed by peer"); |
There was a problem hiding this comment.
ECONNRESET = Connection reset by peer. Here, the connections are closed gracefully when unexpected. This requires a more thoughtful solution.
There was a problem hiding this comment.
Before I revise the error value, could you confirm what error representation you would prefer here? I want to preserve the existing std::system_error / retry behavior rather than introduce a new error abstraction without precedent.
There was a problem hiding this comment.
Honestly, I do not think it is a good idea to return std::system_error here because it suggests that the error occurred somewhere in a system call and that is not the case at all. The system call worked just fine and returned exactly what it was expected to return.
The error occurs at the application level during decoding because the decoder needs more data, but there is none. Maybe the server changed something, maybe a proxy dropped the connection, or maybe we are using a ClickHouse clone with a new protocol implementation.
Either way, this is a decoding error - a truncated data error, to be specific. I would use the ProtocolError exception type here.
There was a problem hiding this comment.
Thanks, that makes sense. I’ll revise the recv() == 0 path to treat the condition as a truncated protocol/decode error and use ProtocolError rather than std::system_error, while keeping actual socket/system-call failures on the existing path.
There was a problem hiding this comment.
🟡 Changes recommended
The new POSIX regression test uses errno/EBADF without including <cerrno>/<errno.h>, which can cause compilation failures on some platforms.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes an error-reporting bug in SocketInput::DoRead where a clean connection close (recv() == 0) could incorrectly surface a stale errno/WSAGetLastError() value to callers, producing misleading std::system_error codes/messages.
Changes:
- Replace the
recv() == 0error path to throw a deterministic close-by-peer error (ECONNRESETon POSIX,WSAECONNRESETon Windows) instead of readingerrno/WSAGetLastError(). - Add a POSIX-only regression test that seeds
errnoto a known stale value before exercising the clean-close path viasocketpair().
File summaries
| File | Description |
|---|---|
clickhouse/base/socket.cpp |
Makes the recv() == 0 path deterministic by throwing a fixed close-by-peer error code/message. |
ut/socket_ut.cpp |
Adds a POSIX regression test ensuring stale errno does not leak into the clean-close exception path. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| #if !defined(_win_) | ||
| # include <sys/socket.h> | ||
| # include <unistd.h> | ||
|
|
When recv() returns 0 the connection was closed cleanly by the peer. The recv() syscall itself succeeded, so this is not an OS-level error and surfacing it as std::system_error was misleading. At the application level the decoder expected more protocol bytes and the connection ended instead, which is a truncated-data / protocol decoding failure. Throw clickhouse::ProtocolError on recv() == 0. The recv() < 0 branch is unchanged and continues to use std::system_error with the real errno/Winsock code, which is the legitimate use of system_error for actual syscall failures. The regression test is updated to expect ProtocolError and to fail hard if a future change re-introduces std::system_error on this path. The errno-seeding step is no longer needed. Refs: ClickHouse#487
Fixes #487.
When
recv()returns 0 for a clean peer close, the socket error value is not guaranteed to be updated. The previous code could therefore surface a stale error value from an earlier system call.This PR adds a regression test covering the stale-error case.
The final error representation for a graceful peer close is still being reviewed.
Validation:
git diff --checkThe patch is intentionally limited to the socket error path and its regression test.