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

fix: allow client connect after close #2581

Conversation

HanaPearlman
Copy link
Contributor

connect doesn't work again after close has been called once on a MongoClient.

Cleaning up some state beforehand fixes the issue.

NODE-2544

@HanaPearlman
Copy link
Contributor Author

Note most of this work comes from PR #2355. I added comments corresponding to some unresolved comments/important points from that PR.

@@ -273,4 +274,31 @@ describe('Connection', function () {
done();
}
});

it('should be able to connect again after close', function () {
return withClient.call(this, (client, done) => {
Copy link
Contributor Author

@HanaPearlman HanaPearlman Oct 14, 2020

Choose a reason for hiding this comment

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

Following up on a discussion from #2355, I'm using this instead of just withClient((client, done) ... because I couldn't see test output (warning messages or failures) without this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

@HanaPearlman Yes! this is super confusing, there's ticket in the back log to address this I added a comment in there about how to use this as I believe it was intended. NODE-2764

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, took a closer look at withClient and it seems like the right approach is to bind withClient to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@reggi Thanks for that! Happy to make that change now to replace the test callback or keep the bind syntax; not 100% clear on what is best...

this.topology = undefined;
this.s.dbCache = new Map();
this.s.sessions = new Set();

Copy link
Contributor Author

@HanaPearlman HanaPearlman Oct 14, 2020

Choose a reason for hiding this comment

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

Per a discussion from #2355, we don't restore this.s.options to what it was originally constructed with here, so we get output warnings when re-connecting:

the options [servers] is not supported
the options [caseTranslate] is not supported
the options [dbName] is not supported

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine for today. There's another unit of work here in the future (after the MongoClientOptions project) where the options will be stored immutably, in which case this problem goes away.

Copy link
Contributor

@reggi reggi left a comment

Choose a reason for hiding this comment

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

Just Some small things 👍

@@ -346,19 +346,29 @@ export class MongoClient extends EventEmitter implements OperationParent {
const force = typeof forceOrCallback === 'boolean' ? forceOrCallback : false;

return maybePromise(callback, cb => {
const completeClose = (err?: AnyError) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: naming of completeClose, perhaps something like resetState is more apt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had a small offline discussion. Leaning towards leaving this as-is because completeClose both resets the state and calls the callback. Does anyone else have thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with completeClose, it's also a pattern we use elsewhere

@@ -273,4 +274,31 @@ describe('Connection', function () {
done();
}
});

it('should be able to connect again after close', function () {
return withClient.bind(this)((client, done) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

withClient is totally wonk right now, but because you're not passing in any extra options to it, it can be used like this:

it('should be able to connect again after close', withClient(function (client, done) {

Note you can't use arrow function to maintain mocha's this scoping.

@HanaPearlman HanaPearlman force-pushed the NODE-2544/master/fix-connect-after-close branch from 9f31a73 to c5e0309 Compare October 14, 2020 14:46
Copy link
Contributor

@emadum emadum left a comment

Choose a reason for hiding this comment

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

LGTM 👍 just suggested a few additional expectations in the new test


client.close(err => {
expect(err).to.not.exist;

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd add expect(client.isConnected()).to.be.false; here

expect(err).to.not.exist;

client.connect(err => {
expect(err).to.not.exist;
Copy link
Contributor

Choose a reason for hiding this comment

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

And another expect(client.isConnected()).to.be.true; here, just as a sanity check.

this.topology = undefined;
this.s.dbCache = new Map();
this.s.sessions = new Set();

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine for today. There's another unit of work here in the future (after the MongoClientOptions project) where the options will be stored immutably, in which case this problem goes away.

@@ -346,19 +346,29 @@ export class MongoClient extends EventEmitter implements OperationParent {
const force = typeof forceOrCallback === 'boolean' ? forceOrCallback : false;

return maybePromise(callback, cb => {
const completeClose = (err?: AnyError) => {
Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with completeClose, it's also a pattern we use elsewhere

@@ -197,6 +197,11 @@ export function connect(
throw new Error('no callback function provided');
}

// Has a connection already been established?
if (mongoClient.topology && mongoClient.topology.isConnected()) {
throw new Error(`'connect' cannot be called when already connected`);
Copy link
Member

Choose a reason for hiding this comment

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

Let's not use plain Error objects, instead prefer a TypeError for simple type mismatches and MongoError or one of its subclasses for the rest. In this case I think a MongoError would be best. (I know the code right above this uses an Error, but let's be the change we want to see in the world right?)

test/functional/connection.test.js Show resolved Hide resolved
@@ -123,7 +123,7 @@ describe('Sessions', function () {
// verify that the `endSessions` command was sent
const lastCommand = test.commands.started[test.commands.started.length - 1];
expect(lastCommand.commandName).to.equal('endSessions');
expect(client.topology.s.sessionPool.sessions).to.have.length(0);
expect(client.topology).to.not.exist;
Copy link
Member

Choose a reason for hiding this comment

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

what's going on with the changes in this file? It looks like we're losing coverage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As part of closing the MongoClient, we set client.topology to undefined, so we cannot check the size of its session pool, we can just check that the topology no longer exists.

I agree it's not the best but it seems like we have to set the topology to undefined (instead of just setting the state of the topology to closed) because otherwise it's ambiguous whether the topology 1) just hasn't been opened yet or 2) was closed/all cleaned up.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. I think an implicit part of this work is that Topology can be connected, closed and reused as well. A Topology basically is a client, the MongoClient being a wrapper around it to provide access to higher level API. Ideally we would want to assign a Topology in the constructor of MongoClient, rather than during connect (but that's not quite possible today).

Have you tried not resetting topology = undefined yet? This might require changes to the connect operation. Let's not go down a huge rabbit hole here, but I'd like to better understand the limitations to that approach before opting for this alternative.

Copy link
Contributor Author

@HanaPearlman HanaPearlman Oct 16, 2020

Choose a reason for hiding this comment

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

Yep, you're right, it is possible to not set topology = undefined after a small change to the connect logic. This means we don't need to do any additional clean-up on client close, since any references to a Db/Client can use the same topology after re-connect. I've made this change, and will write a few tests to verify that the topology still receives the correct events after client re-connect.

Update: decided against making these changes because of the refactor required to re-connect topologies may be too much work for this ticket.

Copy link
Contributor

@reggi reggi left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

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

🚀 🚀 🚀

@HanaPearlman HanaPearlman force-pushed the NODE-2544/master/fix-connect-after-close branch from b214b47 to 981226c Compare October 16, 2020 19:36
@HanaPearlman HanaPearlman force-pushed the NODE-2544/master/fix-connect-after-close branch from 981226c to 09dde51 Compare October 16, 2020 20:49
@HanaPearlman HanaPearlman merged commit 1aecf96 into mongodb:master Oct 16, 2020
@HanaPearlman HanaPearlman deleted the NODE-2544/master/fix-connect-after-close branch October 16, 2020 21:32
HanaPearlman added a commit to HanaPearlman/node-mongodb-native that referenced this pull request Oct 20, 2020
HanaPearlman added a commit that referenced this pull request Oct 21, 2020
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