fix: clear timeout in collection operations to prevent memory leak #15852
Conversation
- Add clearTimeout() calls in success and error paths - Prevents setTimeout references from accumulating - Fixes memory leak in high-throughput applications - Add test to verify timeout cleanup behavior Fixes memory leak where buffered collection operations would create setTimeout timers that were never cleared when operations completed successfully, causing continuous memory growth.
vkarpov15
left a comment
vkarpov15
left a comment
There was a problem hiding this comment.
clearTimeout looks good but there's some unrelated changes that are incorrect
|
|
||
| if (this._readyState !== val) { | ||
| this._readyState = val; | ||
| // [legacy] loop over the otherDbs on this connection and change their state |
There was a problem hiding this comment.
@vkarpov15 I've reverted all the unrelated otherDbs changes as requested. The PR now contains only the setTimeout memory leak fix with clearTimeout() calls added to the success and error paths.
Is this ready to merge, or are there any other changes needed?
Keep only the setTimeout memory leak fix as requested by maintainers. Revert changes to connection.js that were unrelated to the timeout issue.
Move timeout variable declaration to function scope to ensure it's accessible in all success/error paths for proper cleanup.
| conn._lastHeartbeatAt = Date.now(); | ||
| for (const otherDb of conn.otherDbs) { | ||
| otherDb._lastHeartbeatAt = conn._lastHeartbeatAt; | ||
| for (const otherDb of Object.values(conn.relatedDbs)) { |
|
|
||
| for (const db of this.otherDbs) { | ||
| _setClient(db, client, {}, db.name); | ||
| for (const db of Object.values(this.relatedDbs)) { |
| db.$wasForceClosed = true; | ||
|
|
||
| // Remove from all related connections' relatedDbs | ||
| for (const relatedDb of Object.values(this.relatedDbs)) { |
| NativeConnection.prototype.removeDb = function removeDb(name) { | ||
| const dbs = this.otherDbs.filter(db => db.name === name); | ||
| if (!dbs.length) { | ||
| const db = this.relatedDbs[name]; |
| if (options && options.useCache) { | ||
| this.relatedDbs[newConn.name] = newConn; | ||
| newConn.relatedDbs = this.relatedDbs; | ||
| // Add to relatedDbs for state synchronization and caching |
test/collection.timeout.test.js
Outdated
| const assert = require('assert'); | ||
| const mongoose = require('../index'); | ||
|
|
||
| describe('Collection timeout cleanup', function() { |
There was a problem hiding this comment.
Move these tests into collection.test.js
- Add clearTimeout() calls in success/error paths to prevent memory leaks - Move timeout variable to function scope for proper cleanup access - Revert unrelated otherDbs/relatedDbs changes as requested by maintainer - Move tests to collection.test.js as requested
|
Hi @vkarpov15! Thank you for the detailed feedback. I've addressed all your comments: ✅ Reverted all unrelated otherDbs/relatedDbs changes in connection.js back to original implementation The PR is now focused and clean. I really appreciate your guidance and would love to get this memory leak fix MERGED! |
2 participants
Fixes memory leak where buffered collection operations would create setTimeout timers that were never cleared when operations completed successfully, causing continuous memory growth.
Summary
Examples