You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by "CritasWang (via GitHub)" <gi...@apache.org> on 2026/07/31 03:52:22 UTC
[PR] Fix SessionPool blocking indefinitely after a server outage 修复服务端断开后 SessionPool 永久阻塞的问题 [iotdb-client-csharp]
CritasWang opened a new pull request, #62:
URL: https://github.com/apache/iotdb-client-csharp/pull/62
Fixes #61
## What this fixes / 修复内容
A `SessionPool` that outlived a server outage became permanently unusable: callers blocked forever with no
exception, and the pool could not recover even after the server came back. Two independent defects combined
to produce that.
服务端断开后，`SessionPool` 会变得永久不可用：调用方永久阻塞且不抛异常，服务端恢复后也无法自愈。这是
两个独立缺陷叠加的结果。
### 1. Pool wait timeout was written in ms and read in seconds / 池等待超时的单位错误
`Open()` assigned `_clients.Timeout = _timeout * 5` (a millisecond value) while
`ConcurrentClientQueue.Take()` consumed it via `TimeSpan.FromSeconds`. The 500 ms default connection timeout
therefore produced a **~41 minute** block before `SessionPoolDepletedException` surfaced; the legacy
`timeout: 10000` constructor argument produced **13.9 hours**.
`Open()` 以毫秒赋值而 `Take()` 按秒消费，500ms 的默认连接超时因此变成约 **41 分钟** 的阻塞，旧构造函数
传 `timeout: 10000` 时更是 **13.9 小时**。
- `ConcurrentClientQueue` now exposes `TimeoutInMs` and waits with `TimeSpan.FromMilliseconds`.
`ConcurrentClientQueue` 改用 `TimeoutInMs`，并以 `TimeSpan.FromMilliseconds` 等待。
- The old seconds-based `Timeout` property is kept as an `[Obsolete]` shim so existing callers still compile
and behave as they intended.
原秒级 `Timeout` 属性保留为 `[Obsolete]` 兼容包装，既有调用方仍可编译且语义符合预期。
- The pool wait budget is no longer derived from the socket timeout. It is configurable via
`SetPoolWaitTimeoutInMs` on both builders, defaulting to 10 s.
池等待时间不再由 socket 超时推导，改为两个 Builder 上的 `SetPoolWaitTimeoutInMs`，默认 10 秒。
### 2. Pool slots leaked on every failed reconnection / 重连失败导致槽位泄漏
When reconnection failed, the dead client was discarded without being returned and nothing replenished the
pool, so capacity shrank by one per failure. After `PoolSize` failures every caller blocked on an empty queue
that nobody would ever feed, and the pool stayed dead after the server recovered.
重连失败时失效连接被丢弃且不归还，池无补充机制，每失败一次容量减一；`PoolSize` 次失败后所有调用都阻塞在
无人投喂的空队列上，服务端恢复后连接池依然是死的。
- Vacant slots are now tracked (`_vacantSlots`, surfaced as `SessionPool.VacantSlots`) instead of being
silently dropped, so capacity stays at `PoolSize`.
改为记录空槽（`_vacantSlots`，通过 `SessionPool.VacantSlots` 暴露），容量保持为 `PoolSize`。
- `AcquireClientAsync` re-materializes a vacant slot when the queue is empty, so the pool heals itself on the
next operation once the server is reachable — no `Close()` + `Open()` cycle needed.
队列为空时 `AcquireClientAsync` 会就地重建空槽，服务端恢复后下一次操作即可自愈，无需 `Close()` + `Open()`。
- `Reconnect()` now accepts a null original client so the self-heal path also works with `nodeUrls`
(multi-node) configurations.
`Reconnect()` 允许原始连接为 null，使 `nodeUrls`（多节点）配置下的自愈路径同样可用。
### 3. `IsOpen()` semantics documented / 明确 `IsOpen()` 语义
`IsOpen()` is a lifecycle flag, not a connectivity probe — the client keeps no heartbeat, so it stays `true`
after the server goes down. Behaviour is unchanged (and matches the Java client); this PR only documents it,
because the guard `if (pool.IsOpen()) return;` is a common way to accidentally prevent a pool from ever being
rebuilt.
`IsOpen()` 是生命周期标志而非连通性探针——客户端无心跳，服务端断开后它仍为 `true`。行为未变（与 Java
客户端一致），本 PR 仅补充文档说明，因为 `if (pool.IsOpen()) return;` 这种守卫很容易让连接池永远不被重建。
## Compatibility / 兼容性
No breaking changes. New constructor overloads take the pool wait timeout as an extra parameter and the
existing overloads delegate to them with the default; `ConcurrentClientQueue.Timeout` still compiles and now
converts to/from `TimeoutInMs`.
无破坏性变更。新增的构造函数重载多接收一个池等待超时参数，原有重载以默认值委托到新重载；
`ConcurrentClientQueue.Timeout` 仍可编译，并与 `TimeoutInMs` 相互换算。
Behavioural change worth calling out: a depleted pool now fails after 10 s by default instead of tens of
minutes. Anyone who (unintentionally) relied on the long wait should set `SetPoolWaitTimeoutInMs` explicitly.
需要注意的行为变化：池耗尽时现在默认 10 秒后失败，而不是数十分钟。若有人（无意中）依赖了原来的长等待，
请显式设置 `SetPoolWaitTimeoutInMs`。
## Tests / 测试
- `ConcurrentClientQueueTests` — timeout is honoured in milliseconds (the direct regression guard), the
message reports the unit, `Return()` wakes a blocked `Take()`, a ready client is returned without waiting,
and the obsolete `Timeout` shim converts correctly.
`ConcurrentClientQueueTests` —— 超时按毫秒生效（针对该 bug 的直接回归测试）、异常消息标注单位、
`Return()` 能唤醒阻塞的 `Take()`、有可用连接时立即返回、过时的 `Timeout` 兼容属性换算正确。
- `SessionPoolConfigurationTests` — the pool wait timeout is independent of the connection timeout and is
propagated by both `SessionPool.Builder` (host/port and nodeUrls paths) and `TableSessionPool.Builder`.
`SessionPoolConfigurationTests` —— 池等待超时与连接超时相互独立，且能被 `SessionPool.Builder`
（host/port 与 nodeUrls 两条路径）和 `TableSessionPool.Builder` 正确传递。
`dotnet test tests/Apache.IoTDB.Tests` — 33 passed, 0 failed. `dotnet build Apache.IoTDB.sln` — 0 errors.
`dotnet format --verify-no-changes` — clean.
Note: the unit tests were executed locally against `net8.0` because no .NET 5 arm64 runtime is available on
this machine; the committed `TargetFramework` is unchanged at `net5.0`.
说明：本机没有 .NET 5 的 arm64 运行时，单元测试在本地以 `net8.0` 执行；提交的 `TargetFramework` 仍为 `net5.0`。
## Docs / 文档
- `docs/SessionPool_Exception_Handling.md` — new sections on `IsOpen()` semantics and the pool wait timeout,
`VacantSlots` added to the health metrics table, self-healing behaviour described, and the stale builder
API names in the examples corrected (`.Host()` → `.SetHost()`, `.Timeout()` → `.SetConnectionTimeoutInMs()`,
etc., which did not compile as written).
`docs/SessionPool_Exception_Handling.md` —— 新增 `IsOpen()` 语义与池等待超时章节，健康指标表补充
`VacantSlots`，说明自愈行为，并修正示例中过时的 Builder API 名（原样无法编译）。
- `docs/API.md` — clarified the `IsOpen` row.
`docs/API.md` —— 澄清 `IsOpen` 一行的说明。
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix SessionPool blocking indefinitely after a server outage 修复服务端断开后 SessionPool 永久阻塞的问题 [iotdb-client-csharp]
Posted by "HTHou (via GitHub)" <gi...@apache.org>.
HTHou commented on code in PR #62:
URL: https://github.com/apache/iotdb-client-csharp/pull/62#discussion_r3687978031
##########
src/Apache.IoTDB/SessionPool.cs:
##########
@@ -172,10 +205,61 @@ protected internal SessionPool(List<string> nodeUrls, string username, string pa
_certificatePath = certificatePath;
_sqlDialect = sqlDialect;
_database = database;
+ _poolWaitTimeoutInMs = poolWaitTimeoutInMs;
+ }
+ /// <summary>
+ /// Acquires a client from the pool. If the pool has no idle client but owns vacant slots left behind
+ /// by earlier failed reconnections, one of those slots is re-materialized on the spot instead of
+ /// blocking on a queue that nobody will ever feed. This is what lets the pool recover on its own
+ /// after the server has been unreachable for a while.
+ /// </summary>
+ private async Task<Client> AcquireClientAsync(CancellationToken cancellationToken = default)
+ {
+ if (_clients.ClientQueue.IsEmpty && TryReserveVacantSlot())
Review Comment:
[P2] This only rebuilds a vacant slot when the idle queue is completely empty. After a full outage, the first successful request creates one connection and returns it; under sequential or light workloads, later requests keep reusing that connection, so the remaining `VacantSlots` never return to zero even though the server is healthy. That also makes the documented alert semantics (“steady non-zero means the server is still unreachable”) incorrect. Please either refill the remaining capacity after recovery, or explicitly define this as demand-driven capacity and adjust the metric/documentation accordingly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix SessionPool blocking indefinitely after a server outage 修复服务端断开后 SessionPool 永久阻塞的问题 [iotdb-client-csharp]
Posted by "CritasWang (via GitHub)" <gi...@apache.org>.
CritasWang commented on code in PR #62:
URL: https://github.com/apache/iotdb-client-csharp/pull/62#discussion_r3688062462
##########
src/Apache.IoTDB/ConcurrentClientQueue.cs:
##########
@@ -70,7 +90,7 @@ public Client Take()
bool timeout = false;
if (ClientQueue.IsEmpty)
{
- timeout = !Monitor.Wait(ClientQueue, TimeSpan.FromSeconds(Timeout));
+ timeout = !Monitor.Wait(ClientQueue, TimeSpan.FromMilliseconds(TimeoutInMs));
Review Comment:
Confirmed and fixed in f855e96 — you are right that the original failure mode was still reachable under contention.
`Take()` now captures the budget once and recomputes the remaining duration on each wake-up:
```csharp
var budgetMs = TimeoutInMs;
var elapsed = Stopwatch.StartNew();
while (true)
{
if (ClientQueue.TryDequeue(out client)) break;
var remainingMs = budgetMs - (int)elapsed.ElapsedMilliseconds;
if (remainingMs <= 0) break;
Monitor.Wait(ClientQueue, TimeSpan.FromMilliseconds(remainingMs));
}
```
I also restructured the loop to dequeue first rather than testing `IsEmpty` and then dequeuing, which removes the separate `timeout` flag.
Two regression tests added:
- `Take_RepeatedWakeUps_StillHonoursTheOverallDeadline` — a background task pulses the monitor every 20 ms while the queue is **never** fed, so the waiter is only ever woken spuriously. With a 200 ms budget it must still give up. I verified this test against the pre-fix behaviour by temporarily pinning `remainingMs = budgetMs`: the test hangs indefinitely instead of failing fast, which is precisely the defect you described.
- `Take_MultipleWaiters_EachHonoursTheOverallDeadline` — four concurrent waiters, one client returned; the three losers must each settle within their own deadline.
I used spurious pulses rather than return/reclaim churn for the first test because the latter was racy — the waiter occasionally won the client and the test flaked.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix SessionPool blocking indefinitely after a server outage 修复服务端断开后 SessionPool 永久阻塞的问题 [iotdb-client-csharp]
Posted by "CritasWang (via GitHub)" <gi...@apache.org>.
CritasWang commented on code in PR #62:
URL: https://github.com/apache/iotdb-client-csharp/pull/62#discussion_r3688114401
##########
src/Apache.IoTDB/SessionPool.cs:
##########
@@ -172,10 +205,61 @@ protected internal SessionPool(List<string> nodeUrls, string username, string pa
_certificatePath = certificatePath;
_sqlDialect = sqlDialect;
_database = database;
+ _poolWaitTimeoutInMs = poolWaitTimeoutInMs;
+ }
+ /// <summary>
+ /// Acquires a client from the pool. If the pool has no idle client but owns vacant slots left behind
+ /// by earlier failed reconnections, one of those slots is re-materialized on the spot instead of
+ /// blocking on a queue that nobody will ever feed. This is what lets the pool recover on its own
+ /// after the server has been unreachable for a while.
+ /// </summary>
+ private async Task<Client> AcquireClientAsync(CancellationToken cancellationToken = default)
+ {
+ if (_clients.ClientQueue.IsEmpty && TryReserveVacantSlot())
Review Comment:
Renamed in 3ad458f: `VacantSlots` → `UnrealizedCapacity`.
"Vacant slots" reads as "something is broken", which is exactly the wrong intuition now that refill is documented as demand-driven — a steady non-zero value under light load is normal, not a fault. `UnrealizedCapacity` states what the number actually measures: configured capacity that has not been materialized yet. The private field and `TryReserveVacantSlot` were renamed to match, and the metrics table, prose and tests updated.
No compatibility impact — the property is introduced by this PR and has never shipped.
I kept the demand-driven behaviour rather than adding eager refill, for the reasons above. Happy to revisit if you would still prefer the pool to restore full capacity proactively after recovery.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix SessionPool blocking indefinitely after a server outage 修复服务端断开后 SessionPool 永久阻塞的问题 [iotdb-client-csharp]
Posted by "HTHou (via GitHub)" <gi...@apache.org>.
HTHou commented on code in PR #62:
URL: https://github.com/apache/iotdb-client-csharp/pull/62#discussion_r3687977253
##########
src/Apache.IoTDB/ConcurrentClientQueue.cs:
##########
@@ -70,7 +90,7 @@ public Client Take()
bool timeout = false;
if (ClientQueue.IsEmpty)
{
- timeout = !Monitor.Wait(ClientQueue, TimeSpan.FromSeconds(Timeout));
+ timeout = !Monitor.Wait(ClientQueue, TimeSpan.FromMilliseconds(TimeoutInMs));
Review Comment:
[P1] Please preserve one overall deadline across wake-ups. `Return()` uses `PulseAll`, so all waiters wake while only one can dequeue the returned client; the others loop and call `Monitor.Wait` with the full `TimeoutInMs` again. Under steady pool churn, an unlucky waiter can therefore exceed the configured bound indefinitely, so the original “caller blocks forever” failure mode is still possible under contention. A regression test with a 200 ms timeout and repeated wake-ups currently waits about 556 ms. Please compute a deadline/remaining duration and add a multi-waiter regression test.
##########
src/Apache.IoTDB/SessionPool.cs:
##########
@@ -372,6 +462,16 @@ public async Task<Client> Reconnect(Client originalClient = null, CancellationTo
throw new ReconnectionFailedException("Error occurs when reconnecting session pool. Could not connect to any server");
}
+ /// <summary>
+ /// Indicates whether this pool has been opened and not yet closed by the caller.
+ /// </summary>
+ /// <remarks>
+ /// This reflects the lifecycle of the pool object only - it is NOT a server-connectivity probe.
+ /// The client performs no heartbeat, so a server going down does not flip this back to false;
+ /// it stays true until <see cref="Close"/> is called. To reason about connectivity, use
Review Comment:
[P2] `Close()` does not always make this statement true. Once every connection has become a vacant slot, `ClientQueue` is empty; the `foreach` in `Close()` has zero iterations, and `_isClose = true` is only assigned inside that loop. As a result, `Close()` returns while `IsOpen()` remains true, and the vacant-slot path can reconnect again. Please transition the lifecycle state independently of the queued clients and clear or disable vacant-slot recovery when closing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix SessionPool blocking indefinitely after a server outage 修复服务端断开后 SessionPool 永久阻塞的问题 [iotdb-client-csharp]
Posted by "CritasWang (via GitHub)" <gi...@apache.org>.
CritasWang commented on code in PR #62:
URL: https://github.com/apache/iotdb-client-csharp/pull/62#discussion_r3688065208
##########
src/Apache.IoTDB/SessionPool.cs:
##########
@@ -172,10 +205,61 @@ protected internal SessionPool(List<string> nodeUrls, string username, string pa
_certificatePath = certificatePath;
_sqlDialect = sqlDialect;
_database = database;
+ _poolWaitTimeoutInMs = poolWaitTimeoutInMs;
+ }
+ /// <summary>
+ /// Acquires a client from the pool. If the pool has no idle client but owns vacant slots left behind
+ /// by earlier failed reconnections, one of those slots is re-materialized on the spot instead of
+ /// blocking on a queue that nobody will ever feed. This is what lets the pool recover on its own
+ /// after the server has been unreachable for a while.
+ /// </summary>
+ private async Task<Client> AcquireClientAsync(CancellationToken cancellationToken = default)
+ {
+ if (_clients.ClientQueue.IsEmpty && TryReserveVacantSlot())
Review Comment:
You are right, and the documentation I wrote was simply wrong. Fixed in f855e96.
Of the two options you offered I took the second — **explicitly define this as demand-driven capacity** — rather than eagerly refilling. Reasoning:
- Eager refill needs either a background thread or a synchronous burst of `poolSize - 1` connection attempts on the first successful request after an outage. A background thread is a significant design addition for an SDK that currently has none, and the synchronous burst would put a latency spike on exactly the request that just recovered.
- Demand-driven growth is the standard pool semantic (cf. HikariCP's `minimumIdle` vs `maximumPoolSize`): capacity is a ceiling, not a target. Under real load the queue does empty, so capacity grows to match demand on its own. The pathological case you describe — one connection serving everything — is precisely the case where the extra connections would sit idle anyway.
What changed:
- `_vacantSlots` / `VacantSlots` XML docs now say it measures unrealized capacity, that a steady non-zero value under light load is normal, and that it does **not** indicate server availability.
- The metrics table entry changed from "Alert if > 0 and not returning to 0" to "Not an alert signal on its own".
- The section formerly titled "Self-healing behaviour" is now "Capacity is demand-driven" and states outright that refill happens only when an acquisition finds the idle queue empty, so `VacantSlots` legitimately stays above zero after full recovery.
- Alerting guidance now points at `FailedReconnections`, which only increases when a reconnection actually fails and is therefore the correct reachability signal.
- Scenario 2's symptom list clarifies that `FailedReconnections`, not `VacantSlots`, is the outage signal.
If you would prefer eager refill after all, or think the name `VacantSlots` still reads as "something is broken" and should be something like `UnrealizedCapacity`, I am happy to change it — the rename is cheap right now, before the property ships.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix SessionPool blocking indefinitely after a server outage 修复服务端断开后 SessionPool 永久阻塞的问题 [iotdb-client-csharp]
Posted by "CritasWang (via GitHub)" <gi...@apache.org>.
CritasWang commented on code in PR #62:
URL: https://github.com/apache/iotdb-client-csharp/pull/62#discussion_r3688063840
##########
src/Apache.IoTDB/SessionPool.cs:
##########
@@ -372,6 +462,16 @@ public async Task<Client> Reconnect(Client originalClient = null, CancellationTo
throw new ReconnectionFailedException("Error occurs when reconnecting session pool. Could not connect to any server");
}
+ /// <summary>
+ /// Indicates whether this pool has been opened and not yet closed by the caller.
+ /// </summary>
+ /// <remarks>
+ /// This reflects the lifecycle of the pool object only - it is NOT a server-connectivity probe.
+ /// The client performs no heartbeat, so a server going down does not flip this back to false;
+ /// it stays true until <see cref="Close"/> is called. To reason about connectivity, use
Review Comment:
Good catch — confirmed and fixed in f855e96. This was a pre-existing defect that my vacant-slot change made materially worse: previously an empty queue after an outage just meant "nothing to close", but with capacity retention the pool could still reconnect after `Close()` returned.
`Close()` now transitions the lifecycle state before touching the queue, and disarms capacity refill:
```csharp
if (_isClose) return;
_isClose = true;
Volatile.Write(ref _vacantSlots, 0);
foreach (var client in _clients.ClientQueue.AsEnumerable()) { ... }
```
`AcquireClientAsync` additionally checks `!_isClose` before reserving a slot, so a concurrent operation cannot materialize a connection during or after teardown.
Regression tests added in `SessionPoolConfigurationTests`:
- `Close_EmptyClientQueue_StillMarksThePoolClosed` — sets up a pool with an empty queue and eight vacant slots, then asserts `IsOpen()` is false and `VacantSlots` is zero after `Close()`.
- `Close_IsIdempotentWhenQueueIsEmpty` — a second `Close()` on an empty queue is a no-op.
Both use reflection to construct the post-outage state, since reaching it organically requires a live server that then disappears.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix SessionPool blocking indefinitely after a server outage 修复服务端断开后 SessionPool 永久阻塞的问题 [iotdb-client-csharp]
Posted by "HTHou (via GitHub)" <gi...@apache.org>.
HTHou merged PR #62:
URL: https://github.com/apache/iotdb-client-csharp/pull/62
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org