fix: allow client connect after close #2581

HanaPearlman · 2020-10-14T12:58:22Z

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

Cleaning up some state beforehand fixes the issue.

HanaPearlman · 2020-10-14T13:02:40Z

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

HanaPearlman · 2020-10-14T13:05:37Z

test/functional/connection.test.js

@@ -273,4 +274,31 @@ describe('Connection', function () {
      done();
    }
  });
+
+  it('should be able to connect again after close', function () {
+    return withClient.call(this, (client, done) => {


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.

@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

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

@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...

HanaPearlman · 2020-10-14T13:07:54Z

src/mongo_client.ts

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


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

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.

reggi

Just Some small things 👍

reggi · 2020-10-14T14:05:10Z

src/mongo_client.ts

@@ -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) => {


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

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?

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

reggi · 2020-10-14T14:07:16Z

test/functional/connection.test.js

@@ -273,4 +274,31 @@ describe('Connection', function () {
      done();
    }
  });
+
+  it('should be able to connect again after close', function () {
+    return withClient.bind(this)((client, done) => {


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.

emadum

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

emadum · 2020-10-14T14:59:13Z

test/functional/connection.test.js

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


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

emadum · 2020-10-14T15:00:15Z

test/functional/connection.test.js

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


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

mbroadst · 2020-10-15T13:47:07Z

src/mongo_client.ts

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


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.

mbroadst · 2020-10-15T13:47:28Z

src/mongo_client.ts

@@ -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) => {


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

mbroadst · 2020-10-15T13:48:33Z

src/operations/connect.ts

@@ -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`);


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

mbroadst · 2020-10-15T13:53:15Z

test/functional/sessions.test.js

@@ -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;


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

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.

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.

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.

reggi

LGTM

nbbeeken

🚀 🚀 🚀

This reverts commit 1aecf96.

allow for client connect after close

8ca579c

HanaPearlman commented Oct 14, 2020

View reviewed changes

HanaPearlman requested review from mbroadst, nbbeeken, reggi and emadum October 14, 2020 13:11

bind withClient to this

f7ca2ff

reggi suggested changes Oct 14, 2020

View reviewed changes

respond to withClient comment

c5e0309

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

emadum approved these changes Oct 14, 2020

View reviewed changes

add additional connection expectations in test

4fe0911

HanaPearlman requested a review from reggi October 14, 2020 16:17

mbroadst suggested changes Oct 15, 2020

View reviewed changes

HanaPearlman added 2 commits October 15, 2020 10:30

Merge branch 'master' into NODE-2544/master/fix-connect-after-close

4c113dd

use MongoError instead of plain Error

09dde51

HanaPearlman requested a review from mbroadst October 15, 2020 15:05

reggi approved these changes Oct 15, 2020

View reviewed changes

nbbeeken approved these changes Oct 16, 2020

View reviewed changes

HanaPearlman force-pushed the NODE-2544/master/fix-connect-after-close branch from b214b47 to 981226c Compare October 16, 2020 19:36

mbroadst approved these changes Oct 16, 2020

View reviewed changes

HanaPearlman force-pushed the NODE-2544/master/fix-connect-after-close branch from 981226c to 09dde51 Compare October 16, 2020 20:49

HanaPearlman merged commit 1aecf96 into mongodb:master Oct 16, 2020

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

Revert "fix: allow client connect after close (mongodb#2581)"

eb2786f

This reverts commit 1aecf96.

HanaPearlman added a commit that referenced this pull request Oct 21, 2020

Revert "fix: allow client connect after close (#2581)" (#2591)

ba76696

This reverts commit 1aecf96.

HanaPearlman mentioned this pull request Nov 6, 2020

fix: allow client connect after close #2615

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: allow client connect after close #2581

fix: allow client connect after close #2581

HanaPearlman commented Oct 14, 2020

HanaPearlman commented Oct 14, 2020

HanaPearlman Oct 14, 2020 •

edited

reggi Oct 14, 2020

HanaPearlman Oct 14, 2020

HanaPearlman Oct 14, 2020

HanaPearlman Oct 14, 2020 •

edited

mbroadst Oct 15, 2020

reggi left a comment

reggi Oct 14, 2020

HanaPearlman Oct 14, 2020

mbroadst Oct 15, 2020

reggi Oct 14, 2020

emadum left a comment

emadum Oct 14, 2020

emadum Oct 14, 2020

mbroadst Oct 15, 2020

mbroadst Oct 15, 2020

mbroadst Oct 15, 2020

mbroadst Oct 15, 2020

HanaPearlman Oct 15, 2020

mbroadst Oct 16, 2020

HanaPearlman Oct 16, 2020 •

edited

reggi left a comment

nbbeeken left a comment

fix: allow client connect after close #2581

fix: allow client connect after close #2581

Conversation

HanaPearlman commented Oct 14, 2020

HanaPearlman commented Oct 14, 2020

HanaPearlman Oct 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HanaPearlman Oct 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reggi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emadum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HanaPearlman Oct 16, 2020 • edited

Choose a reason for hiding this comment

reggi left a comment

Choose a reason for hiding this comment

nbbeeken left a comment

Choose a reason for hiding this comment

HanaPearlman Oct 14, 2020 •

edited

HanaPearlman Oct 14, 2020 •

edited

HanaPearlman Oct 16, 2020 •

edited