Fix/317 pool null connection#408
Merged
Merged
Conversation
A statement that fails during dispatch raises both 'error' and 'done'
(lib/reader.js emits error, then done, then free). heartbeat() acted on
both: 'error' triggered recreate() -> checkin and 'done' triggered a
second checkin of the same PoolDescription, so it appeared twice in the
idle list.
park() then closed and nulled description.connection through one of the
two references while the duplicate stayed in idle. The next heartbeat or
query checked out that stale duplicate and dereferenced a null
connection:
TypeError: Cannot read properties of null (reading 'query')
at heartbeat (lib/pool.js)
This also explains why raising inactivityTimeoutSecs did not help the
reporter - it delays parking but does not stop the duplication.
Guard both ends: a 'settled' flag so only the first of error/done
returns a heartbeat description to the pool, and a checkin() guard that
refuses a description already present in idle or parked, making the
invariant structural rather than dependent on every call site.
Adds test/pool-description-lifecycle.test.js, which stubs the driver so
the error+done ordering is deterministic and needs no server. Both tests
fail with the exact reported error when the fix is reverted.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Two further defects found while investigating #317. Neither causes that crash, but both are plainly unintended. close(): `idle.map(d => d.connection.promises.close)` mapped the method without invoking it, so Promise.all resolved over an array of functions and every idle connection was left open at pool shutdown. Commit 03f6cb3 ("fix bug in pool when closing not calling method close") addressed the driver.js instance but missed this one. The same line's `.finally(closed = true)` evaluated its argument eagerly rather than passing a callback; `closed` is now set explicitly and synchronously, preserving the existing behaviour that work submitted during shutdown is rejected immediately. promotePause(): tested `pause[i].isPaused`, a property PoolWorkItem does not have - the paused state lives on `poolNotifier.isPaused()`. The predicate was therefore always true, so every paused item was promoted to the work queue and immediately pushed back by crank(), ping-ponging between the two arrays on each poll tick with a wasted setImmediate each time. Behaviour was correct because crank() re-checks, but the churn was not. Adds a test asserting pool.close() closes its idle connections; it fails against the previous implementation. Full suite: 732 passing, unchanged from before these fixes. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes for connection pool null connection