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.

Sign up for GitHub

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

Jump to bottom

fix: allow client connect after close #2581

Merged
merged 6 commits into from
Oct 16, 2020
Merged

fix: allow client connect after close #2581

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8ca579c
allow for client connect after close
HanaPearlman Oct 13, 2020
f7ca2ff
bind withClient to this
HanaPearlman Oct 14, 2020
c5e0309
respond to withClient comment
HanaPearlman Oct 14, 2020
4fe0911
add additional connection expectations in test
HanaPearlman Oct 14, 2020
4c113dd
Merge branch 'master' into NODE-2544/master/fix-connect-after-close
HanaPearlman Oct 15, 2020
09dde51
use MongoError instead of plain Error
HanaPearlman Oct 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions src/mongo_client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

// clear out references to old topology
this.topology = undefined;
this.s.dbCache = new Map();
this.s.sessions = new Set();

Copy link
Contributor Author

@HanaPearlman HanaPearlman Oct 14, 2020 •
edited
Loading

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.

cb(err);
};

if (this.topology == null) {
return cb();
completeClose();
return;
}

const topology = this.topology;
topology.close({ force }, err => {
const autoEncrypter = topology.s.options.autoEncrypter;
if (!autoEncrypter) {
cb(err);
completeClose(err);
return;
}

autoEncrypter.teardown(force, err2 => cb(err || err2));
autoEncrypter.teardown(force, err2 => completeClose(err || err2));
});
});
}
Expand Down
5 changes: 5 additions & 0 deletions src/operations/connect.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?)

}

let didRequestAuthentication = false;
const logger = new Logger('MongoClient', options);

Expand Down
29 changes: 29 additions & 0 deletions test/functional/connection.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
const test = require('./shared').assert,
setupDatabase = require('./shared').setupDatabase,
expect = require('chai').expect;
const withClient = require('./shared').withClient;

describe('Connection', function () {
before(function () {
Expand Down Expand Up @@ -273,4 +274,32 @@ describe('Connection', function () {
done();
}
});

it(
'should be able to connect again after close',
withClient(function (client, done) {
expect(client.isConnected()).to.be.true;

const collection = () => client.db('testReconnect').collection('test');
mbroadst marked this conversation as resolved.
Show resolved Hide resolved
collection().insertOne({ a: 1 }, (err, result) => {
expect(err).to.not.exist;
expect(result).to.exist;

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

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.


collection().insertOne({ b: 2 }, (err, result) => {
expect(err).to.not.exist;
expect(result).to.exist;
expect(client.topology.isDestroyed()).to.be.false;
done();
});
});
});
});
})
);
});
4 changes: 2 additions & 2 deletions test/functional/sessions.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

});
});
});
Expand All @@ -143,7 +143,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;
});
});
}
Expand Down