fix(topology): connect server only on next tick

[rose-m]

Description

The PR defers connecting to a server until the next tick.

If the connection is triggered immediately, an infinite recursion might occur. This is caused for example by an invalid TLS client certificate (i.e. invalid content) that can be specified via the tlsClientCertificateFile option during MongoClient.connect.

The recursion occurs due to the order of operations in connectServers (https://github.com/mongodb/node-mongodb-native/blob/master/src/sdam/topology.ts#L824). The server is connected to before it is added to the internal known server descriptions. An invalid TLS certificate file however causes an immediate error in server.connect() (https://github.com/mongodb/node-mongodb-native/blob/master/src/sdam/topology.ts#L810). This error is caught and tried to react synchronously in an event listener which tries to access the known server descriptions - which have not been updated yet.

A simple reproduction of the stack overflow is the following:

const MongoClient = require('mongodb').MongoClient;

(async() => {
    try {
        const client = await MongoClient.connect('mongodb://localhost/?directConnection=true', {
            tlsCertificateKeyFile: './package.json'
        });
        console.log('that should not work.');
    } catch (e) {
        console.error(e);
    }
})();

What changed?
Connecting to a server is not triggered immediately in the createAndConnectServer
call but on next tick afterwards.

Are there any files to ignore?
No.

NODE-2984

[nbbeeken]

There seems to be a number of tests failing in the "Server Discovery and Monitoring (spec)" suite, looks like there's a few issues, the pool generation isn't changed in time for the test's assertion and there's an issue with the topology type still being unknown when the test runs. This could be something where the runner has to wait for the nextTick to occur but it seems like that internal tracking should be all correctly resolved by the time the callbacks execute the tests assertions. @rose-m you can explore test/unit/sdam/spec.test.js if you'd like to see if there's a fix but otherwise one of us will take a look when we get the chance to.

[rose-m]

I've looked into it and the best way I could see was to also defer the serverSelection call inside Topology.connect to the next tick. Let me know if it still looks good to you! 😊

[emadum]

Some tests are leaking resources as a result of this change (causing timeouts in Evergreen), probably related to that close call.

[emadum]

I don't think this close should be here, did adding it fix a test that was failing or something?

[rose-m]

I think that is just due to the diff view - it is essentially the same this.close() call as https://github.com/mongodb/node-mongodb-native/pull/2685/files#diff-e2ebfc23930db405572db68fb3054e441b2c45f9d7ac1e62b0806862e39de8ddL353.

It's now just part of the nextTick callback.

[rose-m]

Unfortunately I've absolutely no idea how that happens or where it fails... even without that it locally fails for me right now...

[emadum]

Unfortunately I've absolutely no idea how that happens or where it fails... even without that it locally fails for me right now...

I just updated the branch I have for investigating this issue to the latest version of master, and am no longer able to reproduce the infinite recursion. @rose-m could you confirm if it's still an issue on your end? Thanks!

[rose-m]

@emadum Indeed - I also tried it locally and it seems the problem has vanished 👏 I'll leaving closing the issue and JIRA ticket to you?

[emadum]

This is no longer an issue on latest master