fix: allow connect again after close #2355

emadum · 2020-05-06T20:02:21Z

Description

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

Cleaning up some state beforehand fixes the issue.

NODE-2544

What changed?

Also introduced some CI-related cleanup:

output eslint version before linting
upgrade nvm and use --no-progress option to clean up log output

I can split this out into a separate PR if desired.

Are there any files to ignore?

lib/operations/connect.js

emadum · 2020-05-06T20:06:01Z

package.json

@@ -68,7 +68,7 @@
    "test": "npm run lint && mocha --recursive test/functional test/unit test/core",
    "test-nolint": "mocha --recursive test/functional test/unit test/core",
    "coverage": "istanbul cover mongodb-test-runner -- -t 60000 test/core test/unit test/functional",
-    "lint": "eslint lib test",
+    "lint": "eslint -v && eslint lib test",


Unrelated to this PR. I just sometimes get weird discrepancies between running eslint locally and what runs on evergreen. Was thinking adding this could help make things a bit more explicit around the version of eslint being used, when it is being used.

test/functional/shared.js

…after-close

emadum · 2020-05-08T14:00:26Z

.evergreen/install-dependencies.sh

 [ -s "${NVM_DIR}/nvm.sh" ] && \. "${NVM_DIR}/nvm.sh"
-nvm install --lts=${NODE_LTS_NAME}
+nvm install --no-progress --lts=${NODE_LTS_NAME}


I upgraded nvm so we can use the newish --no-progress option.

oh very nice 👏

mbroadst · 2020-05-11T12:35:57Z

.evergreen/install-dependencies.sh

 [ -s "${NVM_DIR}/nvm.sh" ] && \. "${NVM_DIR}/nvm.sh"
-nvm install --lts=${NODE_LTS_NAME}
+nvm install --no-progress --lts=${NODE_LTS_NAME}


oh very nice 👏

test/functional/shared.js

test/functional/connection.test.js

mbroadst · 2020-05-11T12:42:27Z

lib/operations/connect.js

+  // Has a connection already been established?
+  if (mongoClient.topology) {
+    if (mongoClient.topology.isConnected()) {
+      throw new Error('already connected');


I think this error message can be improved

lib/operations/connect.js

emadum · 2020-05-11T13:46:52Z

lib/mongo_client.js

@@ -241,28 +243,35 @@ MongoClient.prototype.close = function(force, callback) {
    force = false;
  }

-  const client = this;


We were using arrow functions everywhere here, so I just updated everything to use this

iirc the reason client was used is because this method has significant enough depth that this can't be bound to the deepest nested functions. Can you confirm, for instance, that assignment to autoEncrypter below is actually referencing the correct this?

emadum · 2020-05-11T13:47:56Z

lib/mongo_client.js

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


the old dbCache still references the old topology, so it is manually cleared out here in addition to this.topology

emadum · 2020-05-11T13:49:19Z

lib/mongo_client.js

+      this.s.dbCache = new Map();
+
+      // restore options
+      this.s.options = Object.assign({}, this.originalOptions);


this.s.options can get mutated by connect, so we need to restore it to what it was originally constructed with. Otherwise we get the following warnings, as reported in one of the issues:

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

I would prefer that the small number of users using this functionality see those warnings than make all users pay to save off another copy of the options.

mbroadst · 2020-05-11T13:53:46Z

lib/mongo_client.js

+      this.s.dbCache = new Map();
+
+      // restore options
+      this.s.options = Object.assign({}, this.originalOptions);


I would prefer that the small number of users using this functionality see those warnings than make all users pay to save off another copy of the options.

mbroadst · 2020-05-11T14:00:12Z

lib/mongo_client.js

@@ -241,28 +243,35 @@ MongoClient.prototype.close = function(force, callback) {
    force = false;
  }

-  const client = this;


iirc the reason client was used is because this method has significant enough depth that this can't be bound to the deepest nested functions. Can you confirm, for instance, that assignment to autoEncrypter below is actually referencing the correct this?

mbroadst · 2020-05-11T14:02:10Z

test/functional/connection.test.js

@@ -517,7 +517,7 @@ describe('Connection', function() {
  });

  it('should be able to connect again after close', function() {
-    return withClient(this.configuration.newClient(), client => done => {
+    return withClient.call(this, (client, done) => {


this should be equivalent to just calling withClient within the scope, why was this change needed?

Without it I get TypeError: Cannot read property 'configuration' of undefined referencing this line.

mbroadst · 2020-05-11T14:03:49Z

test/functional/shared.js

@@ -57,8 +57,37 @@ function setupDatabase(configuration, dbsToClean) {
    );
 }

-function makeCleanupFn(client) {
-  return function(err) {
+function withTempDb(name, options, client, operation, errorHandler) {


Similar to the change streams PR, I'd like to find a way not to wrap up withClient calls into something that doesn't seem to create a client. At very least I think we should have composed naming for these methods, like withClientAndTempDb, but maybe there's an even better way. Maybe we could end up with something like:

withClient(withTempDb((client, db, done) => { }));

I was actually working on this in a separate draft PR (#2362) so I pulled in my new withDb helper.

…after-close

emadum · 2020-05-12T19:27:07Z

Will open a new PR for this after #2362 is merged

anzairyo0127 · 2020-10-09T05:10:59Z

#2362 seems to have been merged.
Has a new Pull Request been issued for this change?

add test and fix

0136cd1

emadum commented May 6, 2020

View reviewed changes

lib/operations/connect.js Outdated Show resolved Hide resolved

emadum commented May 6, 2020

View reviewed changes

test/functional/shared.js Outdated Show resolved Hide resolved

emadum marked this pull request as draft May 6, 2020 21:24

emadum added 4 commits May 7, 2020 15:49

Merge remote-tracking branch 'origin/3.6' into NODE-2544/fix-connect-…

835f0aa

…after-close

clean up evg logs

3be1b95

update nvm to support --no-progress

865d8b5

add XFG_CONFIG_HOME for new nvm

854c72f

emadum commented May 8, 2020

View reviewed changes

emadum marked this pull request as ready for review May 8, 2020 14:08

emadum requested review from mbroadst and reggi May 8, 2020 14:24

mbroadst suggested changes May 11, 2020

View reviewed changes

incorporate review feedback

5043ca3

emadum commented May 11, 2020

View reviewed changes

refactor withTempDb to withDb

5763728

mbroadst suggested changes May 11, 2020

View reviewed changes

emadum added 3 commits May 11, 2020 10:36

dont save options, show warnings instead

c4eb0fa

fix for sessions

32c1ce8

save reference to topology

dafab24

emadum removed the request for review from reggi May 11, 2020 19:02

emadum added 2 commits May 11, 2020 15:04

Merge remote-tracking branch 'origin/3.6' into NODE-2544/fix-connect-…

caf581e

…after-close

fix flaky tryNext tests

414e091

emadum closed this May 12, 2020

mbroadst deleted the NODE-2544/fix-connect-after-close branch August 27, 2020 10:39

HanaPearlman restored the NODE-2544/fix-connect-after-close branch October 13, 2020 19:02

HanaPearlman mentioned this pull request Oct 14, 2020

fix: allow client connect after close #2581

Merged

mbroadst deleted the NODE-2544/fix-connect-after-close branch October 14, 2020 19:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: allow connect again after close #2355

fix: allow connect again after close #2355

emadum commented May 6, 2020 •

edited

Loading

emadum May 6, 2020

emadum May 8, 2020 •

edited

Loading

mbroadst May 11, 2020

mbroadst May 11, 2020

mbroadst May 11, 2020

emadum May 11, 2020

mbroadst May 11, 2020

emadum May 11, 2020

emadum May 11, 2020

mbroadst May 11, 2020

mbroadst May 11, 2020

mbroadst May 11, 2020

mbroadst May 11, 2020

emadum May 11, 2020

mbroadst May 11, 2020

emadum May 11, 2020

emadum commented May 12, 2020

anzairyo0127 commented Oct 9, 2020

fix: allow connect again after close #2355

fix: allow connect again after close #2355

Conversation

emadum commented May 6, 2020 • edited Loading

Description

Choose a reason for hiding this comment

emadum May 8, 2020 • edited Loading

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

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

emadum commented May 12, 2020

anzairyo0127 commented Oct 9, 2020

emadum commented May 6, 2020 •

edited

Loading

emadum May 8, 2020 •

edited

Loading