Skip to content

Commit

Permalink
Fix connection leaks: - Close connection before reassigning server re…
Browse files Browse the repository at this point in the history
…ference - Fix how servers are iterated when closing
  • Loading branch information
elbert3 authored and christkv committed Sep 10, 2012
1 parent d247ea2 commit f75f560
Showing 1 changed file with 18 additions and 4 deletions.
22 changes: 18 additions & 4 deletions lib/mongodb/connection/repl_set.js
Expand Up @@ -475,6 +475,10 @@ var _connectHandler = function(self, candidateServers, instanceServer) {
}

// Make sure we have the right reference
if (self._state.addresses[instanceServer.host + ":" + instanceServer.port] != instanceServer) {
// Close the connection before deleting
self._state.addresses[instanceServer.host + ":" + instanceServer.port].close();
}
delete self._state.addresses[instanceServer.host + ":" + instanceServer.port];
self._state.addresses[me] = instanceServer;

Expand Down Expand Up @@ -556,6 +560,13 @@ var _connectHandler = function(self, candidateServers, instanceServer) {
server.enableRecordQueryStats(self.recordQueryStats);

// Set the server
if (addresses[server.host + ":" + server.port] != server) {
if (addresses[server.host + ":" + server.port]) {
// Close the connection before deleting
addresses[server.host + ":" + server.port].close();
}
delete addresses[server.host + ":" + server.port];
}
addresses[server.host + ":" + server.port] = server;
// Connect
server.connect(self.db, {returnIsMasterResults: true, eventReceiver:server}, _connectHandler(self, candidateServers, server));
Expand Down Expand Up @@ -659,8 +670,9 @@ ReplSet.prototype.connect = function(parent, options, callback) {

// Get the list of servers that is deduplicated and start connecting
var candidateServers = [];
for(var key in addresses) {
candidateServers.push(addresses[key]);
var keys = Object.keys(addresses);
for(var i = 0; i < keys.length; i++) {
candidateServers.push(addresses[keys[i]]);
}
// Let's connect to the first one on the list
server = candidateServers.pop();
Expand Down Expand Up @@ -942,8 +954,10 @@ ReplSet.prototype.close = function(callback) {
this._serverState = 'disconnected';
// Close all servers
if(this._state && this._state.addresses) {
for(var key in this._state.addresses) {
this._state.addresses[key].close();
var keys = Object.keys(this._state.addresses);
// Iterate over all server instances
for(var i = 0; i < keys.length; i++) {
this._state.addresses[keys[i]].close();
}
}

Expand Down

7 comments on commit f75f560

@sberryman
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any chance this could fix an issue with connections and no activity experiencing some weird issues? I have noticed that after 20 minutes of no activity on any of the connections will result in a future request waiting for ~60 seconds before a failed to connect error is thrown. After that error is thrown a new connection is automatically created and everything works fine again. mongodb logs don't show an end connection for anywhere between 37 and 170 minutes. If I create a really simple setInterval and make a request every 60 seconds I never experience this problem. Needless to say it is driving me nuts!

UPDATE: I should state that I'm using mongoose 3.1.2

@christkv
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will likely not fix that issue. you should try to set keepalive and timeout. are you also doing this through a firewall ? after 10 minutes of no activity mongod closes a connection. if the end signal is not reaching the driver it means something is consuming it between the db and the driver my guess some sort of firewall.

@aheckmann
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sberryman mongoose 3.1.2 is using 1.1.7 so maybe test again with this change and post your results back here.

@aheckmann
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh nvm

@sberryman
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aheckmann what is the best way to set config params for the native connection? I'm passing them in as part of the connection string as the first arg for mongoose.connect. I see you have an issue created to provide better explication of connect and createConnection and am really looking forward to that being added to the documentation. I'm using mongoose.connect right now and again, passing the config params by doing something along these lines:

mongodb://[user]:[password[@[host]/[database]?wtimeoutMS=2000;connectTimeoutMS=2000;socketTimeoutMS=20000

@sberryman
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@christkv thanks for that 10 minute comment. I finally tracked down the article on http://www.mongodb.org/display/DOCS/Troubleshooting#Troubleshooting-Socketerrorsinshardedclustersandreplicasets which pointed me to tcp_keepalive_interval which was the default value of 7200 (7200000 on solaris) and changed the value to 5 minutes. BAM no more issues with MongoDB or Redis. I only wasted 4 days on this but glad it appears to be behind me.

@aheckmann
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sberryman I updated the api docs with some more examples for now: http://mongoosejs.com/docs/api.html#index_Mongoose-createConnection

in a nutshell, you pass an options object after the uri.

Please sign in to comment.