ClickHouse / ClickHouse/clickhouse-cpp

`SocketInput::DoRead` reports stale errno on clean connection close

Closed Beginner friendly
#487 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
C
Stars
382
Forks
208
Avg merge
2d 19h
Merged PRs (30d)
14

Description

# `SocketInput::DoRead` reports stale errno on clean connection close (`recv() == 0`)

## Summary

When the remote peer closes the TCP connection cleanly, `recv()` returns 0. The error handling code at this point calls `getSocketErrorCode()` (which reads `errno` / `WSAGetLastError()`), but `recv()` returning 0 is **not an error** — it is an EOF indication — and the OS does **not** update `errno` in this case. The result is a `std::system_error` with a **stale, misleading error code** from a previous syscall.

## Reproduction

A minimal TCP server that accepts a connection, reads the client Hello, then immediately closes:

```cpp
int client_fd = accept(server_fd, nullptr, nullptr);
// optionally recv() the Hello
close(client_fd);
```

Client code:

```cpp
clickhouse::ClientOptions opts;
opts.SetHost("127.0.0.1");
opts.SetPort(port);
clickhouse::Client client(opts); // throws
```

**Actual exception message:**
```
closed: Operation now in progress
```

**Expected exception message:**
```
Connection closed by peer
```

"Operation now in progress" is `EINPROGRESS` — completely unrelated to a connection close.

## Root Cause

In [`socket.cpp`, `SocketInput::DoRead`](https://github.com/ClickHouse/clickhouse-cpp/blob/master/clickhouse/base/socket.cpp):

```cpp
size_t SocketInput::DoRead(void* buf, size_t len) {
const ssize_t ret = ::recv(s_, (char*)buf, (int)len, 0);

if (ret > 0) {
return (size_t)ret;
}

if (ret == 0) {
throw std::system_error(getSocketErrorCode(), getErrorCategory(), "closed");
// ^^^^^^^^^^^^^^^^^^^ BUG: errno is stale
}

throw std::system_error(getSocketErrorCode(), getErrorCategory(), "can't receive string data");
}
```

When `recv()` returns 0, the POSIX specification does not require `errno` to be set. The value of `errno` remains whatever it was from the **last syscall that failed**. In this case, the typical call sequence is:

1. `SocketConnect` → `connect()` in non-blocking mode → `errno` set to **`EINPROGRESS`**
2. `Poll()` succeeds → `getsockopt(SO_ERROR)` returns 0 → socket switched to blocking
3. `SendHello()` → `send()` succeeds → `errno` unchanged (success doesn't clear `errno`)
4. `ReceiveHello()` → `recv()` returns **0** (peer closed) → `errno` still **`EINPROGRESS`**
5. `getSocketErrorCode()` returns `EINPROGRESS` → exception says "Operation now in progress"

Depending on timing and platform, the stale value could be any previous error code, making the exception message **non-deterministic and misleading**.

## Impact

- **Debugging difficulty**: The misleading error code sends developers on the wrong path. "Operation now in progress" suggests a non-blocking socket issue, not a closed connection.
- **Error handling**: Callers catching `std::system_error` and inspecting `.code().value()` get an incorrect error code, making programmatic retry/recovery logic unreliable.
- **Non-determinism**: The stale errno value varies depending on which syscall last set it, making the bug platform- and timing-dependent.

## Suggested Fix

For the `recv() == 0` case, use a well-defined error code instead of reading `errno`:

```cpp
if (ret == 0) {
throw std::system_error(
ECONNRESET, getErrorCategory(), "connection closed by peer");
}
```

Or on Windows:

```cpp
if (ret == 0) {
#if defined(_win_)
throw std::system_error(
WSAECONNRESET, windowsErrorCategory::category(), "connection closed by peer");
#else
throw std::system_error(
ECONNRESET, std::system_category(), "connection closed by peer");
#endif
}
```

Alternatively, a custom error code/category could be used to distinguish a clean close (FIN) from a reset (RST), but at minimum the stale `getSocketErrorCode()` call must be removed from this path.

## Environment

- **Library**: clickhouse-cpp (all current versions)
- **Affected platforms**: All (Linux, macOS, Windows)
- **Affected file**: `clickhouse/base/socket.cpp`, `SocketInput::DoRead`

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in clickhouse/base/socket.cpp at SocketInput::DoRead and trace the recv() == 0 path together with the existing platform-specific error categories. Replace the stale socket-error lookup with a defined connection-closed error, then verify the Client reproduction reports a connection-close message rather than a previous errno value.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
networking
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.