CritasWang commented on code in PR #19:
URL:
https://github.com/apache/iotdb-client-nodejs/pull/19#discussion_r3635175592
##########
src/client/BaseSessionPool.ts:
##########
@@ -276,9 +289,31 @@ export abstract class BaseSessionPool {
);
const waitTimeout = this.config.waitTimeout || 60000;
return new Promise((resolve, reject) => {
+ let settled = false;
+
+ // The queue stores this exact wrapper. On timeout we must remove *this*
+ // reference (not `resolve`, which is never in the queue), and the
+ // `settled` guard makes timeout and fulfillment mutually exclusive so a
+ // session is never handed to a waiter whose promise already rejected.
+ const waiter = (session: Session): boolean => {
+ if (settled) {
+ return false;
+ }
+ settled = true;
+ clearTimeout(timeoutId);
+ const duration = Date.now() - startTime;
+ logger.debug(`[PERF] getSession (waited): ${duration}ms`);
+ resolve(session);
+ return true;
+ };
Review Comment:
The `settled` guard on both sides (waiter *and* timeout callback) is the
right call — `clearTimeout` can't cancel a timer whose callback is already in
the ready queue, so guarding only one side would leave a small double-settle
window. Nice.
One adjacent gap (pre-existing, not introduced here): `close()` doesn't
reject pending waiters, so a caller parked in this queue when the pool shuts
down only fails after the full `waitTimeout`. Since this PR already gives
waiters a `settled` flag, draining the queue in `close()` (settle + reject each
pending waiter) would be a natural small follow-up.
##########
src/client/BaseSessionPool.ts:
##########
@@ -342,24 +377,33 @@ export abstract class BaseSessionPool {
const idleArray = this.idleSessions.toArray();
for (const ps of idleArray) {
- if (now - ps.lastUsed > maxIdleTime && this.pool.length > minSize) {
+ // Subtract already-queued removals so the pool never drops below
+ // minPoolSize: without this the guard sees the constant pre-cleanup size
+ // and can queue every idle session, collapsing the pool to 0.
+ if (
+ now - ps.lastUsed > maxIdleTime &&
+ this.pool.length - sessionsToRemove.length > minSize
+ ) {
sessionsToRemove.push(ps);
}
}
await Promise.all(
sessionsToRemove.map(async (ps) => {
+ // Remove from pool + idle BEFORE closing. close() awaits the
+ // closeSession RPC and isOpen() stays true until it resolves, so a
+ // concurrent getSession() could otherwise shift() this session and
+ // hand out a connection that is about to be destroyed.
+ const poolIndex = this.pool.indexOf(ps);
+ if (poolIndex > -1) {
+ this.pool.splice(poolIndex, 1);
+ }
+ const idleIndex = this.idleSessions.toArray().indexOf(ps);
+ if (idleIndex > -1) {
+ this.idleSessions.remove(idleIndex, 1);
+ }
try {
await ps.session.close();
Review Comment:
Splice-before-close correctly closes the TOCTOU window. One side effect
worth being aware of (acceptable trade-off, no change requested): if `close()`
throws, the session has already been removed from `pool`/`idleSessions`, so it
ends up untracked and possibly not fully closed. That's the standard
connection-pool trade-off — better an orphaned close-failed session than
handing out a dying one — but a comment noting it, or a best-effort
`session.close()` retry in the catch, wouldn't hurt.
##########
src/client/BaseSessionPool.ts:
##########
@@ -261,7 +266,15 @@ export abstract class BaseSessionPool {
const session = await this.createSession();
const pooledSession = this.pool.find((ps) => ps.session === session);
if (pooledSession) {
- this.idleSessions.shift(); // Remove from idle since we just added it
+ // Remove *this* session from idle. createSession() pushed it to the
+ // back of idleSessions; a blind shift() removes the front, which under
+ // concurrent interleaving (a session released into idle while we were
+ // awaiting createSession) would evict a different session and leave
+ // this one double-tracked as both idle and active.
+ const idleIndex = this.idleSessions.toArray().indexOf(pooledSession);
+ if (idleIndex > -1) {
+ this.idleSessions.remove(idleIndex, 1);
+ }
Review Comment:
The targeted removal is correct and fixes the wrong-eviction bug. While
verifying it I noticed a related pre-existing race that this PR neither
introduces nor fixes (out of scope, flagging for a follow-up issue):
`createSession()` pushes the new session into `idleSessions` *before* it
resolves, so between that push and this removal there is a microtask window in
which a concurrent `getSession()` can grab the same session via the idle-reuse
branch — two callers would then share one connection. A cleaner long-term shape
might be for the create branch to claim the new session directly without
routing it through `idleSessions` at all. Happy to open a separate issue for it
so this PR stays focused.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
