Skip to content

Commit 3db873f

Browse files
christkvchristkv
committed
Fixes findAndModify memory leak on errors
1 parent e1af41a commit 3db873f

File tree

7 files changed

+101
-36
lines changed

7 files changed

+101
-36
lines changed

‎HISTORY‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,7 @@
1+
1.3.9 2013-06-05
2+
----------------
3+
- Fixed memory leak when findAndModify errors out on w>1 and chained callbacks not properly cleaned up.
4+
15
1.3.8 2013-05-31
26
----------------
37
- Fixed issue with socket death on windows where it emits error event instead of close event (Issue #987)

‎lib/mongodb/connection/base.js‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -403,8 +403,16 @@ Base.prototype._callHandler = function(id, document, err) {
403403
if(this._callBackStore.listeners(id).length >= 1) {
404404
// Get info object
405405
var info = this._callBackStore._notReplied[id];
406-
// Delete the current object
407-
delete this._callBackStore._notReplied[id];
406+
// // Remove any chained callbacks
407+
// if(Array.isArray(info.chained)) {
408+
// for(var i = 0; i < info.chained.length; i++) {
409+
// delete this._callBackStore._notReplied[info.chained[i]];
410+
// }
411+
// } else {
412+
// Delete the current object
413+
delete this._callBackStore._notReplied[id];
414+
// }
415+
408416
// Emit to the callback of the object
409417
this._callBackStore.emit(id, err, document, info.connection);
410418
}

‎lib/mongodb/connection/server.js‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -483,6 +483,13 @@ Server.prototype.connect = function(dbInstance, options, callback) {
483483
// Check for an error, if we have one let's trigger the callback and clean up
484484
// The chained callbacks
485485
if(firstResult[0].err != null || firstResult[0].errmsg != null) {
486+
// Cleanup the remaining callback
487+
if(Array.isArray(callbackInfo.info.chained)) {
488+
for(var i = 0; i < callbackInfo.info.chained.length; i++) {
489+
if(callbackInfo.info.chained[i] != mongoReply.responseTo)
490+
server._removeHandler(callbackInfo.info.chained[i]);
491+
}
492+
}
486493
// Trigger the callback for the error
487494
server._callHandler(mongoReply.responseTo, mongoReply, null);
488495
} else {
@@ -495,7 +502,7 @@ Server.prototype.connect = function(dbInstance, options, callback) {
495502
for(var i = 0; i < chainedIds.length; i++) server._removeHandler(chainedIds[i]);
496503
// Call the handler
497504
server._callHandler(mongoReply.responseTo, callbackInfo.info.results.shift(), null);
498-
} else{
505+
} else{
499506
// Add the results to all the results
500507
for(var i = 0; i < chainedIds.length; i++) {
501508
var handler = server._findHandler(chainedIds[i]);

‎test/runners/replicaset_runner.js‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -42,14 +42,12 @@ module.exports = function(configurations) {
4242
, '/test/tests/repl_set/read_preferences_tests.js'
4343
, '/test/tests/repl_set/read_preferences_spec_tests.js'
4444
, '/test/tests/repl_set/failover_query_tests.js'
45+
, '/test/tests/repl_set/replicaset_examples_tests.js'
4546
]
4647
);
4748

4849
// After each test is done
4950
repl_set_parallel_tests_runner.on('test_done', function(test_statistics) {
50-
console.log("==================================== TEST_DONE")
51-
console.log("==================================== TEST_DONE")
52-
console.log("==================================== TEST_DONE")
5351
// Unpack statistics
5452
var time_spent = test_statistics.end_time.getTime() - test_statistics.start_time.getTime();
5553
var test = test_statistics.name;
@@ -78,9 +76,6 @@ module.exports = function(configurations) {
7876

7977
// After test suite is finished
8078
repl_set_parallel_tests_runner.on('end', function() {
81-
console.log("==================================== TEST_END")
82-
console.log("==================================== TEST_END")
83-
console.log("==================================== TEST_END")
8479
for(var name in buckets) {
8580
var tests = buckets[name];
8681
var total_time = 0;

‎test/tests/functional/find_tests.js‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -684,6 +684,28 @@ exports.shouldCorrectlyFindAndModifyWithNoGetLastErrorChainedW0 = function(confi
684684
});
685685
}
686686

687+
/**
688+
* Test findAndModify a document with no write concern set
689+
* @ignore
690+
*/
691+
exports.shouldCorrectlyFindAndModifyWithNoGetLastErrorChainedW2 = function(configuration, test) {
692+
var client = configuration.db();
693+
client.createCollection('shouldCorrectlyFindAndModifyWithNoGetLastErrorChained', function(err, collection) {
694+
// Let's modify the document in place
695+
collection.findAndModify({'a':1}, [['a', 1]], {'$set':{'b':3}}, {'new':true, 'fields': {a:1}, w:2}, function(err, updated_doc) {
696+
// Check if we have a chained command or not
697+
var ids = client.serverConfig._callBackStore.notRepliedToIds();
698+
test.equal(0, ids.length);
699+
test.done();
700+
});
701+
702+
// Check if we have a chained command or not
703+
var ids = client.serverConfig._callBackStore.notRepliedToIds();
704+
test.equal(2, ids.length);
705+
test.ok(client.serverConfig._callBackStore.callbackInfo(ids[0].chained) == undefined);
706+
});
707+
}
708+
687709
exports.shouldCorrectlyFindAndModifyWithNoGetLastErrorChainedSafe = function(configuration, test) {
688710
var client = configuration.db();
689711
client.createCollection('shouldCorrectlyFindAndModifyWithNoGetLastErrorChainedSafe', function(err, collection) {
@@ -694,7 +716,8 @@ exports.shouldCorrectlyFindAndModifyWithNoGetLastErrorChainedSafe = function(con
694716

695717
// Check if we have a chained command or not
696718
var ids = client.serverConfig._callBackStore.notRepliedToIds();
697-
test.equal(1, ids.length);
719+
// test.equal(1, ids.length);
720+
console.dir(client.serverConfig._callBackStore._notReplied)
698721
test.ok(client.serverConfig._callBackStore.callbackInfo(ids[0].chained) == undefined);
699722
});
700723
}

‎test/tests/manual_tests/find_modify_break_test.js‎

Lines changed: 29 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1,24 +1,26 @@
1-
var mongodb = require("../../lib/mongodb"),
2-
request = true;
3-
4-
var db = new mongodb.Db('test_db', new mongodb.Server("127.0.0.1", 27017, {
5-
auto_reconnect: true
6-
}), {});
7-
8-
// listen on error
9-
db.on("error", function(err) {
10-
console.log('open request ', request);
11-
console.error('db on error');
12-
console.dir(err);
13-
});
1+
var mongodb = require("../../../lib/mongodb")
2+
, MongoClient = mongodb.MongoClient
3+
, request = true;
4+
5+
// var db = new mongodb.Db('test_db', new mongodb.Server("127.0.0.1", 27017, {
6+
// auto_reconnect: true
7+
// }), {});
8+
9+
// // listen on error
10+
// db.on("error", function(err) {
11+
// console.log('open request ', request);
12+
// console.error('db on error');
13+
// console.dir(err);
14+
// });
1415

1516
// open connection
16-
db.open(function(err, client) {
17+
// db.open(function(err, client) {
18+
MongoClient.connect('mongodb://127.0.0.1:31000/test_db', function(err, db) {
1719
if (err) {
1820
console.error(err);
1921
}
2022

21-
var collection = new mongodb.Collection(client, 'test_collection');
23+
var collection = db.collection('test_collection');
2224

2325
// define find and modify
2426
var findAndModifyLoop = function() {
@@ -27,7 +29,7 @@ db.open(function(err, client) {
2729

2830
console.log('findAndModify request (should not be last)');
2931

30-
collection.findAndModify({hello: 'world'}, [['_id', 'asc']], {$set: {hi: 'there'}},{w:1, upsert:true}, function(err, object) {
32+
collection.findAndModify({hello: 'world'}, [['_id', 'asc']], {$set: {hi: 'there'}},{w:5, wtimeout:1000, upsert:true}, function(err, object) {
3133
if (err) {
3234
console.warn('findAndModify error response ', err.message); // returns error if no matching object found
3335
} else {
@@ -38,6 +40,7 @@ db.open(function(err, client) {
3840
request = false;
3941

4042
process.nextTick(function() {
43+
console.dir("number of callbacks :: " + Object.keys(db.serverConfig._callBackStore._notReplied).length);
4144
// on result does it again
4245
findAndModifyLoop();
4346
})
@@ -48,14 +51,14 @@ db.open(function(err, client) {
4851
findAndModifyLoop();
4952
});
5053

51-
db.on("error", function(err) {
52-
console.log('open request ', request);
53-
console.error('db on error');
54-
console.dir(err);
55-
});
54+
// db.on("error", function(err) {
55+
// console.log('open request ', request);
56+
// console.error('db on error');
57+
// console.dir(err);
58+
// });
5659

57-
db.on("close", function(err) {
58-
console.log('open request ', request);
59-
console.error('db on close');
60-
console.dir(err);
61-
});
60+
// db.on("close", function(err) {
61+
// console.log('open request ', request);
62+
// console.error('db on close');
63+
// console.dir(err);
64+
// });

‎test/tests/repl_set/replicaset_examples_tests.js‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,3 +74,28 @@ exports['Connection to replicaset with secondary only read preference no seconda
7474
// DOC_END
7575
}
7676

77+
/**
78+
* Test findAndModify a document with no write concern set
79+
* @ignore
80+
*/
81+
exports.shouldCorrectlyFindAndModifyWithNoGetLastErrorChainedW2 = function(configuration, test) {
82+
var client = configuration.db();
83+
client.createCollection('shouldCorrectlyFindAndModifyWithNoGetLastErrorChained', function(err, collection) {
84+
// Let's modify the document in place
85+
collection.findAndModify({'a':1}, [['a', 1]], {'$set':{'b':3}}, {'new':true, 'fields': {a:1}, w:5, wtimeout:1000}, function(err, updated_doc) {
86+
console.dir(err)
87+
console.dir(updated_doc)
88+
// Check if we have a chained command or not
89+
var ids = client.serverConfig._callBackStore.notRepliedToIds();
90+
test.equal(0, ids.length);
91+
test.done();
92+
});
93+
94+
// Check if we have a chained command or not
95+
var ids = client.serverConfig._callBackStore.notRepliedToIds();
96+
test.equal(2, ids.length);
97+
test.ok(client.serverConfig._callBackStore.callbackInfo(ids[0].chained) == undefined);
98+
});
99+
}
100+
101+

0 commit comments

Comments
 (0)