fix(NODE-7229): remove duplicate server selection when auto-connecting

Author: nbbeeken

src/operations/execute_operation.ts

@@ -82,11 +82,6 @@ export async function executeOperation<
     session = client.startSession({ owner, explicit: false });
   } else if (session.hasEnded) {
     throw new MongoExpiredSessionError('Use of expired sessions is not permitted');
-  } else if (
-    session.snapshotEnabled &&
-    maxWireVersion(topology) < MIN_SUPPORTED_SNAPSHOT_READS_WIRE_VERSION
-  ) {
-    throw new MongoCompatibilityError('Snapshot reads require MongoDB 5.0 or later');
   } else if (session.client !== client) {
     throw new MongoInvalidArgumentError('ClientSession must be from the same MongoClient');
   }
@@ -210,6 +205,13 @@ async function tryOperation<T extends AbstractOperation, TResult = ResultTypeFro
     signal: operation.options.signal
   });
 
+  if (
+    session?.snapshotEnabled &&
+    maxWireVersion(topology) < MIN_SUPPORTED_SNAPSHOT_READS_WIRE_VERSION
+  ) {
+    throw new MongoCompatibilityError('Snapshot reads require MongoDB 5.0 or later');
+  }
+
   const hasReadAspect = operation.hasAspect(Aspect.READ_OPERATION);
   const hasWriteAspect = operation.hasAspect(Aspect.WRITE_OPERATION);
   const inTransaction = session?.inTransaction() ?? false;

src/sdam/topology.ts

@@ -459,20 +459,14 @@ export class Topology extends TypedEventEmitter<TopologyEvents> {
     };
 
     try {
-      const server = await this.selectServer(
-        readPreferenceServerSelector(readPreference),
-        selectServerOptions
-      );
-
       const skipPingOnConnect = this.s.options.__skipPingOnConnect === true;
       if (!skipPingOnConnect) {
+        const server = await this.selectServer(
+          readPreferenceServerSelector(readPreference),
+          selectServerOptions
+        );
         const connection = await server.pool.checkOut({ timeoutContext: timeoutContext });
         server.pool.checkIn(connection);
-        stateTransition(this, STATE_CONNECTED);
-        this.emit(Topology.OPEN, this);
-        this.emit(Topology.CONNECT, this);
-
-        return this;
       }
 
       stateTransition(this, STATE_CONNECTED);

test/integration/node-specific/abort_signal.test.ts

@@ -29,6 +29,8 @@ import {
   findLast,
   sleep
 } from '../../tools/utils';
+import { Topology } from '../../../lib/sdam/topology';
+import { MongoServerSelectionError } from '../../../lib/error';
 
 const failPointMetadata = { requires: { mongodb: '>=4.4' } };
 
@@ -611,35 +613,22 @@ describe('AbortSignal support', () => {
     let client: MongoClient;
     let db: Db;
     let collection: Collection<{ a: number; ssn: string }>;
-    const logs: Log[] = [];
     let connectStarted;
     let controller: AbortController;
     let signal: AbortSignal;
     let cursor: AbstractCursor<{ a: number }>;
 
     describe('when connect succeeds', () => {
       beforeEach(async function () {
-        logs.length = 0;
-
         const promise = promiseWithResolvers<void>();
         connectStarted = promise.promise;
 
-        client = this.configuration.newClient(
-          {},
-          {
-            mongodbLogComponentSeverities: { serverSelection: 'debug' },
-            mongodbLogPath: {
-              write: log => {
-                if (log.c === 'serverSelection' && log.operation === 'handshake') {
-                  controller.abort();
-                  promise.resolve();
-                }
-                logs.push(log);
-              }
-            },
-            serverSelectionTimeoutMS: 1000
-          }
-        );
+        client = this.configuration.newClient({}, { serverSelectionTimeoutMS: 1000 });
+
+        client.once('open', () => {
+          controller.abort();
+          promise.resolve();
+        });
         db = client.db('abortSignal');
         collection = db.collection('support');
 
@@ -650,7 +639,6 @@ describe('AbortSignal support', () => {
       });
 
       afterEach(async function () {
-        logs.length = 0;
         await client?.close();
       });
 
@@ -666,22 +654,18 @@ describe('AbortSignal support', () => {
 
     describe('when connect fails', () => {
       beforeEach(async function () {
-        logs.length = 0;
-
         const promise = promiseWithResolvers<void>();
         connectStarted = promise.promise;
 
+        const selectServerStub = sinon
+          .stub(Topology.prototype, 'selectServer')
+          .callsFake(async function (...args) {
+            controller.abort();
+            promise.resolve();
+            return selectServerStub.wrappedMethod.call(this, ...args);
+          });
+
         client = this.configuration.newClient('mongodb://iLoveJavaScript', {
-          mongodbLogComponentSeverities: { serverSelection: 'debug' },
-          mongodbLogPath: {
-            write: log => {
-              if (log.c === 'serverSelection' && log.operation === 'handshake') {
-                controller.abort();
-                promise.resolve();
-              }
-              logs.push(log);
-            }
-          },
           serverSelectionTimeoutMS: 200,
           maxPoolSize: 1
         });
@@ -695,18 +679,23 @@ describe('AbortSignal support', () => {
       });
 
       afterEach(async function () {
-        logs.length = 0;
+        sinon.restore();
         await client?.close();
       });
 
-      it('escapes auto connect without interrupting it', async () => {
+      it('server selection error is thrown before reaching signal abort state check', async () => {
         const toArray = cursor.toArray().catch(error => error);
         await connectStarted;
-        expect(await toArray).to.be.instanceOf(DOMException);
+        const findError = await toArray;
+        expect(findError).to.be.instanceOf(MongoServerSelectionError);
+        if (process.platform !== 'win32') {
+          // linux / mac, unix in general will have this errno set,
+          // which is generally helpful if this is kept elevated in the error message
+          expect(findError).to.match(/ENOTFOUND/);
+        }
         await sleep(500);
         expect(client.topology).to.exist;
         expect(client.topology.description).to.have.property('type', 'Unknown');
-        expect(findLast(logs, l => l.message.includes('Server selection failed'))).to.exist;
       });
     });
   });

test/integration/node-specific/auto_connect.test.ts

@@ -9,6 +9,7 @@ import {
   type Collection,
   MongoClient,
   MongoNotConnectedError,
+  MongoOperationTimeoutError,
   ProfilingLevel,
   TopologyType
 } from '../../../src';
@@ -862,12 +863,17 @@ describe('When executing an operation for the first time', () => {
           sinon.restore();
         });
 
-        it('client.connect() takes as long as selectServer is delayed for and does not throw a timeout error', async function () {
+        it('client.connect() takes as long as selectServer is delayed for and throws a timeout error', async function () {
           const start = performance.now();
           expect(client.topology).to.not.exist; // make sure not connected.
-          const res = await client.db().collection('test').insertOne({ a: 1 }, { timeoutMS: 500 }); // auto-connect
+          const error = await client
+            .db()
+            .collection('test')
+            .insertOne({ a: 1 }, { timeoutMS: 500 })
+            .catch(error => error);
           const end = performance.now();
-          expect(res).to.have.property('acknowledged', true);
+          expect(error).to.be.instanceOf(MongoOperationTimeoutError);
+          expect(error).to.match(/Timed out during server selection/);
           expect(end - start).to.be.within(1000, 1500); // timeoutMS is 1000, did not apply.
         });
       }

test/integration/node-specific/mongo_client.test.ts

@@ -339,29 +339,6 @@ describe('class MongoClient', function () {
     });
   });
 
-    it('throws ENOTFOUND error when connecting to non-existent host with no auth and loadBalanced=true', async function () {
-      const configuration = this.configuration;
-      const client = configuration.newClient(
-        'mongodb://iLoveJavaScript:27017/test?loadBalanced=true',
-        { serverSelectionTimeoutMS: 100 }
-      );
-
-      const error = await client.connect().catch(error => error);
-      expect(error).to.be.instanceOf(MongoNetworkError); // not server selection like other topologies
-      expect(error.message).to.match(/ENOTFOUND/);
-    });
-
-    it('throws an error when srv is not a real record', async function () {
-      const client = this.configuration.newClient('mongodb+srv://iLoveJavaScript/test', {
-        serverSelectionTimeoutMS: 100
-      });
-
-      const error = await client.connect().catch(error => error);
-      expect(error).to.be.instanceOf(Error);
-      expect(error.message).to.match(/ENOTFOUND/);
-    });
-  });
-
   it('Should correctly pass through appname', async function () {
     const configuration = this.configuration;
     const options = {
@@ -511,13 +488,29 @@ describe('class MongoClient', function () {
       await client.close();
     });
 
-    it('creates topology and checks out connection', async function () {
-      const checkoutStarted = once(client, 'connectionCheckOutStarted');
-      await client.connect();
-      const checkout = await checkoutStarted;
-      expect(checkout).to.exist;
-      expect(client).to.have.property('topology').that.is.instanceOf(Topology);
-    });
+    it(
+      'creates topology and checks out connection when auth is enabled',
+      { requires: { auth: 'enabled' } },
+      async function () {
+        const checkoutStarted = once(client, 'connectionCheckOutStarted');
+        await client.connect();
+        const checkout = await checkoutStarted;
+        expect(checkout).to.exist;
+        expect(client).to.have.property('topology').that.is.instanceOf(Topology);
+      }
+    );
+
+    it(
+      'checks out connection to confirm connectivity even when authentication is disabled',
+      { requires: { auth: 'disabled' } },
+      async function () {
+        const checkoutStarted = once(client, 'connectionCheckOutStarted');
+        await client.connect();
+        const checkout = await checkoutStarted;
+        expect(checkout).to.exist;
+        expect(client).to.have.property('topology').that.is.instanceOf(Topology);
+      }
+    );
 
     it(
       'permits operations to be run after connect is called',
@@ -606,6 +599,7 @@ describe('class MongoClient', function () {
         expect(result).to.be.instanceOf(MongoServerSelectionError);
         expect(client).to.be.instanceOf(MongoClient);
         expect(client).to.have.property('topology').that.is.instanceOf(Topology);
+        await client.close();
       }
     );
   });

test/integration/server-selection/server_selection.spec.test.ts

@@ -3,18 +3,19 @@ import * as path from 'path';
 import { loadSpecTests } from '../../spec';
 import { runUnifiedSuite } from '../../tools/unified-spec-runner/runner';
 
-describe.skip('Server Selection Unified Tests (Spec)', function () {
+describe('Server Selection Unified Tests (Spec)', function () {
   const tests = loadSpecTests(path.join('server-selection', 'logging'));
   runUnifiedSuite(tests, test => {
     if (
       [
         'Failed bulkWrite operation: log messages have operationIds',
-        'Successful bulkWrite operation: log messages have operationIds'
+        'Failed client bulkWrite operation: log messages have operationIds',
+        'Successful bulkWrite operation: log messages have operationIds',
+        'Successful client bulkWrite operation: log messages have operationIds'
       ].includes(test.description)
     ) {
       return 'not applicable: operationId not supported';
     }
     return false;
   });
-}).skipReason =
-  'TODO: unskip these tests - NODE-2471 (ping on connect) and NODE-5774 (duplicate server selection for bulkWrite and other wrapper operations';
+});