feat: implement COM_RESET_CONNECTION with pool integration#4148
feat: implement COM_RESET_CONNECTION with pool integration#4148
Conversation
Implements COM_RESET_CONNECTION (0x1F) command based on PR #1437. This provides a lightweight way to reset connection session state without re-authentication or closing the TCP connection. Key changes: - Add ResetConnection command and packet classes - Add reset() method to Connection API - Add promise wrapper for reset() - Clear prepared statements cache on reset - Add TypeScript definitions Benefits: - Faster than COM_CHANGE_USER (no re-authentication) - Cleans up session state (variables, temp tables, locks) - Useful for connection pool cleanup - Requires MySQL 5.7.3+ Related: #1437, #1328
…leanup Implements automatic connection reset when connections are released back to the pool, addressing issues #934, #1087, and #1469. Key changes: - Add `resetOnRelease` config option to PoolConfig (default: true) - Modify releaseConnection() to call reset() before returning to pool - Handle reset failures by destroying connection and creating new one - Add comprehensive integration tests for reset functionality - Add pool tests verifying reset behavior with resetOnRelease on/off Benefits: - Prevents session state leakage between pool users (security) - Clears user variables, temp tables, transactions, locks automatically - No-surprises default: each connection starts fresh - Optional opt-out for performance-critical scenarios - Prepared statements cache properly cleared Tests: - Connection reset basic functionality - User variable cleanup - Temporary table cleanup - Prepared statements cache clearing - Transaction rollback on reset - Pool reset on release (default behavior) - Pool without reset (resetOnRelease: false) - Error handling for failed resets - Promise wrapper support - Queued connection reset behavior Related: #934, #1087, #1328, #1437, #1469
- Fix Prettier formatting in test files - Add proper TypeScript types for PoolConnection - Use type assertions for private _statements property - Fix import order per Prettier rules
Some existing tests check internal pool state immediately after releasing connections. With resetOnRelease enabled by default, these tests fail because connections go through async reset before being added to the free connections queue. Fixed tests: - test-pool-release-idle-connection*.test.mts (3 tests) - dispose/sync/pool.test.mts - dispose/async/pool.test.mts - test-parameters-questionmark.test.mts (uses temp tables) These tests are specifically testing idle timeout behavior and dispose mechanics, not reset behavior, so resetOnRelease: false is appropriate.
- Use 'unknown' intermediate type for strict type assertions - Fix reset function signature to match QueryError type - Import QueryError type in pool reset tests All TypeScript checks now pass locally.
- Change function parameter return type from 'any' to 'void' - Fix prettier formatting for multi-line error callback - Resolves eslint no-explicit-any rule violation
There was a problem hiding this comment.
Pull request overview
Implements MySQL COM_RESET_CONNECTION support and integrates it into pooling so session state is cleared automatically when a connection is released back to the pool.
Changes:
- Added
connection.reset()/PromiseConnection.reset()APIs and protocol support forCOM_RESET_CONNECTION(0x1F). - Added
resetOnReleasepool option (defaulttrue) and updated pool release flow to reset state (or destroy on failure). - Added integration tests for reset behavior and pool integration; adjusted existing pool timing/dispose tests to opt out of resets.
Reviewed changes
Copilot reviewed 17 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| typings/mysql/lib/Pool.d.ts | Adds resetOnRelease to pool options typing/docs. |
| typings/mysql/lib/Connection.d.ts | Adds reset(callback) to connection typings. |
| promise.d.ts | Adds promise reset() typing. |
| lib/promise/connection.js | Implements promise wrapper for reset(). |
| lib/constants/commands.js | Introduces RESET_CONNECTION command code. |
| lib/packets/reset_connection.js | Adds packet builder for COM_RESET_CONNECTION. |
| lib/packets/index.js | Registers ResetConnection packet. |
| lib/commands/reset_connection.js | Adds command implementation and clears statement cache on reset. |
| lib/commands/index.js | Exports the reset command. |
| lib/base/connection.js | Adds BaseConnection.reset() entry point. |
| lib/pool_config.js | Adds resetOnRelease config parsing and default. |
| lib/base/pool.js | Resets connections on release and destroys connections on reset failure. |
| test/integration/connection/test-reset-connection.test.mts | Adds reset integration tests (variables, temp tables, statements, txns, promise). |
| test/integration/pool/test-pool-reset-on-release.test.mts | Adds pool reset-on-release integration tests. |
| test/integration/test-pool-release-idle-connection*.test.mts | Disables reset for timing-sensitive idle timeout tests. |
| test/integration/dispose/**/pool.test.mts | Disables reset to keep dispose-related tests deterministic. |
| test/integration/connection/test-parameters-questionmark.test.mts | Disables reset for this pool-based test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Changed resolve(true) to resolve() to match the Promise<void> type definition and align with similar commands like changeUser(). Convention analysis: - ping() - resolves with true, typed as Promise<void> (existing inconsistency) - connect() - resolves with nothing, typed as Promise<void> (correct) - changeUser() - resolves with nothing, typed as Promise<void> (correct) - reset() - now resolves with nothing, typed as Promise<void> (correct) Since reset() is a state management command like changeUser(), it should follow the same pattern of resolving with no value.
|
Hey @sidorares, for the tests that never finish, please see the examples in PR #4094's description 🙋🏻♂️ |
Copilot suggestions: - Use Buffer.alloc(5) instead of Buffer.allocUnsafe(5) for security (prevents potential memory leak of uninitialized buffer contents) - Pass null explicitly to callback on success (not undefined) Aligns with Node.js callback convention: callback(err, result) wellwelwel suggestions: - Replace complex type assertions with @ts-expect-error comments Simpler, clearer, and easier to track for future typing improvements - Add try/finally blocks to ensure connection.end() is called Prevents hanging tests when assertions fail Changes: - lib/packets/reset_connection.js: Buffer.allocUnsafe → Buffer.alloc - lib/commands/reset_connection.js: onResult.bind(this) → onResult.bind(this, null) - test files: Complex type assertions → @ts-expect-error - test files: Added try/finally for proper connection cleanup
The getConfig() function in test/common.test.mts was not passing through
the resetOnRelease option, causing all tests that explicitly set
resetOnRelease: false to actually get resetOnRelease: true (the default).
This caused 80 test failures because:
- Tests expected immediate synchronous release (resetOnRelease: false)
- Actually got async reset behavior (resetOnRelease: true)
- Timing-sensitive assertions failed
- Some tests hung when assertions failed before pool cleanup
Root cause identified through debug logging:
- Test sets: { resetOnRelease: false }
- Debug shows: resetOnRelease: true
- Config was being stripped by getConfig()
Verified fix locally:
- test-pool-release-idle-connection*.test.mts (3 files) - all pass
- dispose/sync/pool.test.mts - passes
- dispose/async/pool.test.mts - passes
This single line addition fixes all 80 failing test runs.
TypeScript error: Property 'resetOnRelease' does not exist on type 'ConnectionOptions' resetOnRelease is a PoolOptions property, not ConnectionOptions. Cast to PoolOptions when accessing pool-specific properties. This follows the same pattern as connectionLimit, maxIdle, idleTimeout which are also PoolOptions properties.
|
Bringing #4154, #4170, #4185, and #4188 here. By forcing a timeout directly in the tests, each test that exceeds the limit now fails and displays the errors instead of hanging indefinitely. This will help to see reports that were not previously displayed since the processes were never closed (the tests continue to wait for an error ( Real case: https://github.com/sidorares/node-mysql2/actions/runs/22732532589/job/65925146713?pr=4148 |
|
@wellwelwel I'm thinking to make 'resetOnRelease' default off, at least for this change |
Just yesterday I fixed an unexpected breaking change conflict from 2024 caused by a change in the instance source of a private class from So I agree with the idea of not breaking the behavior without a major semver 🙋🏻♂️ |
Change resetOnRelease default from true to false so this feature can ship without a major version bump. Revert unrelated test modifications that only existed to work around the true default. Update the first pool reset test to explicitly opt in with resetOnRelease: true.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4148 +/- ##
==========================================
+ Coverage 90.82% 90.87% +0.05%
==========================================
Files 87 89 +2
Lines 14390 14498 +108
Branches 1840 1862 +22
==========================================
+ Hits 13069 13175 +106
- Misses 1321 1323 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Replace verbose `as unknown as` type casts with idiomatic `@ts-expect-error: internal access` pattern. Restructure tests to use describe-level connection scoping so connection.end() always runs even if an assertion fails inside it().
wellwelwel
left a comment
There was a problem hiding this comment.
@sidorares, do you think it's worth including this option/flag in the documentation (a small paragraph in Extra Features, for example)? Otherwise, LGTM 🙋🏻♂️
definitely, I thought I added it, will do now |
Add new documentation page covering connection.reset() with promise/callback examples, the resetOnRelease pool option, error handling, and MySQL version compatibility. Link from the documentation index and update the prepared-statements page to reference reset().
ec3c690 to
dc8da19
Compare
This PR implements MySQL's
COM_RESET_CONNECTIONcommand (introduced in MySQL 5.7.3) and integrates it with connection pools via a newresetOnReleasepool option.This addresses 7-year-old issue #934 and resolves issues #1087, #1328, and #1469.
Motivation
Problem
When connections are reused from a pool, session state persists between different users:
This creates:
Current Workarounds (Suboptimal)
changeUser()with no params: Requires full re-authentication (slow, extra roundtrip)Solution
Implements
COM_RESET_CONNECTION(0x1F) - a lightweight command that:Changes
1. Core Command Implementation
ResetConnectioncommand classResetConnectionpacket classconnection.reset(callback)methodawait connection.reset()2. Pool Integration
resetOnReleasepool config option (default:false)releaseConnection()to callreset()when enabled_handleSuccessfulRelease()helper method3. Comprehensive Tests
6 connection reset tests (
test/integration/connection/test-reset-connection.test.mts):7 pool integration tests (
test/integration/pool/test-pool-reset-on-release.test.mts):API Usage
Connection API (added)
Pool API (enhanced)
Why
resetOnReleasedefaults tofalseWhile
resetOnRelease: trueis the more correct and secure behavior for connection pools, we default tofalsefor this initial release to avoid a semver-breaking change. Existing applications may rely on session state (user variables, temporary tables, etc.) persisting across pool connection reuse. Changing this default silently would break those applications.Future plan: In the next major version,
resetOnReleasewill default totrue, as this is the safer and recommended behavior for production applications. We encourage all users to explicitly opt in toresetOnRelease: truenow.What Gets Reset
When
connection.reset()is called (or triggered by pool withresetOnRelease: true):@variablevalues clearedPerformance Impact
Benchmarks (preliminary)
MySQL Version Compatibility
Error handling: If reset fails (e.g., old MySQL version), the connection is destroyed and removed from pool, and a new connection is created for the next request.
Issues Resolved
This PR directly addresses:
Also helps with:
Related PRs
Backward Compatibility
Fully backward compatible - no breaking changes.
resetOnReleasedefaults tofalse, preserving existing pool behaviorconnection.reset()calls are new API, don't affect existing coderesetOnRelease: trueat their discretionChecklist
Credits