Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Queue multiple open connection callbacks #585

Closed
wants to merge 1 commit into from
Closed

Queue multiple open connection callbacks #585

wants to merge 1 commit into from

Conversation

katspaugh
Copy link

Hi!

Since the latest version of node-mongodb-native isn't allowing multiple opened connections, some third-party code might have broken.

This patch provides backwards compatibility. When someone tries to open a connection more than once, his callback will nevertheless be executed, even if the connection is remaining single.

Emission of open event is also included for convenient subscription.

Thanks!

@Zanisimo
Copy link

+1

@christkv
Copy link
Contributor

Nope, this will leave connections just hanging on mongodb. The reason it was changed was due to people reopening the connection while the driver was attempting to finished the connection startup leaving operations in mid-flight and connections open on mongodb.

just call .close before calling open and it will work as you expect.

@christkv christkv closed this Apr 18, 2012
@katspaugh
Copy link
Author

Hi, Christian!

Just to make it clear, the patch isn't opening new connections, it reuses one single connection queuing callbacks on attempted re-openings.

Thanks, anyway!

@christkv
Copy link
Contributor

self.serverConfig.connect causes problems there is a reason why I don't allow it. Why would you possibly be reopening the connection ????

@christkv
Copy link
Contributor

are you trying to reuse connections across multiple dbs ?

@katspaugh
Copy link
Author

Christian,

We read collections in the same DB from independent modules. The DB object is shared across the modules. Previously, we opened a connection every time we needed to read a collection. Now we have to check if the connection has already been opened or is being opened at the moment, and only then we can use the connection. The proposed patch just moves that logic into the driver.

So, in effect, the people who used to reopen the connection while it was in the flight, would do the same (as wrong as they might be), but in fact, they wouldn't be reopening the connection – the proposed patch would only schedule their callback to run once the single connection is opened.

@aheckmann
Copy link
Contributor

I think this is better left in user land instead of the driver.

@katspaugh
Copy link
Author

@aheckmann, you suggest that we should check if the connection is opened or is being opened on each connection? Then, maybe the driver could provide some event, like "open"?

@aheckmann
Copy link
Contributor

emitting would be nice:

switch (db.state) {
  case 'connected':
    cb();
    break;
  case 'connecting':
    db.once('open', cb);
    break;
  case 'disconnected':
    db.open(cb);
    break;
}

@christkv
Copy link
Contributor

I'm open to emitting as long as it does not break backwards compatibility.

@Zanisimo
Copy link

One problem with closing before opening is that if you do this, the connection is not closed on close callback.

db.open(function(){
    console.log('open');
    db.close(function(){
        console.log('close');
        db.open(function(){
            console.log('open2');
        });
    });
});

@aheckmann
Copy link
Contributor

sounds like a separate bug

@christkv
Copy link
Contributor

this is fixed in master now

@vjavaly
Copy link

vjavaly commented Oct 12, 2012

Hello. Is it possible to maintain 2 simultaneous MongoDB connections (on different servers)? When I try to open the second connection, I get the following exception:

/home/ec2-user/node_modules/mongoose/node_modules/mongodb/lib/mongodb/connection/server.js:437
throw err;
^
Error: Trying to open unclosed connection.
at NativeConnection.open (/home/ec2-user/node_modules/mongoose/lib/connection.js:191:15)
at Mongoose.connect (/home/ec2-user/node_modules/mongoose/lib/index.js:150:15)
at NativeConnection. (/home/ec2-user/vj/rptsvr/Development/bin/rpt_wrkr.js:29:31)
at NativeConnection.emit (events.js:64:17)
at open (/home/ec2-user/node_modules/mongoose/lib/connection.js:394:10)
at NativeConnection.onOpen (/home/ec2-user/node_modules/mongoose/lib/connection.js:401:5)
at /home/ec2-user/node_modules/mongoose/lib/connection.js:372:10
at /home/ec2-user/node_modules/mongoose/lib/drivers/node-mongodb-native/connection.js:47:5
at /home/ec2-user/node_modules/mongoose/node_modules/mongodb/lib/mongodb/db.js:288:14
at [object Object]. (/home/ec2-user/node_modules/mongoose/node_modules/mongodb/lib/mongodb/connection/server.js:235:7)

Thank you.

@vjavaly
Copy link

vjavaly commented Oct 12, 2012

Never mind, I figured it out. I had been using connect instead of createConnection. Thanks anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants