apache / apache/iotdb-client-nodejs

Session pool follow-ups: minPoolSize: 0, pending waiters on close(), and a failed idle-session close

オープン
#22 コメント 0 件 リアクション 0 件 担当者 0 名 GitHub で見る
主要言語
TypeScript
スター
3
フォーク
2
平均マージ
7分
マージ済み PR(30日)
2

説明

## Summary

Three non-blocking observations about `BaseSessionPool` that came out of the reviews of #19 and #20. None of them is a crash or a leak on the normal path, so I did not fold them into those PRs; @CritasWang suggested collecting them in a single tracking issue.

Line references are against `develop` at a8ca4d2.

## 1. `minPoolSize: 0` is silently coerced to 1

`BaseSessionPool.ts:129` (and the cleanup floor at `:373`) read the setting as:

```ts
const minSize = this.config.minPoolSize || 1;
```

`0 || 1` is `1`, so a caller who explicitly asks for `minPoolSize: 0` gets one eagerly created session at `init()`, and `cleanupIdleSessions` then keeps one session alive forever rather than letting the pool drain to empty. In most pool implementations `0` means "create nothing up front, and allow the pool to go back to empty when idle", which is a reasonable ask for short-lived or cost-sensitive processes.

If `0` is intended to be legal, `??` instead of `||` would express it. If it is not intended to be legal, rejecting it in config validation would be clearer than silently changing it.

## 2. `close()` clears the wait queue without settling the waiters

`close()` at `:505` ends with `this.waitQueue.clear()` (`:525`). The queued entries are the waiter callbacks themselves, so clearing the queue drops them without ever resolving or rejecting their promises.

A caller that is inside `await pool.getSession()` when another part of the application closes the pool therefore does not fail fast. It stays pending until the acquire timeout fires (`waitTimeout`, default 60000ms at `:290`) and then rejects with `Timeout waiting for available session` (`:320`) — which points at pool exhaustion rather than at the real cause. Rejecting each queued waiter during `close()` with a "pool is closed" error would make both the latency and the message correct.

## 3. A failed `session.close()` during idle cleanup orphans the connection

`cleanupIdleSessions` (`:391` onward) deliberately removes a session from `pool` and `idleSessions` *before* awaiting `close()`, which is correct — it prevents a concurrent `getSession()` from handing out a session that is about to be destroyed. But if `close()` rejects, the error is logged (`:409`) and nothing else happens: the session is already out of every pool structure, so its underlying connection is no longer referenced, never retried, and never counted anywhere.

That is not a leak in the common case — the RPC usually fails because the connection is already gone — but a transport-level failure would leave a real socket open with no owner. Keeping the session in a small "failed to close" set for a later retry, or at least counting it in a metric, would make the failure visible.

## Not included

The `getSession()` create-branch race that also came up in the #19 review is already fixed on `develop` (`:266-276` now locates the session by identity instead of `shift()`ing the front of the idle queue), so there is nothing left to track there.

I am happy to send a PR for any of these — please let me know which are worth changing and whether you would prefer them separately.

コントリビューションガイド

コントリビューションガイドを開く

調査の方向性

BaseSessionPool.ts の 129 行目と 373 行目付近にある minPoolSize の処理、505 行目付近の close()、391 行目付近の cleanupIdleSessions から始めます。既存の acquire と cleanup のフローを確認し、そのうえで、サイズ 0 のプール、close() 中にキューで待機している waiter、失敗した idle sessions のクローズについて、対象を絞ったカバレッジを追加します。完了条件は、各動作に明示的な結果があり、waiter も失敗した close も暗黙に放置されないことです。

索引モデルが issue の本文から書いたものです。

評価

技術スタック
nodejs, typescript
領域
backend, databases
issue の種類
バグ
難易度
4/5
見積もり時間
3〜5日
活発さ
静か
明瞭さ
おおむね明確
初心者へのやさしさ
48/100

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。