fix: resolve parser cache collision with dual typeCast connections #3644
Conversation
How to reproduce the errorYou can use the following code snippet to reproduce the error import mysql from 'mysql2/promise';
// Pool 1 - typeCast: true
const pool1 = mysql.createPool({
host: '<database 1 host>',
port: 3306,
user: '<database 1 user>',
password: '<database 1 password>',
database: '<database 1 name>',
connectionLimit: 3,
typeCast: true
});
// Pool 2 - typeCast: false
const pool2 = mysql.createPool({
host: '<database 2 host>',
port: 3306,
user: '<database 2 user>',
password: '<database 2 password>',
database: '<database 2 name>',
connectionLimit: 5,
typeCast: false
});
// Query for pool 1
function runQueryUser1() {
console.log('Creating connection User1');
return pool1.getConnection()
.then(connection => {
const queryString = 'SELECT 1;';
return connection.query(queryString)
.then(([rows]) => {
console.log(`User1 (typeCast: ${connection.config.typeCast} ) Result:`, JSON.stringify(rows));
})
.catch(error => {
console.error('User1 Query Error:', error);
})
.finally(() => {
console.log('Releasing connection');
connection.release();
});
})
.catch(error => {
console.error('User1 Connection Error:', error);
});
}
// Query for pool 2
function runQueryUser2() {
console.log('Creating connection User2');
return pool2.getConnection()
.then(connection => {
const queryString = 'SELECT 1;';
return connection.query(queryString)
.then(([rows]) => {
console.log(`User2 (typeCast: ${connection.config.typeCast} ) Result:`, JSON.stringify(rows));
})
.catch(error => {
console.error('User2 Query Error:', error);
})
.finally(() => {
console.log('Releasing connection');
connection.release();
});
})
.catch(error => {
console.error('User2 Connection Error:', error);
});
}
// Run queries with proper error handling
async function runQueries() {
await runQueryUser1();
await runQueryUser2();
process.exit(0);
}
// Run the queries
runQueries(); |
|
Hi, @MOHIT51196! Thanks for the issue + PR 🙋🏻♂️ We have a test to check the parser cache key serialization in test/esm/unit/parsers/cache-key-serialization.test.mjs, where it try the With your implementation, it would be useful to pick one of the 6 tests that repeats the value as Your reproduction example is a great case to validate the behavior, but in practice I think it's more practical to use the existing test. What do you think? |
Updating the existing test cases will keep them updated with the change, hence its necessary but in my opinion new test file is also necessary to verify the flow and also better for hardening tests. |
@MOHIT51196, I'll also reply to your private message here, because I believe it applies to the entire community of contributors and users of the project. My doubt at the moment is whether this problem only applies to
Note Regarding giving priority to issues or reviews, I'd like to point out that our work is entirely voluntary 🤝 About testsThe point of inserting a test that relies on On the other hand, the current tests seem to cover the scenarios adequately: the significant change is that instead of
About the changesA particular concern would be the flexibility that the current change brings by allowing any value for typeof options.typeCast === 'boolean' ? options.typeCast : typeof options.typeCast
Please, feel free to share your thoughts and ideas 🙋🏻♂️ |
I understand your concern @wellwelwel and hence made some updates to the PR to tackle them. Also I have removed the testcases as existing once seems enough |
|
Thanks, @MOHIT51196!
What I meant was that we can pick one of the tests with About the assertion errors, it's due to some |
The parser cache system had a bug where connections with different typeCast boolean values (true vs false) would share cached parsers, causing incorrect data type conversion. Update `parser_cache.js` with following changes : - Enhanced cache key generation to create unique keys for each typeCast value - Now typeCast: true and typeCast: false generate distinct cache keys - Maintains function-based typeCast compatibility - Ensures proper cache isolation and eliminates ambiguous cache hits Add test to validate changes in `test-typecast-dual-connections.test.cjs` with following changes : - Added test-typecast-dual-connections.test.cjs to validate the fix - Tests concurrent connections with different typeCast boolean settings - Prevents future regressions in parser cache logic Signed-off-by: Mohit Malhotra <[email protected]>
… match parser cache behavior Update test expectations for `typeCast` field to reflect actual implementation where `typeCast` returns the value directly instead of the type. This aligns tests with the `lib/parsers/parser_cache.js` key generation logic. Add more robust check for generating key for parser caching for the field `tyepCast` to ensure value is used for only boolean input. Also removed `test-typecast-dual-connections.test.cjs` as existing testcases are sufficient. Signed-off-by: Mohit Malhotra <[email protected]>
|
Thanks again, @MOHIT51196! These changes are already available in |
Problem
There was a critical bug in the parser cache system (
lib/parsers/parser_cache.js) that caused incorrect type casting when multiple connections used differenttypeCastconfigurations simultaneously.The parser cache generates cache keys based on connection options to reuse parsers for performance. However, the cache key generation for
typeCasthad a flaw that created ambiguous cache keys.Issue: When multiple connections had different
typeCastboolean values (truevsfalse), both would generate the same cache key, causing:typeCast: truewould cache a parsertypeCast: falsewould find the same cache key and reuse the wrong parserReal-World Impact
This bug manifested when:
typeCastboolean settings (truevsfalse)Result: Connections with
typeCast: falsewould incorrectly get typed data instead of raw Buffers, breaking applications that expected raw Buffer data.Root Cause Analysis
The issue was in the
keyFromFieldsfunction inlib/parsers/parser_cache.js. The original cache key generation approach created ambiguous keys:typeCast: true: Generated generic type identifier ❌typeCast: false: Generated same generic type identifier ❌This caused cache collisions between
typeCast: trueandtypeCast: falseconnections because both boolean values were treated identically in the cache key generation process.Solution
Improved Cache Key Generation: Enhanced the cache key generation to create unique identifiers (if
typeCastis boolean) for each distincttypeCastvalue, eliminating ambiguous cache hits.Key Improvements
typeCastvalue (boolean) now generates its own unique cache key and no longer collide in the cacheThe enhanced approach ensures that:
typeCast: truegets its own unique cache keytypeCast: falsegets a different unique cache keyTest Coverage Added
Created
test-typecast-dual-connections.test.cjsto validate the fix and prevent regressions:The test creates two connections with different
typeCastconfigurations and validates that each connection gets the correct data type format, ensuring no cross-connection parser contamination.Breaking Changes
None. This fix maintains backward compatibility while resolving the cache collision bug.
Checklist