From ca7287ace4c0299f2c27896c9040f22c966951fa Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 28 Feb 2023 16:03:26 -0500 Subject: [PATCH 01/19] fix(NODE-4854): set timeout on write and reset on message --- src/cmap/connect.ts | 2 +- src/cmap/connection.ts | 6 +++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/cmap/connect.ts b/src/cmap/connect.ts index a69289befaa..ca17ecd052f 100644 --- a/src/cmap/connect.ts +++ b/src/cmap/connect.ts @@ -428,7 +428,7 @@ function makeConnection(options: MakeConnectionOptions, _callback: Callback { this[kDelayedTimeoutId] = null; } + this[kStream].setTimeout(0); + // always emit the message, in case we are streaming this.emit('message', message); let operationDescription = this[kQueue].get(message.responseTo); @@ -683,6 +685,8 @@ function write( if (typeof options.socketTimeoutMS === 'number') { operationDescription.socketTimeoutOverride = true; conn[kStream].setTimeout(options.socketTimeoutMS); + } else if (conn.socketTimeoutMS !== 0) { + conn[kStream].setTimeout(conn.socketTimeoutMS); } // if command monitoring is enabled we need to modify the callback here @@ -692,7 +696,7 @@ function write( operationDescription.started = now(); operationDescription.cb = (err, reply) => { // Command monitoring spec states that if ok is 1, then we must always emit - // a command suceeded event, even if there's an error. Write concern errors + // a command succeeded event, even if there's an error. Write concern errors // will have an ok: 1 in their reply. if (err && reply?.ok !== 1) { conn.emit( From a9c7c71947fa4a44b258492b09dc41dde0e4d0b5 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 28 Feb 2023 16:04:17 -0500 Subject: [PATCH 02/19] test: mv connect.test.js connection.test.ts --- test/unit/cmap/{connect.test.js => connect.test.ts} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename test/unit/cmap/{connect.test.js => connect.test.ts} (100%) diff --git a/test/unit/cmap/connect.test.js b/test/unit/cmap/connect.test.ts similarity index 100% rename from test/unit/cmap/connect.test.js rename to test/unit/cmap/connect.test.ts From c17a7020eaa8ef02f646be80f401056e651ddd0b Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 28 Feb 2023 16:05:08 -0500 Subject: [PATCH 03/19] test: fix imports --- test/unit/cmap/connect.test.ts | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/test/unit/cmap/connect.test.ts b/test/unit/cmap/connect.test.ts index f33361cfa09..4fd58480f36 100644 --- a/test/unit/cmap/connect.test.ts +++ b/test/unit/cmap/connect.test.ts @@ -1,17 +1,19 @@ -'use strict'; - -const mock = require('../../tools/mongodb-mock/index'); -const { expect } = require('chai'); -const EventEmitter = require('events'); -const { setTimeout } = require('timers'); - -const { connect, prepareHandshakeDocument: prepareHandshakeDocumentCb } = require('../../mongodb'); -const { MongoCredentials } = require('../../mongodb'); -const { genClusterTime } = require('../../tools/common'); -const { MongoNetworkError } = require('../../mongodb'); -const { HostAddress, isHello } = require('../../mongodb'); -const { LEGACY_HELLO_COMMAND } = require('../../mongodb'); -const { promisify } = require('util'); +import { expect } from 'chai'; +import { EventEmitter } from 'events'; +import { setTimeout } from 'timers'; +import { promisify } from 'util'; + +import { + connect, + HostAddress, + isHello, + LEGACY_HELLO_COMMAND, + MongoCredentials, + MongoNetworkError, + prepareHandshakeDocument as prepareHandshakeDocumentCb +} from '../../mongodb'; +import { genClusterTime } from '../../tools/common'; +import * as mock from '../../tools/mongodb-mock/index'; describe('Connect Tests', function () { const test = {}; From b9a3ad0395cac47ea002366ca2b0a89a9f65fa95 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 28 Feb 2023 16:12:17 -0500 Subject: [PATCH 04/19] test: fix typescript --- test/unit/cmap/connect.test.ts | 49 +++++++++++++++++++++------------- 1 file changed, 31 insertions(+), 18 deletions(-) diff --git a/test/unit/cmap/connect.test.ts b/test/unit/cmap/connect.test.ts index 4fd58480f36..1a72cb49eba 100644 --- a/test/unit/cmap/connect.test.ts +++ b/test/unit/cmap/connect.test.ts @@ -1,10 +1,12 @@ import { expect } from 'chai'; -import { EventEmitter } from 'events'; import { setTimeout } from 'timers'; import { promisify } from 'util'; import { + CancellationToken, + ClientMetadata, connect, + ConnectionOptions, HostAddress, isHello, LEGACY_HELLO_COMMAND, @@ -15,24 +17,35 @@ import { import { genClusterTime } from '../../tools/common'; import * as mock from '../../tools/mongodb-mock/index'; -describe('Connect Tests', function () { - const test = {}; - beforeEach(() => { - return mock.createServer().then(mockServer => { - test.server = mockServer; - test.connectOptions = { - hostAddress: test.server.hostAddress(), - credentials: new MongoCredentials({ - username: 'testUser', - password: 'pencil', - source: 'admin', - mechanism: 'PLAIN' - }) - }; - }); +describe.only('Connect Tests', function () { + const test: { + server?: any; + connectOptions?: ConnectionOptions; + } = {}; + + beforeEach(async () => { + const mockServer = await mock.createServer(); + test.server = mockServer; + test.connectOptions = { + id: 1, + tls: false, + generation: 1, + monitorCommands: false, + metadata: {} as ClientMetadata, + loadBalanced: false, + hostAddress: test.server.hostAddress() as HostAddress, + credentials: new MongoCredentials({ + username: 'testUser', + password: 'pencil', + source: 'admin', + mechanism: 'PLAIN', + mechanismProperties: {} + }) + }; }); afterEach(() => mock.cleanup()); + it('should auth against a non-arbiter', function (done) { const whatHappened = {}; @@ -104,9 +117,9 @@ describe('Connect Tests', function () { }); it.skip('should allow a cancellaton token', function (done) { - const cancellationToken = new EventEmitter(); + const cancellationToken = new CancellationToken(); setTimeout(() => cancellationToken.emit('cancel'), 500); - // set no response handler for mock server, effecively blackhole requests + // set no response handler for mock server, effectively black hole requests connect({ hostAddress: new HostAddress('240.0.0.1'), cancellationToken }, (err, conn) => { expect(err).to.exist; From 10e74d7d8c9609b07b0bc2515a788a107fe1ddd1 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 28 Feb 2023 16:19:31 -0500 Subject: [PATCH 05/19] test: move PLAIN tests into context --- test/unit/cmap/connect.test.ts | 167 +++++++++++++++++---------------- 1 file changed, 85 insertions(+), 82 deletions(-) diff --git a/test/unit/cmap/connect.test.ts b/test/unit/cmap/connect.test.ts index 1a72cb49eba..f87f8b77584 100644 --- a/test/unit/cmap/connect.test.ts +++ b/test/unit/cmap/connect.test.ts @@ -6,6 +6,7 @@ import { CancellationToken, ClientMetadata, connect, + Connection, ConnectionOptions, HostAddress, isHello, @@ -17,95 +18,97 @@ import { import { genClusterTime } from '../../tools/common'; import * as mock from '../../tools/mongodb-mock/index'; -describe.only('Connect Tests', function () { - const test: { - server?: any; - connectOptions?: ConnectionOptions; - } = {}; - - beforeEach(async () => { - const mockServer = await mock.createServer(); - test.server = mockServer; - test.connectOptions = { - id: 1, - tls: false, - generation: 1, - monitorCommands: false, - metadata: {} as ClientMetadata, - loadBalanced: false, - hostAddress: test.server.hostAddress() as HostAddress, - credentials: new MongoCredentials({ - username: 'testUser', - password: 'pencil', - source: 'admin', - mechanism: 'PLAIN', - mechanismProperties: {} - }) - }; - }); - - afterEach(() => mock.cleanup()); - - it('should auth against a non-arbiter', function (done) { - const whatHappened = {}; - - test.server.setMessageHandler(request => { - const doc = request.document; - const $clusterTime = genClusterTime(Date.now()); - - if (isHello(doc)) { - whatHappened[LEGACY_HELLO_COMMAND] = true; - request.reply( - Object.assign({}, mock.HELLO, { - $clusterTime - }) - ); - } else if (doc.saslStart) { - whatHappened.saslStart = true; - request.reply({ ok: 1 }); - } +describe('Connect Tests', function () { + context('when PLAIN auth enabled', () => { + const test: { + server?: any; + connectOptions?: ConnectionOptions; + } = {}; + + beforeEach(async () => { + const mockServer = await mock.createServer(); + test.server = mockServer; + test.connectOptions = { + id: 1, + tls: false, + generation: 1, + monitorCommands: false, + metadata: {} as ClientMetadata, + loadBalanced: false, + hostAddress: test.server.hostAddress() as HostAddress, + credentials: new MongoCredentials({ + username: 'testUser', + password: 'pencil', + source: 'admin', + mechanism: 'PLAIN', + mechanismProperties: {} + }) + }; }); - connect(test.connectOptions, err => { - try { - expect(whatHappened).to.have.property(LEGACY_HELLO_COMMAND, true); - expect(whatHappened).to.have.property('saslStart', true); - } catch (_err) { - err = _err; - } + afterEach(() => mock.cleanup()); + + it('should auth against a non-arbiter', function (done) { + const whatHappened = {}; + + test.server.setMessageHandler(request => { + const doc = request.document; + const $clusterTime = genClusterTime(Date.now()); + + if (isHello(doc)) { + whatHappened[LEGACY_HELLO_COMMAND] = true; + request.reply( + Object.assign({}, mock.HELLO, { + $clusterTime + }) + ); + } else if (doc.saslStart) { + whatHappened.saslStart = true; + request.reply({ ok: 1 }); + } + }); - done(err); - }); - }); + connect(test.connectOptions, err => { + try { + expect(whatHappened).to.have.property(LEGACY_HELLO_COMMAND, true); + expect(whatHappened).to.have.property('saslStart', true); + } catch (_err) { + err = _err; + } - it('should not auth against an arbiter', function (done) { - const whatHappened = {}; - test.server.setMessageHandler(request => { - const doc = request.document; - const $clusterTime = genClusterTime(Date.now()); - if (isHello(doc)) { - whatHappened[LEGACY_HELLO_COMMAND] = true; - request.reply( - Object.assign({}, mock.HELLO, { - $clusterTime, - arbiterOnly: true - }) - ); - } else if (doc.saslStart) { - whatHappened.saslStart = true; - request.reply({ ok: 0 }); - } + done(err); + }); }); - connect(test.connectOptions, err => { - try { - expect(whatHappened).to.have.property(LEGACY_HELLO_COMMAND, true); - expect(whatHappened).to.not.have.property('saslStart'); - } catch (_err) { - err = _err; - } + it('should not auth against an arbiter', function (done) { + const whatHappened = {}; + test.server.setMessageHandler(request => { + const doc = request.document; + const $clusterTime = genClusterTime(Date.now()); + if (isHello(doc)) { + whatHappened[LEGACY_HELLO_COMMAND] = true; + request.reply( + Object.assign({}, mock.HELLO, { + $clusterTime, + arbiterOnly: true + }) + ); + } else if (doc.saslStart) { + whatHappened.saslStart = true; + request.reply({ ok: 0 }); + } + }); - done(err); + connect(test.connectOptions, err => { + try { + expect(whatHappened).to.have.property(LEGACY_HELLO_COMMAND, true); + expect(whatHappened).to.not.have.property('saslStart'); + } catch (_err) { + err = _err; + } + + done(err); + }); }); }); From 76cab39ca6fbc9a8389ba8ca31ea3e41e65e6369 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 28 Feb 2023 16:41:11 -0500 Subject: [PATCH 06/19] test: connection creation does not set socketTimeoutMS --- test/unit/cmap/connect.test.ts | 49 +++++++++++++++++++++++++++++----- 1 file changed, 43 insertions(+), 6 deletions(-) diff --git a/test/unit/cmap/connect.test.ts b/test/unit/cmap/connect.test.ts index f87f8b77584..01313649bf2 100644 --- a/test/unit/cmap/connect.test.ts +++ b/test/unit/cmap/connect.test.ts @@ -18,6 +18,15 @@ import { import { genClusterTime } from '../../tools/common'; import * as mock from '../../tools/mongodb-mock/index'; +const CONNECT_DEFAULTS = { + id: 1, + tls: false, + generation: 1, + monitorCommands: false, + metadata: {} as ClientMetadata, + loadBalanced: false +}; + describe('Connect Tests', function () { context('when PLAIN auth enabled', () => { const test: { @@ -29,12 +38,7 @@ describe('Connect Tests', function () { const mockServer = await mock.createServer(); test.server = mockServer; test.connectOptions = { - id: 1, - tls: false, - generation: 1, - monitorCommands: false, - metadata: {} as ClientMetadata, - loadBalanced: false, + ...CONNECT_DEFAULTS, hostAddress: test.server.hostAddress() as HostAddress, credentials: new MongoCredentials({ username: 'testUser', @@ -112,6 +116,39 @@ describe('Connect Tests', function () { }); }); + context('when creating a connection', () => { + let server; + let connectOptions; + let connection: Connection; + + beforeEach(async () => { + server = await mock.createServer(); + server.setMessageHandler(request => { + if (isHello(request.document)) { + request.reply(mock.HELLO); + } + }); + connectOptions = { + ...CONNECT_DEFAULTS, + hostAddress: server.hostAddress() as HostAddress + }; + + connection = await promisify(callback => + //@ts-expect-error: Callbacks do not have mutual exclusion for error/result existence + connect(connectOptions, callback) + )(); + }); + + afterEach(async () => { + connection.destroy({ force: true }); + await mock.cleanup(); + }); + + it('creates a connection with an infinite timeout', async () => { + expect(connection.stream).to.have.property('timeout', 0); + }); + }); + it('should emit `MongoNetworkError` for network errors', function (done) { connect({ hostAddress: new HostAddress('non-existent:27018') }, err => { expect(err).to.be.instanceOf(MongoNetworkError); From 9fc784ea48b342dfc93495863a1a1da22e4b8a94 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 28 Feb 2023 17:07:54 -0500 Subject: [PATCH 07/19] fix: remove socketTimeoutOverride boolean --- src/cmap/connection.ts | 3 --- src/cmap/message_stream.ts | 1 - 2 files changed, 4 deletions(-) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index d2ad56e7e6a..6f64dea206d 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -358,8 +358,6 @@ export class Connection extends TypedEventEmitter { // back in the queue with the correct requestId and will resolve not being able // to find the next one via the responseTo of the next streaming hello. this[kQueue].set(message.requestId, operationDescription); - } else if (operationDescription.socketTimeoutOverride) { - this[kStream].setTimeout(this.socketTimeoutMS); } try { @@ -683,7 +681,6 @@ function write( } if (typeof options.socketTimeoutMS === 'number') { - operationDescription.socketTimeoutOverride = true; conn[kStream].setTimeout(options.socketTimeoutMS); } else if (conn.socketTimeoutMS !== 0) { conn[kStream].setTimeout(conn.socketTimeoutMS); diff --git a/src/cmap/message_stream.ts b/src/cmap/message_stream.ts index 409e9997778..1e62f6bd7b0 100644 --- a/src/cmap/message_stream.ts +++ b/src/cmap/message_stream.ts @@ -36,7 +36,6 @@ export interface OperationDescription extends BSONSerializeOptions { raw: boolean; requestId: number; session?: ClientSession; - socketTimeoutOverride?: boolean; agreedCompressor?: CompressorName; zlibCompressionLevel?: number; $clusterTime?: Document; From fa22da8670745c85850e64a26f42ba30df5364da Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 28 Feb 2023 17:08:56 -0500 Subject: [PATCH 08/19] test: ensure timeout is not set after creation, set before writing, and cleared after message is received --- test/unit/cmap/connect.test.ts | 77 +++++++++++++++++++++++++++++++++- 1 file changed, 76 insertions(+), 1 deletion(-) diff --git a/test/unit/cmap/connect.test.ts b/test/unit/cmap/connect.test.ts index 01313649bf2..6741678ef71 100644 --- a/test/unit/cmap/connect.test.ts +++ b/test/unit/cmap/connect.test.ts @@ -1,8 +1,10 @@ import { expect } from 'chai'; +import * as sinon from 'sinon'; import { setTimeout } from 'timers'; import { promisify } from 'util'; import { + BinMsg, CancellationToken, ClientMetadata, connect, @@ -13,10 +15,12 @@ import { LEGACY_HELLO_COMMAND, MongoCredentials, MongoNetworkError, + ns, prepareHandshakeDocument as prepareHandshakeDocumentCb } from '../../mongodb'; import { genClusterTime } from '../../tools/common'; import * as mock from '../../tools/mongodb-mock/index'; +import { generateOpMsgBuffer } from '../../tools/utils'; const CONNECT_DEFAULTS = { id: 1, @@ -130,7 +134,8 @@ describe('Connect Tests', function () { }); connectOptions = { ...CONNECT_DEFAULTS, - hostAddress: server.hostAddress() as HostAddress + hostAddress: server.hostAddress() as HostAddress, + socketTimeoutMS: 15000 }; connection = await promisify(callback => @@ -147,6 +152,76 @@ describe('Connect Tests', function () { it('creates a connection with an infinite timeout', async () => { expect(connection.stream).to.have.property('timeout', 0); }); + + it('connection instance has property socketTimeoutMS equal to the value passed in the connectOptions', async () => { + expect(connection).to.have.property('socketTimeoutMS', 15000); + }); + }); + + context('when sending commands on a connection', () => { + let server; + let connectOptions; + let connection: Connection; + let streamSetTimeoutSpy; + + beforeEach(async () => { + server = await mock.createServer(); + server.setMessageHandler(request => { + if (isHello(request.document)) { + request.reply(mock.HELLO); + } + }); + connectOptions = { + ...CONNECT_DEFAULTS, + hostAddress: server.hostAddress() as HostAddress + }; + + connection = await promisify(callback => + //@ts-expect-error: Callbacks do not have mutual exclusion for error/result existence + connect(connectOptions, callback) + )(); + + streamSetTimeoutSpy = sinon.spy(connection.stream, 'setTimeout'); + }); + + afterEach(async () => { + connection.destroy({ force: true }); + sinon.restore(); + await mock.cleanup(); + }); + + it('sets timeout specified on class before writing to the socket', async () => { + await promisify(callback => + connection.command(ns('admin.$cmd'), { hello: 1 }, {}, callback) + )(); + expect(streamSetTimeoutSpy).to.have.been.calledWith(15000); + }); + + it('sets timeout specified on options before writing to the socket', async () => { + await promisify(callback => + connection.command(ns('admin.$cmd'), { hello: 1 }, { socketTimeoutMS: 2000 }, callback) + )(); + expect(streamSetTimeoutSpy).to.have.been.calledWith(2000); + }); + + it('clears timeout after getting a message', async () => { + connection.stream.setTimeout(1); + const msg = generateOpMsgBuffer({ hello: 1 }); + const msgHeader = { + length: msg.readInt32LE(0), + requestId: 1, + responseTo: 0, + opCode: msg.readInt32LE(12) + }; + const msgBody = msg.subarray(16); + try { + connection.onMessage(new BinMsg(msg, msgHeader, msgBody)); + } catch { + // regardless of outcome + } + // timeout is still reset + expect(connection.stream).to.have.property('timeout', 0); + }); }); it('should emit `MongoNetworkError` for network errors', function (done) { From 6b91f05b835d0c39ae22fcba73be9b37e99d693b Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 28 Feb 2023 18:47:26 -0500 Subject: [PATCH 09/19] test: fix fake socket issues --- test/unit/cmap/connect.test.ts | 3 ++- test/unit/cmap/connection.test.ts | 16 +++++++++++++++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/test/unit/cmap/connect.test.ts b/test/unit/cmap/connect.test.ts index 6741678ef71..ada68de7a6e 100644 --- a/test/unit/cmap/connect.test.ts +++ b/test/unit/cmap/connect.test.ts @@ -173,7 +173,8 @@ describe('Connect Tests', function () { }); connectOptions = { ...CONNECT_DEFAULTS, - hostAddress: server.hostAddress() as HostAddress + hostAddress: server.hostAddress() as HostAddress, + socketTimeoutMS: 15000 }; connection = await promisify(callback => diff --git a/test/unit/cmap/connection.test.ts b/test/unit/cmap/connection.test.ts index 8298b13c778..e53e032fdf7 100644 --- a/test/unit/cmap/connection.test.ts +++ b/test/unit/cmap/connection.test.ts @@ -31,10 +31,15 @@ const connectionOptionsDefaults = { loadBalanced: false }; -/** The absolute minimum socket API needed by Connection as of writing this test */ +/** + * The absolute minimum socket API needed by these tests + * + * The driver has a greater API requirement for sockets detailed in: NODE-4785 + */ class FakeSocket extends EventEmitter { destroyed = false; writableEnded: boolean; + timeout = 0; address() { // is never called } @@ -59,10 +64,15 @@ class FakeSocket extends EventEmitter { get remotePort() { return 123; } + setTimeout(timeout) { + this.timeout = timeout; + } } class InputStream extends Readable { writableEnded: boolean; + timeout = 0; + constructor(options?) { super(options); } @@ -73,6 +83,10 @@ class InputStream extends Readable { process.nextTick(cb); } } + + setTimeout(timeout) { + this.timeout = timeout; + } } describe('new Connection()', function () { From f6bd7496b6b626b8577fdaba3259e8d07579ba24 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 1 Mar 2023 11:21:19 -0500 Subject: [PATCH 10/19] fix: don't reset timeout if moreToCome is set --- src/cmap/connection.ts | 4 ++-- test/unit/cmap/connect.test.ts | 22 +++++++++++++++++++++- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index 6f64dea206d..355d65b9a25 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -316,8 +316,6 @@ export class Connection extends TypedEventEmitter { this[kDelayedTimeoutId] = null; } - this[kStream].setTimeout(0); - // always emit the message, in case we are streaming this.emit('message', message); let operationDescription = this[kQueue].get(message.responseTo); @@ -358,6 +356,8 @@ export class Connection extends TypedEventEmitter { // back in the queue with the correct requestId and will resolve not being able // to find the next one via the responseTo of the next streaming hello. this[kQueue].set(message.requestId, operationDescription); + } else { + this[kStream].setTimeout(0); } try { diff --git a/test/unit/cmap/connect.test.ts b/test/unit/cmap/connect.test.ts index ada68de7a6e..88a6dd7a0a6 100644 --- a/test/unit/cmap/connect.test.ts +++ b/test/unit/cmap/connect.test.ts @@ -205,7 +205,7 @@ describe('Connect Tests', function () { expect(streamSetTimeoutSpy).to.have.been.calledWith(2000); }); - it('clears timeout after getting a message', async () => { + it('clears timeout after getting a message if moreToCome=false', async () => { connection.stream.setTimeout(1); const msg = generateOpMsgBuffer({ hello: 1 }); const msgHeader = { @@ -223,6 +223,26 @@ describe('Connect Tests', function () { // timeout is still reset expect(connection.stream).to.have.property('timeout', 0); }); + + it('does not clear timeout after getting a message if moreToCome=true', async () => { + connection.stream.setTimeout(1); + const msg = generateOpMsgBuffer({ hello: 1 }); + const msgHeader = { + length: msg.readInt32LE(0), + requestId: 1, + responseTo: 0, + opCode: msg.readInt32LE(12) + }; + const msgBody = msg.subarray(16); + msgBody.writeInt32LE(2); // OPTS_MORE_TO_COME + try { + connection.onMessage(new BinMsg(msg, msgHeader, msgBody)); + } catch { + // regardless of outcome + } + // timeout is still set + expect(connection.stream).to.have.property('timeout', 1); + }); }); it('should emit `MongoNetworkError` for network errors', function (done) { From 51277912815ad9464f63923da89b2b35e401f7a0 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 1 Mar 2023 12:21:57 -0500 Subject: [PATCH 11/19] test: reorg --- test/unit/cmap/connect.test.ts | 91 ---------------------------- test/unit/cmap/connection.test.ts | 98 +++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 91 deletions(-) diff --git a/test/unit/cmap/connect.test.ts b/test/unit/cmap/connect.test.ts index 88a6dd7a0a6..028d008ef66 100644 --- a/test/unit/cmap/connect.test.ts +++ b/test/unit/cmap/connect.test.ts @@ -1,10 +1,8 @@ import { expect } from 'chai'; -import * as sinon from 'sinon'; import { setTimeout } from 'timers'; import { promisify } from 'util'; import { - BinMsg, CancellationToken, ClientMetadata, connect, @@ -15,12 +13,10 @@ import { LEGACY_HELLO_COMMAND, MongoCredentials, MongoNetworkError, - ns, prepareHandshakeDocument as prepareHandshakeDocumentCb } from '../../mongodb'; import { genClusterTime } from '../../tools/common'; import * as mock from '../../tools/mongodb-mock/index'; -import { generateOpMsgBuffer } from '../../tools/utils'; const CONNECT_DEFAULTS = { id: 1, @@ -158,93 +154,6 @@ describe('Connect Tests', function () { }); }); - context('when sending commands on a connection', () => { - let server; - let connectOptions; - let connection: Connection; - let streamSetTimeoutSpy; - - beforeEach(async () => { - server = await mock.createServer(); - server.setMessageHandler(request => { - if (isHello(request.document)) { - request.reply(mock.HELLO); - } - }); - connectOptions = { - ...CONNECT_DEFAULTS, - hostAddress: server.hostAddress() as HostAddress, - socketTimeoutMS: 15000 - }; - - connection = await promisify(callback => - //@ts-expect-error: Callbacks do not have mutual exclusion for error/result existence - connect(connectOptions, callback) - )(); - - streamSetTimeoutSpy = sinon.spy(connection.stream, 'setTimeout'); - }); - - afterEach(async () => { - connection.destroy({ force: true }); - sinon.restore(); - await mock.cleanup(); - }); - - it('sets timeout specified on class before writing to the socket', async () => { - await promisify(callback => - connection.command(ns('admin.$cmd'), { hello: 1 }, {}, callback) - )(); - expect(streamSetTimeoutSpy).to.have.been.calledWith(15000); - }); - - it('sets timeout specified on options before writing to the socket', async () => { - await promisify(callback => - connection.command(ns('admin.$cmd'), { hello: 1 }, { socketTimeoutMS: 2000 }, callback) - )(); - expect(streamSetTimeoutSpy).to.have.been.calledWith(2000); - }); - - it('clears timeout after getting a message if moreToCome=false', async () => { - connection.stream.setTimeout(1); - const msg = generateOpMsgBuffer({ hello: 1 }); - const msgHeader = { - length: msg.readInt32LE(0), - requestId: 1, - responseTo: 0, - opCode: msg.readInt32LE(12) - }; - const msgBody = msg.subarray(16); - try { - connection.onMessage(new BinMsg(msg, msgHeader, msgBody)); - } catch { - // regardless of outcome - } - // timeout is still reset - expect(connection.stream).to.have.property('timeout', 0); - }); - - it('does not clear timeout after getting a message if moreToCome=true', async () => { - connection.stream.setTimeout(1); - const msg = generateOpMsgBuffer({ hello: 1 }); - const msgHeader = { - length: msg.readInt32LE(0), - requestId: 1, - responseTo: 0, - opCode: msg.readInt32LE(12) - }; - const msgBody = msg.subarray(16); - msgBody.writeInt32LE(2); // OPTS_MORE_TO_COME - try { - connection.onMessage(new BinMsg(msg, msgHeader, msgBody)); - } catch { - // regardless of outcome - } - // timeout is still set - expect(connection.stream).to.have.property('timeout', 1); - }); - }); - it('should emit `MongoNetworkError` for network errors', function (done) { connect({ hostAddress: new HostAddress('non-existent:27018') }, err => { expect(err).to.be.instanceOf(MongoNetworkError); diff --git a/test/unit/cmap/connection.test.ts b/test/unit/cmap/connection.test.ts index e53e032fdf7..fbd689a11fc 100644 --- a/test/unit/cmap/connection.test.ts +++ b/test/unit/cmap/connection.test.ts @@ -4,12 +4,15 @@ import { Socket } from 'net'; import * as sinon from 'sinon'; import { Readable } from 'stream'; import { setTimeout } from 'timers'; +import { promisify } from 'util'; import { BinMsg, + ClientMetadata, connect, Connection, hasSessionSupport, + HostAddress, isHello, MessageStream, MongoNetworkError, @@ -413,6 +416,101 @@ describe('new Connection()', function () { }); }); }); + + context('when sending commands on a connection', () => { + const CONNECT_DEFAULTS = { + id: 1, + tls: false, + generation: 1, + monitorCommands: false, + metadata: {} as ClientMetadata, + loadBalanced: false + }; + let server; + let connectOptions; + let connection: Connection; + let streamSetTimeoutSpy; + + beforeEach(async () => { + server = await mock.createServer(); + server.setMessageHandler(request => { + if (isHello(request.document)) { + request.reply(mock.HELLO); + } + }); + connectOptions = { + ...CONNECT_DEFAULTS, + hostAddress: server.hostAddress() as HostAddress, + socketTimeoutMS: 15000 + }; + + connection = await promisify(callback => + //@ts-expect-error: Callbacks do not have mutual exclusion for error/result existence + connect(connectOptions, callback) + )(); + + streamSetTimeoutSpy = sinon.spy(connection.stream, 'setTimeout'); + }); + + afterEach(async () => { + connection.destroy({ force: true }); + sinon.restore(); + await mock.cleanup(); + }); + + it('sets timeout specified on class before writing to the socket', async () => { + await promisify(callback => + connection.command(ns('admin.$cmd'), { hello: 1 }, {}, callback) + )(); + expect(streamSetTimeoutSpy).to.have.been.calledWith(15000); + }); + + it('sets timeout specified on options before writing to the socket', async () => { + await promisify(callback => + connection.command(ns('admin.$cmd'), { hello: 1 }, { socketTimeoutMS: 2000 }, callback) + )(); + expect(streamSetTimeoutSpy).to.have.been.calledWith(2000); + }); + + it('clears timeout after getting a message if moreToCome=false', async () => { + connection.stream.setTimeout(1); + const msg = generateOpMsgBuffer({ hello: 1 }); + const msgHeader = { + length: msg.readInt32LE(0), + requestId: 1, + responseTo: 0, + opCode: msg.readInt32LE(12) + }; + const msgBody = msg.subarray(16); + try { + connection.onMessage(new BinMsg(msg, msgHeader, msgBody)); + } catch { + // regardless of outcome + } + // timeout is still reset + expect(connection.stream).to.have.property('timeout', 0); + }); + + it('does not clear timeout after getting a message if moreToCome=true', async () => { + connection.stream.setTimeout(1); + const msg = generateOpMsgBuffer({ hello: 1 }); + const msgHeader = { + length: msg.readInt32LE(0), + requestId: 1, + responseTo: 0, + opCode: msg.readInt32LE(12) + }; + const msgBody = msg.subarray(16); + msgBody.writeInt32LE(2); // OPTS_MORE_TO_COME + try { + connection.onMessage(new BinMsg(msg, msgHeader, msgBody)); + } catch { + // regardless of outcome + } + // timeout is still set + expect(connection.stream).to.have.property('timeout', 1); + }); + }); }); describe('when the socket times out', () => { From ce176594781505d39f9f4fc996f31ccf0290e389 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 2 Mar 2023 10:13:42 -0500 Subject: [PATCH 12/19] fix: set 0 timeout on function exit --- src/cmap/connection.ts | 1 + test/unit/cmap/connection.test.ts | 15 ++++----------- 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index 355d65b9a25..71b2187442b 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -341,6 +341,7 @@ export class Connection extends TypedEventEmitter { } if (!operationDescription) { + this[kStream].setTimeout(0); return; } diff --git a/test/unit/cmap/connection.test.ts b/test/unit/cmap/connection.test.ts index fbd689a11fc..1c9ca0c5bab 100644 --- a/test/unit/cmap/connection.test.ts +++ b/test/unit/cmap/connection.test.ts @@ -482,11 +482,8 @@ describe('new Connection()', function () { opCode: msg.readInt32LE(12) }; const msgBody = msg.subarray(16); - try { - connection.onMessage(new BinMsg(msg, msgHeader, msgBody)); - } catch { - // regardless of outcome - } + msgBody.writeInt32LE(0, 0); // OPTS_MORE_TO_COME + connection.onMessage(new BinMsg(msg, msgHeader, msgBody)); // timeout is still reset expect(connection.stream).to.have.property('timeout', 0); }); @@ -501,12 +498,8 @@ describe('new Connection()', function () { opCode: msg.readInt32LE(12) }; const msgBody = msg.subarray(16); - msgBody.writeInt32LE(2); // OPTS_MORE_TO_COME - try { - connection.onMessage(new BinMsg(msg, msgHeader, msgBody)); - } catch { - // regardless of outcome - } + msgBody.writeInt32LE(2, 0); // OPTS_MORE_TO_COME + connection.onMessage(new BinMsg(msg, msgHeader, msgBody)); // timeout is still set expect(connection.stream).to.have.property('timeout', 1); }); From 910dfaf9291679aea501379f91db61b9c7227164 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 2 Mar 2023 10:56:21 -0500 Subject: [PATCH 13/19] fix: 0 timeout always, reset if moreToCome is set --- src/cmap/connection.ts | 7 ++++--- test/unit/cmap/connection.test.ts | 1 + 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index 71b2187442b..b31f1f170bc 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -316,6 +316,9 @@ export class Connection extends TypedEventEmitter { this[kDelayedTimeoutId] = null; } + const socketTimeoutMS = this[kStream].timeout ?? 0; + this[kStream].setTimeout(0); + // always emit the message, in case we are streaming this.emit('message', message); let operationDescription = this[kQueue].get(message.responseTo); @@ -341,7 +344,6 @@ export class Connection extends TypedEventEmitter { } if (!operationDescription) { - this[kStream].setTimeout(0); return; } @@ -357,8 +359,7 @@ export class Connection extends TypedEventEmitter { // back in the queue with the correct requestId and will resolve not being able // to find the next one via the responseTo of the next streaming hello. this[kQueue].set(message.requestId, operationDescription); - } else { - this[kStream].setTimeout(0); + this[kStream].setTimeout(socketTimeoutMS); } try { diff --git a/test/unit/cmap/connection.test.ts b/test/unit/cmap/connection.test.ts index 1c9ca0c5bab..7a8f41310e5 100644 --- a/test/unit/cmap/connection.test.ts +++ b/test/unit/cmap/connection.test.ts @@ -499,6 +499,7 @@ describe('new Connection()', function () { }; const msgBody = msg.subarray(16); msgBody.writeInt32LE(2, 0); // OPTS_MORE_TO_COME + connection[getSymbolFrom(connection, 'queue')].set(0, { cb: () => null }); connection.onMessage(new BinMsg(msg, msgHeader, msgBody)); // timeout is still set expect(connection.stream).to.have.property('timeout', 1); From 63a3a6e2a8569c3d427e18f9948e640efb009c09 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 2 Mar 2023 14:07:21 -0500 Subject: [PATCH 14/19] test: fix cancellationToken test --- test/unit/cmap/connect.test.ts | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/test/unit/cmap/connect.test.ts b/test/unit/cmap/connect.test.ts index 028d008ef66..5b6fb1cd9a2 100644 --- a/test/unit/cmap/connect.test.ts +++ b/test/unit/cmap/connect.test.ts @@ -152,6 +152,21 @@ describe('Connect Tests', function () { it('connection instance has property socketTimeoutMS equal to the value passed in the connectOptions', async () => { expect(connection).to.have.property('socketTimeoutMS', 15000); }); + + it('cancels connecting if provided cancellationToken emits cancel', async () => { + // set no response handler for mock server, effectively black hole requests + server.setMessageHandler(() => null); + + const cancellationToken = new CancellationToken(); + setTimeout(() => cancellationToken.emit('cancel'), 5); + + const error = await promisify(callback => + //@ts-expect-error: Callbacks do not have mutual exclusion for error/result existence + connect({ ...connectOptions, cancellationToken }, callback) + )().catch(error => error); + + expect(error).to.match(/connection establishment was cancelled/); + }); }); it('should emit `MongoNetworkError` for network errors', function (done) { @@ -161,19 +176,6 @@ describe('Connect Tests', function () { }); }); - it.skip('should allow a cancellaton token', function (done) { - const cancellationToken = new CancellationToken(); - setTimeout(() => cancellationToken.emit('cancel'), 500); - // set no response handler for mock server, effectively black hole requests - - connect({ hostAddress: new HostAddress('240.0.0.1'), cancellationToken }, (err, conn) => { - expect(err).to.exist; - expect(err).to.match(/connection establishment was cancelled/); - expect(conn).to.not.exist; - done(); - }); - }).skipReason = 'TODO(NODE-2941): stop using 240.0.0.1 in tests'; - context('prepareHandshakeDocument', () => { const prepareHandshakeDocument = promisify(prepareHandshakeDocumentCb); From b1377f99f91055fcd2242c5dbe7552e55d208a1e Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 2 Mar 2023 14:12:16 -0500 Subject: [PATCH 15/19] test: fix other 240.0.0.1 test --- .../connection.test.ts | 22 ------------------- test/unit/cmap/connect.test.ts | 12 ++++++++++ 2 files changed, 12 insertions(+), 22 deletions(-) diff --git a/test/integration/connection-monitoring-and-pooling/connection.test.ts b/test/integration/connection-monitoring-and-pooling/connection.test.ts index 4b0133892bd..8a4f1d10e5a 100644 --- a/test/integration/connection-monitoring-and-pooling/connection.test.ts +++ b/test/integration/connection-monitoring-and-pooling/connection.test.ts @@ -79,28 +79,6 @@ describe('Connection', function () { } }); - it.skip('should support socket timeouts', { - // FIXME: NODE-2941 - metadata: { - requires: { - os: '!win32' // 240.0.0.1 doesnt work for windows - } - }, - test: function (done) { - const connectOptions = { - hostAddress: new HostAddress('240.0.0.1'), - connectionType: Connection, - connectionTimeout: 500 - }; - - connect(connectOptions, err => { - expect(err).to.exist; - expect(err).to.match(/timed out/); - done(); - }); - } - }); - it('should support calling back multiple times on exhaust commands', { metadata: { requires: { apiVersion: false, mongodb: '>=4.2.0', topology: ['single'] } diff --git a/test/unit/cmap/connect.test.ts b/test/unit/cmap/connect.test.ts index 5b6fb1cd9a2..6177b1cd148 100644 --- a/test/unit/cmap/connect.test.ts +++ b/test/unit/cmap/connect.test.ts @@ -167,6 +167,18 @@ describe('Connect Tests', function () { expect(error).to.match(/connection establishment was cancelled/); }); + + it('interrupts connecting based on connectionTimeoutMS setting', async () => { + // set no response handler for mock server, effectively black hole requests + server.setMessageHandler(() => null); + + const error = await promisify(callback => + //@ts-expect-error: Callbacks do not have mutual exclusion for error/result existence + connect({ ...connectOptions, connectTimeoutMS: 5 }, callback) + )().catch(error => error); + + expect(error).to.match(/timed out/); + }); }); it('should emit `MongoNetworkError` for network errors', function (done) { From f43d491c8724bf3f3d0dac98341e5cbc8e047efd Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 2 Mar 2023 15:22:16 -0500 Subject: [PATCH 16/19] test: fix cancellation token test --- .../connection.test.ts | 1 - test/unit/cmap/connect.test.ts | 17 +++++++++++++---- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/test/integration/connection-monitoring-and-pooling/connection.test.ts b/test/integration/connection-monitoring-and-pooling/connection.test.ts index 8a4f1d10e5a..b13ee006b33 100644 --- a/test/integration/connection-monitoring-and-pooling/connection.test.ts +++ b/test/integration/connection-monitoring-and-pooling/connection.test.ts @@ -3,7 +3,6 @@ import { expect } from 'chai'; import { connect, Connection, - HostAddress, LEGACY_HELLO_COMMAND, MongoClient, MongoServerError, diff --git a/test/unit/cmap/connect.test.ts b/test/unit/cmap/connect.test.ts index 6177b1cd148..f191bb50ffe 100644 --- a/test/unit/cmap/connect.test.ts +++ b/test/unit/cmap/connect.test.ts @@ -153,16 +153,25 @@ describe('Connect Tests', function () { expect(connection).to.have.property('socketTimeoutMS', 15000); }); - it('cancels connecting if provided cancellationToken emits cancel', async () => { + it.only('cancels connecting if provided cancellationToken emits cancel', async () => { // set no response handler for mock server, effectively black hole requests server.setMessageHandler(() => null); const cancellationToken = new CancellationToken(); - setTimeout(() => cancellationToken.emit('cancel'), 5); + setTimeout(() => cancellationToken.emit('cancel'), 500); const error = await promisify(callback => - //@ts-expect-error: Callbacks do not have mutual exclusion for error/result existence - connect({ ...connectOptions, cancellationToken }, callback) + connect( + { + ...connectOptions, + // Ensure these timeouts do not fire first + socketTimeoutMS: 1000, + connectTimeoutMS: 1000, + cancellationToken + }, + //@ts-expect-error: Callbacks do not have mutual exclusion for error/result existence + callback + ) )().catch(error => error); expect(error).to.match(/connection establishment was cancelled/); From 46a37d3b3f903c8c0349e97d215fe0c95c79654b Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 2 Mar 2023 17:24:35 -0500 Subject: [PATCH 17/19] test fix --- test/unit/cmap/connect.test.ts | 64 ++++++++++++++++++---------------- 1 file changed, 34 insertions(+), 30 deletions(-) diff --git a/test/unit/cmap/connect.test.ts b/test/unit/cmap/connect.test.ts index f191bb50ffe..bb923a96721 100644 --- a/test/unit/cmap/connect.test.ts +++ b/test/unit/cmap/connect.test.ts @@ -153,40 +153,44 @@ describe('Connect Tests', function () { expect(connection).to.have.property('socketTimeoutMS', 15000); }); - it.only('cancels connecting if provided cancellationToken emits cancel', async () => { - // set no response handler for mock server, effectively black hole requests - server.setMessageHandler(() => null); - - const cancellationToken = new CancellationToken(); - setTimeout(() => cancellationToken.emit('cancel'), 500); - - const error = await promisify(callback => - connect( - { - ...connectOptions, - // Ensure these timeouts do not fire first - socketTimeoutMS: 1000, - connectTimeoutMS: 1000, - cancellationToken - }, - //@ts-expect-error: Callbacks do not have mutual exclusion for error/result existence - callback - ) - )().catch(error => error); - - expect(error).to.match(/connection establishment was cancelled/); + context('when the provided cancellation token emits cancel', () => { + it('interrupts the connection with an error', async () => { + // set no response handler for mock server, effectively black hole requests + server.setMessageHandler(() => null); + + const cancellationToken = new CancellationToken(); + setTimeout(() => cancellationToken.emit('cancel'), 500); + + const error = await promisify(callback => + connect( + { + ...connectOptions, + // Ensure these timeouts do not fire first + socketTimeoutMS: 1000, + connectTimeoutMS: 1000, + cancellationToken + }, + //@ts-expect-error: Callbacks do not have mutual exclusion for error/result existence + callback + ) + )().catch(error => error); + + expect(error).to.match(/connection establishment was cancelled/); + }); }); - it('interrupts connecting based on connectionTimeoutMS setting', async () => { - // set no response handler for mock server, effectively black hole requests - server.setMessageHandler(() => null); + context('when connecting takes longer than connectTimeoutMS', () => { + it('interrupts the connection with an error', async () => { + // set no response handler for mock server, effectively black hole requests + server.setMessageHandler(() => null); - const error = await promisify(callback => - //@ts-expect-error: Callbacks do not have mutual exclusion for error/result existence - connect({ ...connectOptions, connectTimeoutMS: 5 }, callback) - )().catch(error => error); + const error = await promisify(callback => + //@ts-expect-error: Callbacks do not have mutual exclusion for error/result existence + connect({ ...connectOptions, connectTimeoutMS: 5 }, callback) + )().catch(error => error); - expect(error).to.match(/timed out/); + expect(error).to.match(/timed out/); + }); }); }); From 104582da0955e440990b42d513dbeea4266e0b56 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 3 Mar 2023 12:07:07 -0500 Subject: [PATCH 18/19] test: fix unit test --- test/unit/cmap/connect.test.ts | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/test/unit/cmap/connect.test.ts b/test/unit/cmap/connect.test.ts index bb923a96721..02a35dce51d 100644 --- a/test/unit/cmap/connect.test.ts +++ b/test/unit/cmap/connect.test.ts @@ -159,15 +159,20 @@ describe('Connect Tests', function () { server.setMessageHandler(() => null); const cancellationToken = new CancellationToken(); - setTimeout(() => cancellationToken.emit('cancel'), 500); + // Make sure the cancel listener is added before emitting cancel + cancellationToken.addListener('newListener', () => { + process.nextTick(() => { + cancellationToken.emit('cancel'); + }); + }); const error = await promisify(callback => connect( { ...connectOptions, // Ensure these timeouts do not fire first - socketTimeoutMS: 1000, - connectTimeoutMS: 1000, + socketTimeoutMS: 5000, + connectTimeoutMS: 5000, cancellationToken }, //@ts-expect-error: Callbacks do not have mutual exclusion for error/result existence @@ -175,7 +180,7 @@ describe('Connect Tests', function () { ) )().catch(error => error); - expect(error).to.match(/connection establishment was cancelled/); + expect(error, error.stack).to.match(/connection establishment was cancelled/); }); }); From eba8d8966d70892b92b99ed52d788aff34b84332 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 3 Mar 2023 12:24:11 -0500 Subject: [PATCH 19/19] fix: lint --- test/unit/cmap/connect.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/test/unit/cmap/connect.test.ts b/test/unit/cmap/connect.test.ts index 02a35dce51d..9a038951a3a 100644 --- a/test/unit/cmap/connect.test.ts +++ b/test/unit/cmap/connect.test.ts @@ -1,5 +1,4 @@ import { expect } from 'chai'; -import { setTimeout } from 'timers'; import { promisify } from 'util'; import {