From 413059787acb619bcfe7e2ea9244df6a2441a204 Mon Sep 17 00:00:00 2001 From: Warren James Date: Mon, 5 Dec 2022 16:11:50 -0500 Subject: [PATCH 01/36] fix(NODE-4834): Add calls to destroy kMessageStream --- src/cmap/connection.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index 85f6000817..79a130e013 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -314,6 +314,7 @@ export class Connection extends TypedEventEmitter { return; } + this[kMessageStream].destroy(error); this[kStream].destroy(error); this.closed = true; @@ -330,6 +331,7 @@ export class Connection extends TypedEventEmitter { if (this.closed) { return; } + this[kMessageStream].destroy(); this.closed = true; @@ -348,6 +350,7 @@ export class Connection extends TypedEventEmitter { } this[kDelayedTimeoutId] = setTimeout(() => { + this[kMessageStream].destroy(); this[kStream].destroy(); this.closed = true; @@ -467,6 +470,8 @@ export class Connection extends TypedEventEmitter { this.removeAllListeners(Connection.PINNED); this.removeAllListeners(Connection.UNPINNED); + + this[kMessageStream].destroy(); options = Object.assign({ force: false }, options); if (this[kStream] == null || this.destroyed) { From 4c1023574a01718d10c36ee3b1a404bc2e905123 Mon Sep 17 00:00:00 2001 From: Warren James Date: Mon, 5 Dec 2022 16:13:45 -0500 Subject: [PATCH 02/36] test(NODE-4834): WIP - Start work on unit tests --- test/unit/cmap/connection.test.ts | 167 +++++++++++++++++++++++------- 1 file changed, 132 insertions(+), 35 deletions(-) diff --git a/test/unit/cmap/connection.test.ts b/test/unit/cmap/connection.test.ts index 5c8d872bb8..5d7219fb8d 100644 --- a/test/unit/cmap/connection.test.ts +++ b/test/unit/cmap/connection.test.ts @@ -43,12 +43,12 @@ class FakeSocket extends EventEmitter { } } -describe('new Connection()', function () { +describe('new Connection()', function() { let server; after(() => mock.cleanup()); before(() => mock.createServer().then(s => (server = s))); - it('should support fire-and-forget messages', function (done) { + it('should support fire-and-forget messages', function(done) { server.setMessageHandler(request => { const doc = request.document; if (isHello(doc)) { @@ -77,7 +77,7 @@ describe('new Connection()', function () { }); }); - it('should destroy streams which time out', function (done) { + it('should destroy streams which time out', function(done) { server.setMessageHandler(request => { const doc = request.document; if (isHello(doc)) { @@ -108,7 +108,7 @@ describe('new Connection()', function () { }); }); - it('should throw a network error with kBeforeHandshake set to false on timeout after handshake', function (done) { + it('should throw a network error with kBeforeHandshake set to false on timeout after handshake', function(done) { server.setMessageHandler(request => { const doc = request.document; if (isHello(doc)) { @@ -137,7 +137,7 @@ describe('new Connection()', function () { }); }); - it('should throw a network error with kBeforeHandshake set to true on timeout before handshake', function (done) { + it('should throw a network error with kBeforeHandshake set to true on timeout before handshake', function(done) { server.setMessageHandler(() => { // respond to no requests to trigger timeout event }); @@ -158,23 +158,23 @@ describe('new Connection()', function () { }); }); - describe('#onMessage', function () { - context('when the connection is a monitoring connection', function () { + describe('#onMessage', function() { + context('when the connection is a monitoring connection', function() { let queue: Map; let driverSocket: FakeSocket; let connection: Connection; - beforeEach(function () { + beforeEach(function() { driverSocket = sinon.spy(new FakeSocket()); }); - context('when multiple hellos exist on the stream', function () { + context('when multiple hellos exist on the stream', function() { let callbackSpy; const inputStream = new Readable(); const document = { ok: 1 }; const last = { isWritablePrimary: true }; - beforeEach(function () { + beforeEach(function() { callbackSpy = sinon.spy(); const firstHello = generateOpMsgBuffer(document); const secondHello = generateOpMsgBuffer(document); @@ -200,18 +200,18 @@ describe('new Connection()', function () { inputStream.push(null); }); - it('calls the callback with the last hello document', async function () { + it('calls the callback with the last hello document', async function() { const messages = await once(connection, 'message'); expect(messages[0].responseTo).to.equal(0); expect(callbackSpy).to.be.calledOnceWith(undefined, last); }); }); - context('when requestId/responseTo do not match', function () { + context('when requestId/responseTo do not match', function() { let callbackSpy; const document = { ok: 1 }; - beforeEach(function () { + beforeEach(function() { callbackSpy = sinon.spy(); // @ts-expect-error: driverSocket does not fully satisfy the stream type, but that's okay @@ -242,16 +242,16 @@ describe('new Connection()', function () { connection.onMessage(message); }); - it('calls the operation description callback with the document', function () { + it('calls the operation description callback with the document', function() { expect(callbackSpy).to.be.calledOnceWith(undefined, document); }); }); - context('when requestId/reponseTo match', function () { + context('when requestId/reponseTo match', function() { let callbackSpy; const document = { ok: 1 }; - beforeEach(function () { + beforeEach(function() { callbackSpy = sinon.spy(); // @ts-expect-error: driverSocket does not fully satisfy the stream type, but that's okay @@ -282,15 +282,15 @@ describe('new Connection()', function () { connection.onMessage(message); }); - it('calls the operation description callback with the document', function () { + it('calls the operation description callback with the document', function() { expect(callbackSpy).to.be.calledOnceWith(undefined, document); }); }); - context('when no operation description is in the queue', function () { + context('when no operation description is in the queue', function() { const document = { ok: 1 }; - beforeEach(function () { + beforeEach(function() { // @ts-expect-error: driverSocket does not fully satisfy the stream type, but that's okay connection = sinon.spy(new Connection(driverSocket, connectionOptionsDefaults)); connection.isMonitoringConnection = true; @@ -298,7 +298,7 @@ describe('new Connection()', function () { queue = connection[queueSymbol]; }); - it('does not error', function () { + it('does not error', function() { const msg = generateOpMsgBuffer(document); const msgHeader: MessageHeader = { length: msg.readInt32LE(0), @@ -315,12 +315,12 @@ describe('new Connection()', function () { }); }); - context('when more than one operation description is in the queue', function () { + context('when more than one operation description is in the queue', function() { let spyOne; let spyTwo; const document = { ok: 1 }; - beforeEach(function () { + beforeEach(function() { spyOne = sinon.spy(); spyTwo = sinon.spy(); @@ -357,7 +357,7 @@ describe('new Connection()', function () { connection.onMessage(message); }); - it('calls all operation description callbacks with an error', function () { + it('calls all operation description callbacks with an error', function() { expect(spyOne).to.be.calledOnce; expect(spyTwo).to.be.calledOnce; const errorOne = spyOne.firstCall.args[0]; @@ -389,7 +389,7 @@ describe('new Connection()', function () { connection = sinon.spy(new Connection(driverSocket, connectionOptionsDefaults)); const messageStreamSymbol = getSymbolFrom(connection, 'messageStream'); kDelayedTimeoutId = getSymbolFrom(connection, 'delayedTimeoutId'); - messageStream = connection[messageStreamSymbol]; + messageStream = sinon.spy(connection[messageStreamSymbol]); }); afterEach(() => { @@ -433,14 +433,106 @@ describe('new Connection()', function () { expect(connection).to.have.property('closed', false); expect(connection).to.have.property(kDelayedTimeoutId, null); }); + + // TODO(NODE-4834): Ask if this test should go in the test above + it('should destroy message stream and socket', () => { + expect(connection).to.have.property(kDelayedTimeoutId, null); + + driverSocket.emit('timeout'); + + clock.tick(1); + + expect(connection.onTimeout).to.have.been.calledOnce; + expect(connection).to.have.property(kDelayedTimeoutId).that.is.instanceOf(NodeJSTimeoutClass); + + expect(messageStream.destroy).to.have.been.calledOnce; + expect(driverSocket.destroy).to.have.been.calledOnce; + }); }); - describe('.hasSessionSupport', function () { + describe('onError()', () => { + let connection: sinon.SinonSpiedInstance; + let clock: sinon.SinonFakeTimers; + let timerSandbox: sinon.SinonFakeTimers; + let driverSocket: sinon.SinonSpiedInstance; + let messageStream: MessageStream; + let kDelayedTimeoutId: symbol; + let NodeJSTimeoutClass: any; + beforeEach(() => { + timerSandbox = createTimerSandbox(); + clock = sinon.useFakeTimers(); + + NodeJSTimeoutClass = setTimeout(() => null, 1).constructor; + + driverSocket = sinon.spy(new FakeSocket()); + // @ts-expect-error: driverSocket does not fully satisfy the stream type, but that's okay + connection = sinon.spy(new Connection(driverSocket, connectionOptionsDefaults)); + const messageStreamSymbol = getSymbolFrom(connection, 'messageStream'); + kDelayedTimeoutId = getSymbolFrom(connection, 'delayedTimeoutId'); + messageStream = sinon.spy(connection[messageStreamSymbol]); + }); + + afterEach(() => { + timerSandbox.restore(); + clock.restore(); + }); + + it('should destroy message stream and socket', () => { + messageStream.emit('error'); + + clock.tick(1); + + expect(connection.onError).to.have.been.calledOnce; + + expect(messageStream.destroy).to.have.been.calledOnce; + expect(driverSocket.destroy).to.have.been.calledOnce; + }) + }); + + describe('onClose()', () => { + let connection: sinon.SinonSpiedInstance; + let clock: sinon.SinonFakeTimers; + let timerSandbox: sinon.SinonFakeTimers; + let driverSocket: sinon.SinonSpiedInstance; + let messageStream: MessageStream; + let kDelayedTimeoutId: symbol; + let NodeJSTimeoutClass: any; + beforeEach(() => { + timerSandbox = createTimerSandbox(); + clock = sinon.useFakeTimers(); + + NodeJSTimeoutClass = setTimeout(() => null, 1).constructor; + + driverSocket = sinon.spy(new FakeSocket()); + // @ts-expect-error: driverSocket does not fully satisfy the stream type, but that's okay + connection = sinon.spy(new Connection(driverSocket, connectionOptionsDefaults)); + const messageStreamSymbol = getSymbolFrom(connection, 'messageStream'); + kDelayedTimeoutId = getSymbolFrom(connection, 'delayedTimeoutId'); + messageStream = sinon.spy(connection[messageStreamSymbol]); + }); + + afterEach(() => { + timerSandbox.restore(); + clock.restore(); + }); + + it('should destroy message stream', () => { + driverSocket.emit('close'); + + clock.tick(1); + + expect(connection.onClose).to.have.been.calledOnce; + + expect(messageStream.destroy).to.have.been.calledOnce; + }) + }); + + describe('.hasSessionSupport', function() { let connection; const stream = new Socket(); - context('when logicalSessionTimeoutMinutes is present', function () { - beforeEach(function () { + context('when logicalSessionTimeoutMinutes is present', function() { + beforeEach(function() { const options = { ...connectionOptionsDefaults, hostAddress: server.hostAddress(), @@ -449,14 +541,14 @@ describe('new Connection()', function () { connection = new Connection(stream, options); }); - it('returns true', function () { + it('returns true', function() { expect(hasSessionSupport(connection)).to.be.true; }); }); - context('when logicalSessionTimeoutMinutes is not present', function () { - context('when in load balancing mode', function () { - beforeEach(function () { + context('when logicalSessionTimeoutMinutes is not present', function() { + context('when in load balancing mode', function() { + beforeEach(function() { const options = { ...connectionOptionsDefaults, hostAddress: server.hostAddress(), @@ -465,13 +557,13 @@ describe('new Connection()', function () { connection = new Connection(stream, options); }); - it('returns true', function () { + it('returns true', function() { expect(hasSessionSupport(connection)).to.be.true; }); }); - context('when not in load balancing mode', function () { - beforeEach(function () { + context('when not in load balancing mode', function() { + beforeEach(function() { const options = { ...connectionOptionsDefaults, hostAddress: server.hostAddress(), @@ -480,10 +572,15 @@ describe('new Connection()', function () { connection = new Connection(stream, options); }); - it('returns false', function () { + it('returns false', function() { expect(hasSessionSupport(connection)).to.be.false; }); }); }); }); + + // TODO(NODE-4834): Implement me + describe('destroy()', () => { + it('should end tcp socket and destroy messageStream', () => { }); + }); }); From b341938f6fe492756f10eb3aa9017370e070d45e Mon Sep 17 00:00:00 2001 From: Warren James Date: Tue, 6 Dec 2022 14:37:10 -0500 Subject: [PATCH 03/36] fix(NODE-4834): Add guards to onError, onClose, onTimeout --- src/cmap/connection.ts | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index 79a130e013..f7be6260b2 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -310,7 +310,7 @@ export class Connection extends TypedEventEmitter { } onError(error: Error) { - if (this.closed) { + if (this.closed || this.destroyed) { return; } @@ -328,7 +328,7 @@ export class Connection extends TypedEventEmitter { } onClose() { - if (this.closed) { + if (this.closed || this.destroyed) { return; } this[kMessageStream].destroy(); @@ -345,7 +345,7 @@ export class Connection extends TypedEventEmitter { } onTimeout() { - if (this.closed) { + if (this.closed || this.destroyed) { return; } @@ -493,12 +493,19 @@ export class Connection extends TypedEventEmitter { return; } - this[kStream].end(() => { + if (this[kStream].destroyed) { this.destroyed = true; if (typeof callback === 'function') { callback(); } - }); + } else { + this[kStream].end(() => { + this.destroyed = true; + if (typeof callback === 'function') { + callback(); + } + }); + } } command( From 032de2d529bf35560830a2bfbf958b3fdc82132f Mon Sep 17 00:00:00 2001 From: Warren James Date: Tue, 6 Dec 2022 14:39:09 -0500 Subject: [PATCH 04/36] test(NODE-4834): WIP progress on unit tests --- test/unit/cmap/connection.test.ts | 59 +++++++++++++++++++++++-------- 1 file changed, 45 insertions(+), 14 deletions(-) diff --git a/test/unit/cmap/connection.test.ts b/test/unit/cmap/connection.test.ts index 5d7219fb8d..83adbf6915 100644 --- a/test/unit/cmap/connection.test.ts +++ b/test/unit/cmap/connection.test.ts @@ -26,6 +26,7 @@ const connectionOptionsDefaults = { /** The absolute minimum socket API needed by Connection as of writing this test */ class FakeSocket extends EventEmitter { + destroyed: boolean; address() { // is never called } @@ -34,6 +35,11 @@ class FakeSocket extends EventEmitter { } destroy() { // is called, has no side effects + this.destroyed = true; + } + end(cb) { + cb(); + // is called, } get remoteAddress() { return 'iLoveJavaScript'; @@ -434,7 +440,6 @@ describe('new Connection()', function() { expect(connection).to.have.property(kDelayedTimeoutId, null); }); - // TODO(NODE-4834): Ask if this test should go in the test above it('should destroy message stream and socket', () => { expect(connection).to.have.property(kDelayedTimeoutId, null); @@ -461,14 +466,10 @@ describe('new Connection()', function() { beforeEach(() => { timerSandbox = createTimerSandbox(); clock = sinon.useFakeTimers(); - - NodeJSTimeoutClass = setTimeout(() => null, 1).constructor; - driverSocket = sinon.spy(new FakeSocket()); // @ts-expect-error: driverSocket does not fully satisfy the stream type, but that's okay connection = sinon.spy(new Connection(driverSocket, connectionOptionsDefaults)); const messageStreamSymbol = getSymbolFrom(connection, 'messageStream'); - kDelayedTimeoutId = getSymbolFrom(connection, 'delayedTimeoutId'); messageStream = sinon.spy(connection[messageStreamSymbol]); }); @@ -486,7 +487,17 @@ describe('new Connection()', function() { expect(messageStream.destroy).to.have.been.calledOnce; expect(driverSocket.destroy).to.have.been.calledOnce; - }) + }); + + it.only('should not call stream.end after onClose, onTimeout, or onError', () => { + messageStream.emit('error'); + clock.tick(1); + expect(connection.onError).to.have.been.calledOnce; + expect(driverSocket.destroy).to.have.been.calledOnce; + connection.destroy(); + clock.tick(1); + expect(driverSocket.end).to.not.have.been.called; + }); }); describe('onClose()', () => { @@ -495,19 +506,14 @@ describe('new Connection()', function() { let timerSandbox: sinon.SinonFakeTimers; let driverSocket: sinon.SinonSpiedInstance; let messageStream: MessageStream; - let kDelayedTimeoutId: symbol; - let NodeJSTimeoutClass: any; beforeEach(() => { timerSandbox = createTimerSandbox(); clock = sinon.useFakeTimers(); - NodeJSTimeoutClass = setTimeout(() => null, 1).constructor; - driverSocket = sinon.spy(new FakeSocket()); // @ts-expect-error: driverSocket does not fully satisfy the stream type, but that's okay connection = sinon.spy(new Connection(driverSocket, connectionOptionsDefaults)); const messageStreamSymbol = getSymbolFrom(connection, 'messageStream'); - kDelayedTimeoutId = getSymbolFrom(connection, 'delayedTimeoutId'); messageStream = sinon.spy(connection[messageStreamSymbol]); }); @@ -522,9 +528,8 @@ describe('new Connection()', function() { clock.tick(1); expect(connection.onClose).to.have.been.calledOnce; - expect(messageStream.destroy).to.have.been.calledOnce; - }) + }); }); describe('.hasSessionSupport', function() { @@ -581,6 +586,32 @@ describe('new Connection()', function() { // TODO(NODE-4834): Implement me describe('destroy()', () => { - it('should end tcp socket and destroy messageStream', () => { }); + let connection: sinon.SinonSpiedInstance; + let clock: sinon.SinonFakeTimers; + let timerSandbox: sinon.SinonFakeTimers; + let driverSocket: sinon.SinonSpiedInstance; + let messageStream: MessageStream; + beforeEach(() => { + timerSandbox = createTimerSandbox(); + clock = sinon.useFakeTimers(); + + driverSocket = sinon.spy(new FakeSocket()); + // @ts-expect-error: driverSocket does not fully satisfy the stream type, but that's okay + connection = sinon.spy(new Connection(driverSocket, connectionOptionsDefaults)); + const messageStreamSymbol = getSymbolFrom(connection, 'messageStream'); + messageStream = sinon.spy(connection[messageStreamSymbol]); + }); + + afterEach(() => { + timerSandbox.restore(); + clock.restore(); + }); + + it.only('should end tcp socket and destroy messageStream', () => { + connection.destroy({ force: true }); + clock.tick(1); + expect(messageStream.destroy).to.have.been.calledOnce; + expect(driverSocket.destroy).to.have.been.calledOnce; + }); }); }); From f75e202b6c506f4f8aba724d35c8ca80d12775f6 Mon Sep 17 00:00:00 2001 From: Warren James Date: Tue, 6 Dec 2022 14:53:46 -0500 Subject: [PATCH 05/36] style(NODE-4834): Revert style changes --- test/unit/cmap/connection.test.ts | 68 +++++++++++++++---------------- 1 file changed, 34 insertions(+), 34 deletions(-) diff --git a/test/unit/cmap/connection.test.ts b/test/unit/cmap/connection.test.ts index 83adbf6915..7c5a8d50fb 100644 --- a/test/unit/cmap/connection.test.ts +++ b/test/unit/cmap/connection.test.ts @@ -49,12 +49,12 @@ class FakeSocket extends EventEmitter { } } -describe('new Connection()', function() { +describe('new Connection()', function () { let server; after(() => mock.cleanup()); before(() => mock.createServer().then(s => (server = s))); - it('should support fire-and-forget messages', function(done) { + it('should support fire-and-forget messages', function (done) { server.setMessageHandler(request => { const doc = request.document; if (isHello(doc)) { @@ -83,7 +83,7 @@ describe('new Connection()', function() { }); }); - it('should destroy streams which time out', function(done) { + it('should destroy streams which time out', function (done) { server.setMessageHandler(request => { const doc = request.document; if (isHello(doc)) { @@ -114,7 +114,7 @@ describe('new Connection()', function() { }); }); - it('should throw a network error with kBeforeHandshake set to false on timeout after handshake', function(done) { + it('should throw a network error with kBeforeHandshake set to false on timeout after handshake', function (done) { server.setMessageHandler(request => { const doc = request.document; if (isHello(doc)) { @@ -143,7 +143,7 @@ describe('new Connection()', function() { }); }); - it('should throw a network error with kBeforeHandshake set to true on timeout before handshake', function(done) { + it('should throw a network error with kBeforeHandshake set to true on timeout before handshake', function (done) { server.setMessageHandler(() => { // respond to no requests to trigger timeout event }); @@ -164,23 +164,23 @@ describe('new Connection()', function() { }); }); - describe('#onMessage', function() { - context('when the connection is a monitoring connection', function() { + describe('#onMessage', function () { + context('when the connection is a monitoring connection', function () { let queue: Map; let driverSocket: FakeSocket; let connection: Connection; - beforeEach(function() { + beforeEach(function () { driverSocket = sinon.spy(new FakeSocket()); }); - context('when multiple hellos exist on the stream', function() { + context('when multiple hellos exist on the stream', function () { let callbackSpy; const inputStream = new Readable(); const document = { ok: 1 }; const last = { isWritablePrimary: true }; - beforeEach(function() { + beforeEach(function () { callbackSpy = sinon.spy(); const firstHello = generateOpMsgBuffer(document); const secondHello = generateOpMsgBuffer(document); @@ -206,18 +206,18 @@ describe('new Connection()', function() { inputStream.push(null); }); - it('calls the callback with the last hello document', async function() { + it('calls the callback with the last hello document', async function () { const messages = await once(connection, 'message'); expect(messages[0].responseTo).to.equal(0); expect(callbackSpy).to.be.calledOnceWith(undefined, last); }); }); - context('when requestId/responseTo do not match', function() { + context('when requestId/responseTo do not match', function () { let callbackSpy; const document = { ok: 1 }; - beforeEach(function() { + beforeEach(function () { callbackSpy = sinon.spy(); // @ts-expect-error: driverSocket does not fully satisfy the stream type, but that's okay @@ -248,16 +248,16 @@ describe('new Connection()', function() { connection.onMessage(message); }); - it('calls the operation description callback with the document', function() { + it('calls the operation description callback with the document', function () { expect(callbackSpy).to.be.calledOnceWith(undefined, document); }); }); - context('when requestId/reponseTo match', function() { + context('when requestId/reponseTo match', function () { let callbackSpy; const document = { ok: 1 }; - beforeEach(function() { + beforeEach(function () { callbackSpy = sinon.spy(); // @ts-expect-error: driverSocket does not fully satisfy the stream type, but that's okay @@ -288,15 +288,15 @@ describe('new Connection()', function() { connection.onMessage(message); }); - it('calls the operation description callback with the document', function() { + it('calls the operation description callback with the document', function () { expect(callbackSpy).to.be.calledOnceWith(undefined, document); }); }); - context('when no operation description is in the queue', function() { + context('when no operation description is in the queue', function () { const document = { ok: 1 }; - beforeEach(function() { + beforeEach(function () { // @ts-expect-error: driverSocket does not fully satisfy the stream type, but that's okay connection = sinon.spy(new Connection(driverSocket, connectionOptionsDefaults)); connection.isMonitoringConnection = true; @@ -304,7 +304,7 @@ describe('new Connection()', function() { queue = connection[queueSymbol]; }); - it('does not error', function() { + it('does not error', function () { const msg = generateOpMsgBuffer(document); const msgHeader: MessageHeader = { length: msg.readInt32LE(0), @@ -321,12 +321,12 @@ describe('new Connection()', function() { }); }); - context('when more than one operation description is in the queue', function() { + context('when more than one operation description is in the queue', function () { let spyOne; let spyTwo; const document = { ok: 1 }; - beforeEach(function() { + beforeEach(function () { spyOne = sinon.spy(); spyTwo = sinon.spy(); @@ -363,7 +363,7 @@ describe('new Connection()', function() { connection.onMessage(message); }); - it('calls all operation description callbacks with an error', function() { + it('calls all operation description callbacks with an error', function () { expect(spyOne).to.be.calledOnce; expect(spyTwo).to.be.calledOnce; const errorOne = spyOne.firstCall.args[0]; @@ -532,12 +532,12 @@ describe('new Connection()', function() { }); }); - describe('.hasSessionSupport', function() { + describe('.hasSessionSupport', function () { let connection; const stream = new Socket(); - context('when logicalSessionTimeoutMinutes is present', function() { - beforeEach(function() { + context('when logicalSessionTimeoutMinutes is present', function () { + beforeEach(function () { const options = { ...connectionOptionsDefaults, hostAddress: server.hostAddress(), @@ -546,14 +546,14 @@ describe('new Connection()', function() { connection = new Connection(stream, options); }); - it('returns true', function() { + it('returns true', function () { expect(hasSessionSupport(connection)).to.be.true; }); }); - context('when logicalSessionTimeoutMinutes is not present', function() { - context('when in load balancing mode', function() { - beforeEach(function() { + context('when logicalSessionTimeoutMinutes is not present', function () { + context('when in load balancing mode', function () { + beforeEach(function () { const options = { ...connectionOptionsDefaults, hostAddress: server.hostAddress(), @@ -562,13 +562,13 @@ describe('new Connection()', function() { connection = new Connection(stream, options); }); - it('returns true', function() { + it('returns true', function () { expect(hasSessionSupport(connection)).to.be.true; }); }); - context('when not in load balancing mode', function() { - beforeEach(function() { + context('when not in load balancing mode', function () { + beforeEach(function () { const options = { ...connectionOptionsDefaults, hostAddress: server.hostAddress(), @@ -577,7 +577,7 @@ describe('new Connection()', function() { connection = new Connection(stream, options); }); - it('returns false', function() { + it('returns false', function () { expect(hasSessionSupport(connection)).to.be.false; }); }); From 586c410f42079f09501882a9ca09f5d1fc55b701 Mon Sep 17 00:00:00 2001 From: Warren James Date: Tue, 6 Dec 2022 14:57:29 -0500 Subject: [PATCH 06/36] test(NODE-4834): Remove unit test 'only' annotations --- test/unit/cmap/connection.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/cmap/connection.test.ts b/test/unit/cmap/connection.test.ts index 7c5a8d50fb..77b02aa935 100644 --- a/test/unit/cmap/connection.test.ts +++ b/test/unit/cmap/connection.test.ts @@ -489,7 +489,7 @@ describe('new Connection()', function () { expect(driverSocket.destroy).to.have.been.calledOnce; }); - it.only('should not call stream.end after onClose, onTimeout, or onError', () => { + it('should not call stream.end after onClose, onTimeout, or onError', () => { messageStream.emit('error'); clock.tick(1); expect(connection.onError).to.have.been.calledOnce; @@ -607,7 +607,7 @@ describe('new Connection()', function () { clock.restore(); }); - it.only('should end tcp socket and destroy messageStream', () => { + it('should end tcp socket and destroy messageStream', () => { connection.destroy({ force: true }); clock.tick(1); expect(messageStream.destroy).to.have.been.calledOnce; From 7a8d807b3c31e62b1c628ebc671a391b3c6cfc42 Mon Sep 17 00:00:00 2001 From: Warren James Date: Tue, 6 Dec 2022 15:14:01 -0500 Subject: [PATCH 07/36] test(NODE-4834): move test to more appropriate position --- test/unit/cmap/connection.test.ts | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/test/unit/cmap/connection.test.ts b/test/unit/cmap/connection.test.ts index 77b02aa935..6366763429 100644 --- a/test/unit/cmap/connection.test.ts +++ b/test/unit/cmap/connection.test.ts @@ -39,7 +39,6 @@ class FakeSocket extends EventEmitter { } end(cb) { cb(); - // is called, } get remoteAddress() { return 'iLoveJavaScript'; @@ -489,15 +488,6 @@ describe('new Connection()', function () { expect(driverSocket.destroy).to.have.been.calledOnce; }); - it('should not call stream.end after onClose, onTimeout, or onError', () => { - messageStream.emit('error'); - clock.tick(1); - expect(connection.onError).to.have.been.calledOnce; - expect(driverSocket.destroy).to.have.been.calledOnce; - connection.destroy(); - clock.tick(1); - expect(driverSocket.end).to.not.have.been.called; - }); }); describe('onClose()', () => { @@ -608,10 +598,20 @@ describe('new Connection()', function () { }); it('should end tcp socket and destroy messageStream', () => { - connection.destroy({ force: true }); + connection.destroy(); clock.tick(1); expect(messageStream.destroy).to.have.been.calledOnce; + expect(driverSocket.end).to.have.been.calledOnce; + }); + + it('should not call stream.end after onClose, onTimeout, or onError', () => { + messageStream.emit('error'); + clock.tick(1); + expect(connection.onError).to.have.been.calledOnce; expect(driverSocket.destroy).to.have.been.calledOnce; + connection.destroy(); + clock.tick(1); + expect(driverSocket.end).to.not.have.been.called; }); }); }); From 0c804a6b20bbaae6befc6c7f81ddb33967c7689a Mon Sep 17 00:00:00 2001 From: Warren James Date: Tue, 6 Dec 2022 15:21:35 -0500 Subject: [PATCH 08/36] style(NODE-4834): Remove todo comment --- test/unit/cmap/connection.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/test/unit/cmap/connection.test.ts b/test/unit/cmap/connection.test.ts index 6366763429..b5f56be018 100644 --- a/test/unit/cmap/connection.test.ts +++ b/test/unit/cmap/connection.test.ts @@ -574,7 +574,6 @@ describe('new Connection()', function () { }); }); - // TODO(NODE-4834): Implement me describe('destroy()', () => { let connection: sinon.SinonSpiedInstance; let clock: sinon.SinonFakeTimers; From 4e5a9bbe1a3516ef6c286f9120f829dc1d03c75a Mon Sep 17 00:00:00 2001 From: Warren James Date: Tue, 6 Dec 2022 15:35:54 -0500 Subject: [PATCH 09/36] docs(NODE-4834): Add tsdoc comments for clarification on closed and destroyed flags and on{Error,Timeout,Close} event handlers --- src/cmap/connection.ts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index f7be6260b2..3ee023ec17 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -170,7 +170,9 @@ export class Connection extends TypedEventEmitter { address: string; socketTimeoutMS: number; monitorCommands: boolean; + /** True when after of onClose, onError, or onTimeout was called */ closed: boolean; + /** True only after destroy has been called */ destroyed: boolean; lastHelloMS?: number; serverApi?: ServerApi; @@ -309,6 +311,12 @@ export class Connection extends TypedEventEmitter { this[kLastUseTime] = now(); } + /** + * @remarks + * onError is called when an error is propagated up from the socket behind + * kStream to kMessageStream. This occurs prior to the closing of the socket, + * so the resource must be return to the operating system here. + */ onError(error: Error) { if (this.closed || this.destroyed) { return; @@ -327,6 +335,11 @@ export class Connection extends TypedEventEmitter { this.emit(Connection.CLOSE); } + /** + * @remarks + * onClose is called when the socket underlying kStream has been closed and + * the resource is returned to the operating system + */ onClose() { if (this.closed || this.destroyed) { return; @@ -344,6 +357,11 @@ export class Connection extends TypedEventEmitter { this.emit(Connection.CLOSE); } + /** + * @remarks + * onTimeout is called when the tcp socket underlying kStream times out. This + * occurs prior to the closing of the socket, so the resouce must be returned + * to the operating system here */ onTimeout() { if (this.closed || this.destroyed) { return; From 5ccb807e8a89c1cf13a7b7fe9abd1357b2b3ca81 Mon Sep 17 00:00:00 2001 From: Warren James Date: Tue, 6 Dec 2022 15:46:01 -0500 Subject: [PATCH 10/36] style(NODE-4834): Eslint fixes --- src/cmap/connection.ts | 2 +- test/unit/cmap/connection.test.ts | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index 3ee023ec17..ea7a110b0c 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -488,7 +488,7 @@ export class Connection extends TypedEventEmitter { this.removeAllListeners(Connection.PINNED); this.removeAllListeners(Connection.UNPINNED); - + this[kMessageStream].destroy(); options = Object.assign({ force: false }, options); diff --git a/test/unit/cmap/connection.test.ts b/test/unit/cmap/connection.test.ts index b5f56be018..3b7b4c78f2 100644 --- a/test/unit/cmap/connection.test.ts +++ b/test/unit/cmap/connection.test.ts @@ -460,8 +460,6 @@ describe('new Connection()', function () { let timerSandbox: sinon.SinonFakeTimers; let driverSocket: sinon.SinonSpiedInstance; let messageStream: MessageStream; - let kDelayedTimeoutId: symbol; - let NodeJSTimeoutClass: any; beforeEach(() => { timerSandbox = createTimerSandbox(); clock = sinon.useFakeTimers(); @@ -487,7 +485,6 @@ describe('new Connection()', function () { expect(messageStream.destroy).to.have.been.calledOnce; expect(driverSocket.destroy).to.have.been.calledOnce; }); - }); describe('onClose()', () => { From 770d050c124c59972566e36f0f1369fc42d834d1 Mon Sep 17 00:00:00 2001 From: Warren James <28974128+W-A-James@users.noreply.github.com> Date: Wed, 7 Dec 2022 09:27:39 -0500 Subject: [PATCH 11/36] test(NODE-4834): Change test descriptions to match testing standards and terminology used in spec Co-authored-by: Durran Jordan --- test/unit/cmap/connection.test.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/unit/cmap/connection.test.ts b/test/unit/cmap/connection.test.ts index 3b7b4c78f2..63608690e2 100644 --- a/test/unit/cmap/connection.test.ts +++ b/test/unit/cmap/connection.test.ts @@ -439,7 +439,7 @@ describe('new Connection()', function () { expect(connection).to.have.property(kDelayedTimeoutId, null); }); - it('should destroy message stream and socket', () => { + it('destroys the message stream and socket', () => { expect(connection).to.have.property(kDelayedTimeoutId, null); driverSocket.emit('timeout'); @@ -475,7 +475,7 @@ describe('new Connection()', function () { clock.restore(); }); - it('should destroy message stream and socket', () => { + it('destroys the message stream and socket', () => { messageStream.emit('error'); clock.tick(1); @@ -509,7 +509,7 @@ describe('new Connection()', function () { clock.restore(); }); - it('should destroy message stream', () => { + it('destroys the message stream', () => { driverSocket.emit('close'); clock.tick(1); @@ -593,14 +593,14 @@ describe('new Connection()', function () { clock.restore(); }); - it('should end tcp socket and destroy messageStream', () => { + it('ends the tcp socket and destroys the messageStream', () => { connection.destroy(); clock.tick(1); expect(messageStream.destroy).to.have.been.calledOnce; expect(driverSocket.end).to.have.been.calledOnce; }); - it('should not call stream.end after onClose, onTimeout, or onError', () => { + it('does not call stream.end after onClose, onTimeout, or onError', () => { messageStream.emit('error'); clock.tick(1); expect(connection.onError).to.have.been.calledOnce; From 92d577da6a1b63df0563fd5a29cc770c3673ed92 Mon Sep 17 00:00:00 2001 From: Warren James Date: Wed, 7 Dec 2022 11:06:08 -0500 Subject: [PATCH 12/36] docs(NODE-4834): Update inline documentation to reference state rather than explicit values --- src/cmap/connection.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index ea7a110b0c..8473243326 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -170,9 +170,9 @@ export class Connection extends TypedEventEmitter { address: string; socketTimeoutMS: number; monitorCommands: boolean; - /** True when after of onClose, onError, or onTimeout was called */ + /** Indicates that the connection (including underlying TCP socket) has been closed. */ closed: boolean; - /** True only after destroy has been called */ + /** Indicates that the connection has been explicitly destroyed. In the destroyed state, the connection is also closed */ destroyed: boolean; lastHelloMS?: number; serverApi?: ServerApi; From 1ada8ca717c9367bfe525c49936feb82a23a759b Mon Sep 17 00:00:00 2001 From: Warren James Date: Thu, 8 Dec 2022 10:28:43 -0500 Subject: [PATCH 13/36] fix(NODE-4834): Address review comments --- src/cmap/connection.ts | 31 +++++++++---------------------- 1 file changed, 9 insertions(+), 22 deletions(-) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index 8473243326..469dfd93a1 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -122,8 +122,8 @@ export interface ProxyOptions { /** @public */ export interface ConnectionOptions extends SupportedNodeConnectionOptions, - StreamDescriptionOptions, - ProxyOptions { + StreamDescriptionOptions, + ProxyOptions { // Internal creation info id: number | ''; generation: number; @@ -360,7 +360,7 @@ export class Connection extends TypedEventEmitter { /** * @remarks * onTimeout is called when the tcp socket underlying kStream times out. This - * occurs prior to the closing of the socket, so the resouce must be returned + * occurs prior to the closing of the socket, so the resource must be returned * to the operating system here */ onTimeout() { if (this.closed || this.destroyed) { @@ -492,36 +492,23 @@ export class Connection extends TypedEventEmitter { this[kMessageStream].destroy(); options = Object.assign({ force: false }, options); - if (this[kStream] == null || this.destroyed) { - this.destroyed = true; - if (typeof callback === 'function') { - callback(); - } - - return; + if (this.destroyed) { + return callback?.(); } if (options.force) { this[kStream].destroy(); this.destroyed = true; - if (typeof callback === 'function') { - callback(); - } - - return; + return callback?.(); } - if (this[kStream].destroyed) { + if (this[kStream].writableEnded) { this.destroyed = true; - if (typeof callback === 'function') { - callback(); - } + return callback?.(); } else { this[kStream].end(() => { this.destroyed = true; - if (typeof callback === 'function') { - callback(); - } + callback?.(); }); } } From 21dc8b48d5f2667647d4f5ad819d4f1a91ffc558 Mon Sep 17 00:00:00 2001 From: Warren James Date: Thu, 8 Dec 2022 10:33:54 -0500 Subject: [PATCH 14/36] test(NODE-4834): FakeSocket now simulates IO delay --- test/unit/cmap/connection.test.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/unit/cmap/connection.test.ts b/test/unit/cmap/connection.test.ts index 63608690e2..d8ba1941c3 100644 --- a/test/unit/cmap/connection.test.ts +++ b/test/unit/cmap/connection.test.ts @@ -26,7 +26,7 @@ const connectionOptionsDefaults = { /** The absolute minimum socket API needed by Connection as of writing this test */ class FakeSocket extends EventEmitter { - destroyed: boolean; + writableEnded: boolean; address() { // is never called } @@ -35,10 +35,11 @@ class FakeSocket extends EventEmitter { } destroy() { // is called, has no side effects - this.destroyed = true; + this.writableEnded = true; } end(cb) { - cb(); + // nextTick to simulate I/O delay + process.nextTick(cb); } get remoteAddress() { return 'iLoveJavaScript'; From f5f12f5aa62fbe3e6f5ceff5835df4c967c4d1c3 Mon Sep 17 00:00:00 2001 From: Warren James Date: Thu, 8 Dec 2022 10:38:25 -0500 Subject: [PATCH 15/36] style(NODE-4834): Revert unintended style change --- src/cmap/connection.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index 469dfd93a1..48c7f9f2d0 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -122,8 +122,8 @@ export interface ProxyOptions { /** @public */ export interface ConnectionOptions extends SupportedNodeConnectionOptions, - StreamDescriptionOptions, - ProxyOptions { + StreamDescriptionOptions, + ProxyOptions { // Internal creation info id: number | ''; generation: number; From 1e5d874150488364eb75e255533577552677a4db Mon Sep 17 00:00:00 2001 From: Warren James Date: Thu, 8 Dec 2022 11:18:19 -0500 Subject: [PATCH 16/36] test(NODE-4834): Ensure that FakeSocket.end also sets this.writableEnded to true --- test/unit/cmap/connection.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/test/unit/cmap/connection.test.ts b/test/unit/cmap/connection.test.ts index d8ba1941c3..bed70177e7 100644 --- a/test/unit/cmap/connection.test.ts +++ b/test/unit/cmap/connection.test.ts @@ -38,6 +38,7 @@ class FakeSocket extends EventEmitter { this.writableEnded = true; } end(cb) { + this.writableEnded = true; // nextTick to simulate I/O delay process.nextTick(cb); } From f9402811ca04cd4a7c6db124cf26c7d51acf5fdc Mon Sep 17 00:00:00 2001 From: Warren James Date: Mon, 12 Dec 2022 11:26:39 -0500 Subject: [PATCH 17/36] fix(NODE-4834): Update Connection.destroy and event handler logic to avoid race conditions on the this.closed flag --- src/cmap/connect.ts | 2 +- src/cmap/connection.ts | 53 +++++++++++-------------------------- src/cmap/connection_pool.ts | 2 +- 3 files changed, 17 insertions(+), 40 deletions(-) diff --git a/src/cmap/connect.ts b/src/cmap/connect.ts index fedf731132..0fc10c93e0 100644 --- a/src/cmap/connect.ts +++ b/src/cmap/connect.ts @@ -96,7 +96,7 @@ function performInitialHandshake( ) { const callback: Callback = function (err, ret) { if (err && conn) { - conn.destroy(); + conn.destroy({ force: false }); } _callback(err, ret); }; diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index 48c7f9f2d0..e38a5120fb 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -122,8 +122,8 @@ export interface ProxyOptions { /** @public */ export interface ConnectionOptions extends SupportedNodeConnectionOptions, - StreamDescriptionOptions, - ProxyOptions { + StreamDescriptionOptions, + ProxyOptions { // Internal creation info id: number | ''; generation: number; @@ -149,7 +149,7 @@ export interface ConnectionOptions /** @public */ export interface DestroyOptions { /** Force the destruction. */ - force?: boolean; + force: boolean; } /** @public */ @@ -172,8 +172,6 @@ export class Connection extends TypedEventEmitter { monitorCommands: boolean; /** Indicates that the connection (including underlying TCP socket) has been closed. */ closed: boolean; - /** Indicates that the connection has been explicitly destroyed. In the destroyed state, the connection is also closed */ - destroyed: boolean; lastHelloMS?: number; serverApi?: ServerApi; helloOk?: boolean; @@ -222,7 +220,6 @@ export class Connection extends TypedEventEmitter { this.monitorCommands = options.monitorCommands; this.serverApi = options.serverApi; this.closed = false; - this.destroyed = false; this[kHello] = null; this[kClusterTime] = null; @@ -318,14 +315,10 @@ export class Connection extends TypedEventEmitter { * so the resource must be return to the operating system here. */ onError(error: Error) { - if (this.closed || this.destroyed) { + if (this.closed) { return; } - - this[kMessageStream].destroy(error); - this[kStream].destroy(error); - - this.closed = true; + this.destroy({ force: false }); for (const op of this[kQueue].values()) { op.cb(error); @@ -341,12 +334,10 @@ export class Connection extends TypedEventEmitter { * the resource is returned to the operating system */ onClose() { - if (this.closed || this.destroyed) { + if (this.closed) { return; } - this[kMessageStream].destroy(); - - this.closed = true; + this.destroy({ force: false }); const message = `connection ${this.id} to ${this.address} closed`; for (const op of this[kQueue].values()) { @@ -363,15 +354,12 @@ export class Connection extends TypedEventEmitter { * occurs prior to the closing of the socket, so the resource must be returned * to the operating system here */ onTimeout() { - if (this.closed || this.destroyed) { + if (this.closed) { return; } this[kDelayedTimeoutId] = setTimeout(() => { - this[kMessageStream].destroy(); - this[kStream].destroy(); - - this.closed = true; + this.destroy({ force: false }); const message = `connection ${this.id} to ${this.address} timed out`; const beforeHandshake = this.hello == null; @@ -480,36 +468,25 @@ export class Connection extends TypedEventEmitter { callback(undefined, message.documents[0]); } - destroy(options?: DestroyOptions, callback?: Callback): void { - if (typeof options === 'function') { - callback = options; - options = { force: false }; - } - + destroy(options: DestroyOptions, callback?: Callback): void { this.removeAllListeners(Connection.PINNED); this.removeAllListeners(Connection.UNPINNED); - this[kMessageStream].destroy(); - options = Object.assign({ force: false }, options); - if (this.destroyed) { - return callback?.(); - } + this[kMessageStream].destroy(); + this.closed = true; if (options.force) { this[kStream].destroy(); - this.destroyed = true; return callback?.(); } - if (this[kStream].writableEnded) { - this.destroyed = true; - return callback?.(); - } else { + if (!this[kStream].writableEnded) { this[kStream].end(() => { - this.destroyed = true; callback?.(); }); + } else { + callback?.(); } } diff --git a/src/cmap/connection_pool.ts b/src/cmap/connection_pool.ts index 5c8cbc9765..9827cf722f 100644 --- a/src/cmap/connection_pool.ts +++ b/src/cmap/connection_pool.ts @@ -591,7 +591,7 @@ export class ConnectionPool extends TypedEventEmitter { new ConnectionClosedEvent(this, connection, reason) ); // destroy the connection - process.nextTick(() => connection.destroy()); + process.nextTick(() => connection.destroy({ force: false })); } private connectionIsStale(connection: Connection) { From 927b8834aa1890a203d60c35c1b4171474a37d98 Mon Sep 17 00:00:00 2001 From: Warren James Date: Mon, 12 Dec 2022 11:27:26 -0500 Subject: [PATCH 18/36] test(NODE-4834): Add unit tests to check that options are treated correctly in Connection.destroy --- test/unit/cmap/connection.test.ts | 115 +++++++++++++++++++----------- 1 file changed, 72 insertions(+), 43 deletions(-) diff --git a/test/unit/cmap/connection.test.ts b/test/unit/cmap/connection.test.ts index bed70177e7..fcfb3471e7 100644 --- a/test/unit/cmap/connection.test.ts +++ b/test/unit/cmap/connection.test.ts @@ -50,12 +50,24 @@ class FakeSocket extends EventEmitter { } } -describe('new Connection()', function () { +class InputStream extends Readable { + writableEnded: boolean; + constructor(options?) { + super(options); + } + + end(cb) { + this.writableEnded = true; + process.nextTick(cb); + } +} + +describe('new Connection()', function() { let server; after(() => mock.cleanup()); before(() => mock.createServer().then(s => (server = s))); - it('should support fire-and-forget messages', function (done) { + it('should support fire-and-forget messages', function(done) { server.setMessageHandler(request => { const doc = request.document; if (isHello(doc)) { @@ -84,7 +96,7 @@ describe('new Connection()', function () { }); }); - it('should destroy streams which time out', function (done) { + it('should destroy streams which time out', function(done) { server.setMessageHandler(request => { const doc = request.document; if (isHello(doc)) { @@ -108,14 +120,14 @@ describe('new Connection()', function () { expect(err).to.be.instanceOf(MongoNetworkTimeoutError); expect(result).to.not.exist; - expect(conn).property('stream').property('destroyed', true); + expect(conn).property('stream').property('writableEnded', true); done(); }); }); }); - it('should throw a network error with kBeforeHandshake set to false on timeout after handshake', function (done) { + it('should throw a network error with kBeforeHandshake set to false on timeout after handshake', function(done) { server.setMessageHandler(request => { const doc = request.document; if (isHello(doc)) { @@ -144,7 +156,7 @@ describe('new Connection()', function () { }); }); - it('should throw a network error with kBeforeHandshake set to true on timeout before handshake', function (done) { + it('should throw a network error with kBeforeHandshake set to true on timeout before handshake', function(done) { server.setMessageHandler(() => { // respond to no requests to trigger timeout event }); @@ -165,23 +177,23 @@ describe('new Connection()', function () { }); }); - describe('#onMessage', function () { - context('when the connection is a monitoring connection', function () { + describe('#onMessage', function() { + context('when the connection is a monitoring connection', function() { let queue: Map; let driverSocket: FakeSocket; let connection: Connection; - beforeEach(function () { + beforeEach(function() { driverSocket = sinon.spy(new FakeSocket()); }); - context('when multiple hellos exist on the stream', function () { + context('when multiple hellos exist on the stream', function() { let callbackSpy; - const inputStream = new Readable(); + const inputStream = new InputStream(); const document = { ok: 1 }; const last = { isWritablePrimary: true }; - beforeEach(function () { + beforeEach(function() { callbackSpy = sinon.spy(); const firstHello = generateOpMsgBuffer(document); const secondHello = generateOpMsgBuffer(document); @@ -207,18 +219,18 @@ describe('new Connection()', function () { inputStream.push(null); }); - it('calls the callback with the last hello document', async function () { + it('calls the callback with the last hello document', async function() { const messages = await once(connection, 'message'); expect(messages[0].responseTo).to.equal(0); expect(callbackSpy).to.be.calledOnceWith(undefined, last); }); }); - context('when requestId/responseTo do not match', function () { + context('when requestId/responseTo do not match', function() { let callbackSpy; const document = { ok: 1 }; - beforeEach(function () { + beforeEach(function() { callbackSpy = sinon.spy(); // @ts-expect-error: driverSocket does not fully satisfy the stream type, but that's okay @@ -249,16 +261,16 @@ describe('new Connection()', function () { connection.onMessage(message); }); - it('calls the operation description callback with the document', function () { + it('calls the operation description callback with the document', function() { expect(callbackSpy).to.be.calledOnceWith(undefined, document); }); }); - context('when requestId/reponseTo match', function () { + context('when requestId/reponseTo match', function() { let callbackSpy; const document = { ok: 1 }; - beforeEach(function () { + beforeEach(function() { callbackSpy = sinon.spy(); // @ts-expect-error: driverSocket does not fully satisfy the stream type, but that's okay @@ -289,15 +301,15 @@ describe('new Connection()', function () { connection.onMessage(message); }); - it('calls the operation description callback with the document', function () { + it('calls the operation description callback with the document', function() { expect(callbackSpy).to.be.calledOnceWith(undefined, document); }); }); - context('when no operation description is in the queue', function () { + context('when no operation description is in the queue', function() { const document = { ok: 1 }; - beforeEach(function () { + beforeEach(function() { // @ts-expect-error: driverSocket does not fully satisfy the stream type, but that's okay connection = sinon.spy(new Connection(driverSocket, connectionOptionsDefaults)); connection.isMonitoringConnection = true; @@ -305,7 +317,7 @@ describe('new Connection()', function () { queue = connection[queueSymbol]; }); - it('does not error', function () { + it('does not error', function() { const msg = generateOpMsgBuffer(document); const msgHeader: MessageHeader = { length: msg.readInt32LE(0), @@ -322,12 +334,12 @@ describe('new Connection()', function () { }); }); - context('when more than one operation description is in the queue', function () { + context('when more than one operation description is in the queue', function() { let spyOne; let spyTwo; const document = { ok: 1 }; - beforeEach(function () { + beforeEach(function() { spyOne = sinon.spy(); spyTwo = sinon.spy(); @@ -364,7 +376,7 @@ describe('new Connection()', function () { connection.onMessage(message); }); - it('calls all operation description callbacks with an error', function () { + it('calls all operation description callbacks with an error', function() { expect(spyOne).to.be.calledOnce; expect(spyTwo).to.be.calledOnce; const errorOne = spyOne.firstCall.args[0]; @@ -409,13 +421,15 @@ describe('new Connection()', function () { driverSocket.emit('timeout'); expect(connection.onTimeout).to.have.been.calledOnce; + expect(connection.destroy).to.not.have.been.called; expect(connection).to.have.property(kDelayedTimeoutId).that.is.instanceOf(NodeJSTimeoutClass); expect(connection).to.have.property('closed', false); - expect(driverSocket.destroy).to.not.have.been.called; + expect(driverSocket.end).to.not.have.been.called; clock.tick(1); - expect(driverSocket.destroy).to.have.been.calledOnce; + expect(driverSocket.end).to.have.been.calledOnce; + expect(connection.destroy).to.have.been.calledOnce; expect(connection).to.have.property('closed', true); }); @@ -452,7 +466,7 @@ describe('new Connection()', function () { expect(connection).to.have.property(kDelayedTimeoutId).that.is.instanceOf(NodeJSTimeoutClass); expect(messageStream.destroy).to.have.been.calledOnce; - expect(driverSocket.destroy).to.have.been.calledOnce; + expect(driverSocket.end).to.have.been.calledOnce; }); }); @@ -485,7 +499,7 @@ describe('new Connection()', function () { expect(connection.onError).to.have.been.calledOnce; expect(messageStream.destroy).to.have.been.calledOnce; - expect(driverSocket.destroy).to.have.been.calledOnce; + expect(driverSocket.end).to.have.been.calledOnce; }); }); @@ -521,12 +535,12 @@ describe('new Connection()', function () { }); }); - describe('.hasSessionSupport', function () { + describe('.hasSessionSupport', function() { let connection; const stream = new Socket(); - context('when logicalSessionTimeoutMinutes is present', function () { - beforeEach(function () { + context('when logicalSessionTimeoutMinutes is present', function() { + beforeEach(function() { const options = { ...connectionOptionsDefaults, hostAddress: server.hostAddress(), @@ -535,14 +549,14 @@ describe('new Connection()', function () { connection = new Connection(stream, options); }); - it('returns true', function () { + it('returns true', function() { expect(hasSessionSupport(connection)).to.be.true; }); }); - context('when logicalSessionTimeoutMinutes is not present', function () { - context('when in load balancing mode', function () { - beforeEach(function () { + context('when logicalSessionTimeoutMinutes is not present', function() { + context('when in load balancing mode', function() { + beforeEach(function() { const options = { ...connectionOptionsDefaults, hostAddress: server.hostAddress(), @@ -551,13 +565,13 @@ describe('new Connection()', function () { connection = new Connection(stream, options); }); - it('returns true', function () { + it('returns true', function() { expect(hasSessionSupport(connection)).to.be.true; }); }); - context('when not in load balancing mode', function () { - beforeEach(function () { + context('when not in load balancing mode', function() { + beforeEach(function() { const options = { ...connectionOptionsDefaults, hostAddress: server.hostAddress(), @@ -566,7 +580,7 @@ describe('new Connection()', function () { connection = new Connection(stream, options); }); - it('returns false', function () { + it('returns false', function() { expect(hasSessionSupport(connection)).to.be.false; }); }); @@ -596,7 +610,7 @@ describe('new Connection()', function () { }); it('ends the tcp socket and destroys the messageStream', () => { - connection.destroy(); + connection.destroy({force: false}); clock.tick(1); expect(messageStream.destroy).to.have.been.calledOnce; expect(driverSocket.end).to.have.been.calledOnce; @@ -606,10 +620,25 @@ describe('new Connection()', function () { messageStream.emit('error'); clock.tick(1); expect(connection.onError).to.have.been.calledOnce; - expect(driverSocket.destroy).to.have.been.calledOnce; - connection.destroy(); clock.tick(1); + expect(driverSocket.destroy).to.not.have.been.calledOnce; + connection.destroy({force:false}); + clock.tick(1); + expect(driverSocket.end).to.have.been.called; + }); + + it('does not call stream.end if options.force == true', () => { + connection.destroy({force:true}); + clock.tick(1); + expect(driverSocket.destroy).to.have.been.calledOnce; expect(driverSocket.end).to.not.have.been.called; }); + + it('does not call stream.destroy if options.force == false', () => { + connection.destroy({force: false}); + clock.tick(1); + expect(driverSocket.destroy).to.not.have.been.called; + expect(driverSocket.end).to.have.been.calledOnce; + }); }); }); From bf1c8f057ef2667d3665ef9089bd35e36c5d3333 Mon Sep 17 00:00:00 2001 From: Warren James Date: Mon, 12 Dec 2022 11:46:04 -0500 Subject: [PATCH 19/36] fix(NODE-4834): Address eslint fixes --- src/cmap/connection.ts | 5 ++--- src/cmap/connection_pool.ts | 2 +- src/sdam/server.ts | 5 ++++- src/sdam/topology.ts | 6 +++--- 4 files changed, 10 insertions(+), 8 deletions(-) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index e38a5120fb..f21df7e55f 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -122,8 +122,8 @@ export interface ProxyOptions { /** @public */ export interface ConnectionOptions extends SupportedNodeConnectionOptions, - StreamDescriptionOptions, - ProxyOptions { + StreamDescriptionOptions, + ProxyOptions { // Internal creation info id: number | ''; generation: number; @@ -472,7 +472,6 @@ export class Connection extends TypedEventEmitter { this.removeAllListeners(Connection.PINNED); this.removeAllListeners(Connection.UNPINNED); - this[kMessageStream].destroy(); this.closed = true; diff --git a/src/cmap/connection_pool.ts b/src/cmap/connection_pool.ts index 9827cf722f..d6e19a7e3d 100644 --- a/src/cmap/connection_pool.ts +++ b/src/cmap/connection_pool.ts @@ -515,7 +515,7 @@ export class ConnectionPool extends TypedEventEmitter { ConnectionPool.CONNECTION_CLOSED, new ConnectionClosedEvent(this, conn, 'poolClosed') ); - conn.destroy(options, cb); + conn.destroy({ force: !!options.force }, cb); }, err => { this[kConnections].clear(); diff --git a/src/sdam/server.ts b/src/sdam/server.ts index ae7a1fd5f6..9cd204ebf5 100644 --- a/src/sdam/server.ts +++ b/src/sdam/server.ts @@ -244,7 +244,10 @@ export class Server extends TypedEventEmitter { /** Destroy the server connection */ destroy(options?: DestroyOptions, callback?: Callback): void { - if (typeof options === 'function') (callback = options), (options = {}); + if (typeof options === 'function') { + callback = options; + options = { force: false }; + } options = Object.assign({}, { force: false }, options); if (this.s.state === STATE_CLOSED) { diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index 601ae2c382..19b5af5484 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -496,14 +496,14 @@ export class Topology extends TypedEventEmitter { if (typeof options === 'boolean') { options = { force: options }; } - options = options ?? {}; + options = options ?? { force: false }; if (this.s.state === STATE_CLOSED || this.s.state === STATE_CLOSING) { return callback?.(); } const destroyedServers = Array.from(this.s.servers.values(), server => { - return promisify(destroyServer)(server, this, options as CloseOptions); + return promisify(destroyServer)(server, this, Object.assign({ force: false }, options)); }); Promise.all(destroyedServers) @@ -765,7 +765,7 @@ function destroyServer( options?: DestroyOptions, callback?: Callback ) { - options = options ?? {}; + options = options ?? { force: false }; for (const event of LOCAL_SERVER_EVENTS) { server.removeAllListeners(event); } From 452c1f0f4edd6cf83ac254cd0e8e959a1cd39e8a Mon Sep 17 00:00:00 2001 From: Warren James Date: Mon, 12 Dec 2022 11:47:18 -0500 Subject: [PATCH 20/36] docs(NODE-4834): Remove irrelevant docs --- src/cmap/connection.ts | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index f21df7e55f..3d83d076e0 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -308,12 +308,6 @@ export class Connection extends TypedEventEmitter { this[kLastUseTime] = now(); } - /** - * @remarks - * onError is called when an error is propagated up from the socket behind - * kStream to kMessageStream. This occurs prior to the closing of the socket, - * so the resource must be return to the operating system here. - */ onError(error: Error) { if (this.closed) { return; @@ -328,11 +322,6 @@ export class Connection extends TypedEventEmitter { this.emit(Connection.CLOSE); } - /** - * @remarks - * onClose is called when the socket underlying kStream has been closed and - * the resource is returned to the operating system - */ onClose() { if (this.closed) { return; @@ -348,11 +337,6 @@ export class Connection extends TypedEventEmitter { this.emit(Connection.CLOSE); } - /** - * @remarks - * onTimeout is called when the tcp socket underlying kStream times out. This - * occurs prior to the closing of the socket, so the resource must be returned - * to the operating system here */ onTimeout() { if (this.closed) { return; From dcda86cb1c7f63a9dca18196a747e96535fd88bb Mon Sep 17 00:00:00 2001 From: Warren James Date: Mon, 12 Dec 2022 11:47:40 -0500 Subject: [PATCH 21/36] style(NODE-4834): eslint fixes --- test/unit/cmap/connection.test.ts | 76 +++++++++++++++---------------- 1 file changed, 38 insertions(+), 38 deletions(-) diff --git a/test/unit/cmap/connection.test.ts b/test/unit/cmap/connection.test.ts index fcfb3471e7..5f2ee16496 100644 --- a/test/unit/cmap/connection.test.ts +++ b/test/unit/cmap/connection.test.ts @@ -62,12 +62,12 @@ class InputStream extends Readable { } } -describe('new Connection()', function() { +describe('new Connection()', function () { let server; after(() => mock.cleanup()); before(() => mock.createServer().then(s => (server = s))); - it('should support fire-and-forget messages', function(done) { + it('should support fire-and-forget messages', function (done) { server.setMessageHandler(request => { const doc = request.document; if (isHello(doc)) { @@ -96,7 +96,7 @@ describe('new Connection()', function() { }); }); - it('should destroy streams which time out', function(done) { + it('should destroy streams which time out', function (done) { server.setMessageHandler(request => { const doc = request.document; if (isHello(doc)) { @@ -127,7 +127,7 @@ describe('new Connection()', function() { }); }); - it('should throw a network error with kBeforeHandshake set to false on timeout after handshake', function(done) { + it('should throw a network error with kBeforeHandshake set to false on timeout after handshake', function (done) { server.setMessageHandler(request => { const doc = request.document; if (isHello(doc)) { @@ -156,7 +156,7 @@ describe('new Connection()', function() { }); }); - it('should throw a network error with kBeforeHandshake set to true on timeout before handshake', function(done) { + it('should throw a network error with kBeforeHandshake set to true on timeout before handshake', function (done) { server.setMessageHandler(() => { // respond to no requests to trigger timeout event }); @@ -177,23 +177,23 @@ describe('new Connection()', function() { }); }); - describe('#onMessage', function() { - context('when the connection is a monitoring connection', function() { + describe('#onMessage', function () { + context('when the connection is a monitoring connection', function () { let queue: Map; let driverSocket: FakeSocket; let connection: Connection; - beforeEach(function() { + beforeEach(function () { driverSocket = sinon.spy(new FakeSocket()); }); - context('when multiple hellos exist on the stream', function() { + context('when multiple hellos exist on the stream', function () { let callbackSpy; const inputStream = new InputStream(); const document = { ok: 1 }; const last = { isWritablePrimary: true }; - beforeEach(function() { + beforeEach(function () { callbackSpy = sinon.spy(); const firstHello = generateOpMsgBuffer(document); const secondHello = generateOpMsgBuffer(document); @@ -219,18 +219,18 @@ describe('new Connection()', function() { inputStream.push(null); }); - it('calls the callback with the last hello document', async function() { + it('calls the callback with the last hello document', async function () { const messages = await once(connection, 'message'); expect(messages[0].responseTo).to.equal(0); expect(callbackSpy).to.be.calledOnceWith(undefined, last); }); }); - context('when requestId/responseTo do not match', function() { + context('when requestId/responseTo do not match', function () { let callbackSpy; const document = { ok: 1 }; - beforeEach(function() { + beforeEach(function () { callbackSpy = sinon.spy(); // @ts-expect-error: driverSocket does not fully satisfy the stream type, but that's okay @@ -261,16 +261,16 @@ describe('new Connection()', function() { connection.onMessage(message); }); - it('calls the operation description callback with the document', function() { + it('calls the operation description callback with the document', function () { expect(callbackSpy).to.be.calledOnceWith(undefined, document); }); }); - context('when requestId/reponseTo match', function() { + context('when requestId/reponseTo match', function () { let callbackSpy; const document = { ok: 1 }; - beforeEach(function() { + beforeEach(function () { callbackSpy = sinon.spy(); // @ts-expect-error: driverSocket does not fully satisfy the stream type, but that's okay @@ -301,15 +301,15 @@ describe('new Connection()', function() { connection.onMessage(message); }); - it('calls the operation description callback with the document', function() { + it('calls the operation description callback with the document', function () { expect(callbackSpy).to.be.calledOnceWith(undefined, document); }); }); - context('when no operation description is in the queue', function() { + context('when no operation description is in the queue', function () { const document = { ok: 1 }; - beforeEach(function() { + beforeEach(function () { // @ts-expect-error: driverSocket does not fully satisfy the stream type, but that's okay connection = sinon.spy(new Connection(driverSocket, connectionOptionsDefaults)); connection.isMonitoringConnection = true; @@ -317,7 +317,7 @@ describe('new Connection()', function() { queue = connection[queueSymbol]; }); - it('does not error', function() { + it('does not error', function () { const msg = generateOpMsgBuffer(document); const msgHeader: MessageHeader = { length: msg.readInt32LE(0), @@ -334,12 +334,12 @@ describe('new Connection()', function() { }); }); - context('when more than one operation description is in the queue', function() { + context('when more than one operation description is in the queue', function () { let spyOne; let spyTwo; const document = { ok: 1 }; - beforeEach(function() { + beforeEach(function () { spyOne = sinon.spy(); spyTwo = sinon.spy(); @@ -376,7 +376,7 @@ describe('new Connection()', function() { connection.onMessage(message); }); - it('calls all operation description callbacks with an error', function() { + it('calls all operation description callbacks with an error', function () { expect(spyOne).to.be.calledOnce; expect(spyTwo).to.be.calledOnce; const errorOne = spyOne.firstCall.args[0]; @@ -535,12 +535,12 @@ describe('new Connection()', function() { }); }); - describe('.hasSessionSupport', function() { + describe('.hasSessionSupport', function () { let connection; const stream = new Socket(); - context('when logicalSessionTimeoutMinutes is present', function() { - beforeEach(function() { + context('when logicalSessionTimeoutMinutes is present', function () { + beforeEach(function () { const options = { ...connectionOptionsDefaults, hostAddress: server.hostAddress(), @@ -549,14 +549,14 @@ describe('new Connection()', function() { connection = new Connection(stream, options); }); - it('returns true', function() { + it('returns true', function () { expect(hasSessionSupport(connection)).to.be.true; }); }); - context('when logicalSessionTimeoutMinutes is not present', function() { - context('when in load balancing mode', function() { - beforeEach(function() { + context('when logicalSessionTimeoutMinutes is not present', function () { + context('when in load balancing mode', function () { + beforeEach(function () { const options = { ...connectionOptionsDefaults, hostAddress: server.hostAddress(), @@ -565,13 +565,13 @@ describe('new Connection()', function() { connection = new Connection(stream, options); }); - it('returns true', function() { + it('returns true', function () { expect(hasSessionSupport(connection)).to.be.true; }); }); - context('when not in load balancing mode', function() { - beforeEach(function() { + context('when not in load balancing mode', function () { + beforeEach(function () { const options = { ...connectionOptionsDefaults, hostAddress: server.hostAddress(), @@ -580,7 +580,7 @@ describe('new Connection()', function() { connection = new Connection(stream, options); }); - it('returns false', function() { + it('returns false', function () { expect(hasSessionSupport(connection)).to.be.false; }); }); @@ -610,7 +610,7 @@ describe('new Connection()', function() { }); it('ends the tcp socket and destroys the messageStream', () => { - connection.destroy({force: false}); + connection.destroy({ force: false }); clock.tick(1); expect(messageStream.destroy).to.have.been.calledOnce; expect(driverSocket.end).to.have.been.calledOnce; @@ -622,20 +622,20 @@ describe('new Connection()', function() { expect(connection.onError).to.have.been.calledOnce; clock.tick(1); expect(driverSocket.destroy).to.not.have.been.calledOnce; - connection.destroy({force:false}); + connection.destroy({ force: false }); clock.tick(1); expect(driverSocket.end).to.have.been.called; }); it('does not call stream.end if options.force == true', () => { - connection.destroy({force:true}); + connection.destroy({ force: true }); clock.tick(1); expect(driverSocket.destroy).to.have.been.calledOnce; expect(driverSocket.end).to.not.have.been.called; }); it('does not call stream.destroy if options.force == false', () => { - connection.destroy({force: false}); + connection.destroy({ force: false }); clock.tick(1); expect(driverSocket.destroy).to.not.have.been.called; expect(driverSocket.end).to.have.been.calledOnce; From c74bff8e239c6289166ac207b5e90e48940df5c6 Mon Sep 17 00:00:00 2001 From: Warren James Date: Mon, 12 Dec 2022 14:08:45 -0500 Subject: [PATCH 22/36] refactor(NODE-4834): Make all paths through Connection.destroy async --- src/cmap/connection.ts | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index 3d83d076e0..4bffacee74 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -458,18 +458,21 @@ export class Connection extends TypedEventEmitter { this[kMessageStream].destroy(); this.closed = true; + callback = callback + ? callback + : () => { + /* Ignore */ + }; if (options.force) { this[kStream].destroy(); - return callback?.(); + return process.nextTick(callback); } if (!this[kStream].writableEnded) { - this[kStream].end(() => { - callback?.(); - }); + this[kStream].end(callback); } else { - callback?.(); + process.nextTick(callback); } } From 7993c4ecd7d7c51a68b272d115f4e4a778503f1f Mon Sep 17 00:00:00 2001 From: Warren James Date: Mon, 12 Dec 2022 15:35:54 -0500 Subject: [PATCH 23/36] fix(NODE-4834): Address review comments --- src/cmap/connection.ts | 13 ++++++------- src/sdam/topology.ts | 13 ++----------- 2 files changed, 8 insertions(+), 18 deletions(-) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index 4bffacee74..06ab8f396f 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -458,21 +458,20 @@ export class Connection extends TypedEventEmitter { this[kMessageStream].destroy(); this.closed = true; - callback = callback - ? callback - : () => { - /* Ignore */ - }; if (options.force) { this[kStream].destroy(); - return process.nextTick(callback); + if (callback) { + return process.nextTick(callback); + } } if (!this[kStream].writableEnded) { this[kStream].end(callback); } else { - process.nextTick(callback); + if (callback) { + return process.nextTick(callback); + } } } diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index 19b5af5484..8850063c36 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -484,18 +484,9 @@ export class Topology extends TypedEventEmitter { } /** Close this topology */ - close(callback: Callback): void; close(options: CloseOptions): void; close(options: CloseOptions, callback: Callback): void; - close(options?: CloseOptions | Callback, callback?: Callback): void { - if (typeof options === 'function') { - callback = options; - options = {}; - } - - if (typeof options === 'boolean') { - options = { force: options }; - } + close(options?: CloseOptions, callback?: Callback): void { options = options ?? { force: false }; if (this.s.state === STATE_CLOSED || this.s.state === STATE_CLOSING) { @@ -503,7 +494,7 @@ export class Topology extends TypedEventEmitter { } const destroyedServers = Array.from(this.s.servers.values(), server => { - return promisify(destroyServer)(server, this, Object.assign({ force: false }, options)); + return promisify(destroyServer)(server, this, { force: !!options?.force }); }); Promise.all(destroyedServers) From e5c8e0af50834db48357ef2df1f3d5bb2c59d59a Mon Sep 17 00:00:00 2001 From: Warren James Date: Mon, 12 Dec 2022 16:08:48 -0500 Subject: [PATCH 24/36] test(NODE-4834): Update old test --- test/integration/crud/misc_cursors.test.js | 75 ++++++++-------------- 1 file changed, 26 insertions(+), 49 deletions(-) diff --git a/test/integration/crud/misc_cursors.test.js b/test/integration/crud/misc_cursors.test.js index ff3776ff35..392689cee8 100644 --- a/test/integration/crud/misc_cursors.test.js +++ b/test/integration/crud/misc_cursors.test.js @@ -13,6 +13,7 @@ const { ReadPreference } = require('../../../src/read_preference'); const { ServerType } = require('../../../src/sdam/common'); const { formatSort } = require('../../../src/sort'); const { getSymbolFrom } = require('../../tools/utils'); +const { MongoExpiredSessionError } = require('../../../src/error'); describe('Cursor', function () { before(function () { @@ -1905,62 +1906,38 @@ describe('Cursor', function () { } }); - it('should close dead tailable cursors', { - metadata: { - os: '!win32' // NODE-2943: timeout on windows - }, - - test: function (done) { - // http://www.mongodb.org/display/DOCS/Tailable+Cursors - - const configuration = this.configuration; - client.connect((err, client) => { - expect(err).to.not.exist; - this.defer(() => client.close()); - - const db = client.db(configuration.db); - const options = { capped: true, size: 10000000 }; - db.createCollection( - 'test_if_dead_tailable_cursors_close', - options, - function (err, collection) { - expect(err).to.not.exist; - - let closeCount = 0; - const docs = Array.from({ length: 100 }).map(() => ({ a: 1 })); - collection.insertMany(docs, { w: 'majority', wtimeoutMS: 5000 }, err => { - expect(err).to.not.exist; + it.only('closes cursors when client is closed even if it has not been exhausted', { + metadata: { + os: '!win32' // NODE-2943: timeout on windows + }, - const cursor = collection.find({}, { tailable: true, awaitData: true }); - const stream = cursor.stream(); + async test() { + await client + .db() + .dropCollection('test_cleanup_tailable') + .catch(() => null); - stream.resume(); + const collection = await client + .db() + .createCollection('test_cleanup_tailable', { capped: true, size: 1000, max: 3 }); - var validator = () => { - closeCount++; - if (closeCount === 2) { - done(); - } - }; + // insert only 2 docs in capped coll of 3 + await collection.insertMany([{ a: 1 }, { a: 1 }]); - // we validate that the stream "ends" either cleanly or with an error - stream.on('end', validator); - stream.on('error', validator); + const cursor = collection.find({}, { tailable: true, awaitData: true, maxAwaitTimeMS: 2000 }); - cursor.on('close', validator); + await cursor.next(); + await cursor.next(); + // will block for maxAwaitTimeMS (except we are closing the client) + const rejectedEarlyBecauseClientClosed = cursor.next().catch(error => error); - const docs = Array.from({ length: 100 }).map(() => ({ a: 1 })); - collection.insertMany(docs, err => { - expect(err).to.not.exist; + await client.close(); + expect(cursor).to.have.property('killed', true); - setTimeout(() => client.close()); - }); - }); - } - ); - }); - } - }); + const error = await rejectedEarlyBecauseClientClosed; + expect(error).to.be.instanceOf(MongoExpiredSessionError); + } + }); it('shouldAwaitData', { // Add a tag that our runner can trigger on From 654019759ee80a44a98b071695d62a92efac46e9 Mon Sep 17 00:00:00 2001 From: Warren James Date: Mon, 12 Dec 2022 16:24:03 -0500 Subject: [PATCH 25/36] test(NODE-4834): remove 'only' annotation --- test/integration/crud/misc_cursors.test.js | 46 ++++++++++------------ 1 file changed, 20 insertions(+), 26 deletions(-) diff --git a/test/integration/crud/misc_cursors.test.js b/test/integration/crud/misc_cursors.test.js index 392689cee8..31db6d4de8 100644 --- a/test/integration/crud/misc_cursors.test.js +++ b/test/integration/crud/misc_cursors.test.js @@ -1906,38 +1906,32 @@ describe('Cursor', function () { } }); - it.only('closes cursors when client is closed even if it has not been exhausted', { - metadata: { - os: '!win32' // NODE-2943: timeout on windows - }, + it('closes cursors when client is closed even if it has not been exhausted', async function () { + await client + .db() + .dropCollection('test_cleanup_tailable') + .catch(() => null); - async test() { - await client - .db() - .dropCollection('test_cleanup_tailable') - .catch(() => null); + const collection = await client + .db() + .createCollection('test_cleanup_tailable', { capped: true, size: 1000, max: 3 }); - const collection = await client - .db() - .createCollection('test_cleanup_tailable', { capped: true, size: 1000, max: 3 }); + // insert only 2 docs in capped coll of 3 + await collection.insertMany([{ a: 1 }, { a: 1 }]); - // insert only 2 docs in capped coll of 3 - await collection.insertMany([{ a: 1 }, { a: 1 }]); + const cursor = collection.find({}, { tailable: true, awaitData: true, maxAwaitTimeMS: 2000 }); - const cursor = collection.find({}, { tailable: true, awaitData: true, maxAwaitTimeMS: 2000 }); + await cursor.next(); + await cursor.next(); + // will block for maxAwaitTimeMS (except we are closing the client) + const rejectedEarlyBecauseClientClosed = cursor.next().catch(error => error); - await cursor.next(); - await cursor.next(); - // will block for maxAwaitTimeMS (except we are closing the client) - const rejectedEarlyBecauseClientClosed = cursor.next().catch(error => error); - - await client.close(); - expect(cursor).to.have.property('killed', true); + await client.close(); + expect(cursor).to.have.property('killed', true); - const error = await rejectedEarlyBecauseClientClosed; - expect(error).to.be.instanceOf(MongoExpiredSessionError); - } - }); + const error = await rejectedEarlyBecauseClientClosed; + expect(error).to.be.instanceOf(MongoExpiredSessionError); + }); it('shouldAwaitData', { // Add a tag that our runner can trigger on From 7d43afbee4c41ad9d1e8f87f55b263fb97b0f48f Mon Sep 17 00:00:00 2001 From: Warren James Date: Tue, 13 Dec 2022 12:59:27 -0500 Subject: [PATCH 26/36] test(NODE-4834): Fix failing tests --- test/integration/crud/misc_cursors.test.js | 2 +- .../integration/node-specific/topology.test.js | 18 +++++++++++++----- ..._records_for_mongos_discovery.prose.test.ts | 2 +- .../assorted/server_selection_spec_helper.js | 2 +- test/unit/error.test.ts | 2 +- test/unit/sdam/monitor.test.ts | 4 ++-- test/unit/sdam/topology.test.js | 18 +++++++++--------- 7 files changed, 28 insertions(+), 20 deletions(-) diff --git a/test/integration/crud/misc_cursors.test.js b/test/integration/crud/misc_cursors.test.js index 31db6d4de8..87fd9187f1 100644 --- a/test/integration/crud/misc_cursors.test.js +++ b/test/integration/crud/misc_cursors.test.js @@ -1906,7 +1906,7 @@ describe('Cursor', function () { } }); - it('closes cursors when client is closed even if it has not been exhausted', async function () { + it('closes cursors when client is closed even if it has not been exhausted', async function () { await client .db() .dropCollection('test_cleanup_tailable') diff --git a/test/integration/node-specific/topology.test.js b/test/integration/node-specific/topology.test.js index ee806b9691..912c1443c4 100644 --- a/test/integration/node-specific/topology.test.js +++ b/test/integration/node-specific/topology.test.js @@ -10,12 +10,20 @@ describe('Topology', function () { const states = []; topology.on('stateChanged', (_, newState) => states.push(newState)); topology.connect(err => { - expect(err).to.not.exist; - topology.close(err => { + try { expect(err).to.not.exist; - expect(topology.isDestroyed()).to.be.true; - expect(states).to.eql(['connecting', 'connected', 'closing', 'closed']); - done(); + } catch (error) { + done(error); + } + topology.close({}, err => { + try { + expect(err).to.not.exist; + expect(topology.isDestroyed()).to.be.true; + expect(states).to.eql(['connecting', 'connected', 'closing', 'closed']); + done(); + } catch (error) { + done(error); + } }); }); } diff --git a/test/unit/assorted/polling_srv_records_for_mongos_discovery.prose.test.ts b/test/unit/assorted/polling_srv_records_for_mongos_discovery.prose.test.ts index c96f738fa9..339f7c065b 100644 --- a/test/unit/assorted/polling_srv_records_for_mongos_discovery.prose.test.ts +++ b/test/unit/assorted/polling_srv_records_for_mongos_discovery.prose.test.ts @@ -97,7 +97,7 @@ describe('Polling Srv Records for Mongos Discovery', () => { afterEach(function (done) { if (context.topology) { - context.topology.close(done); + context.topology.close({}, done); } else { done(); } diff --git a/test/unit/assorted/server_selection_spec_helper.js b/test/unit/assorted/server_selection_spec_helper.js index 3490ae9aac..724a8e4981 100644 --- a/test/unit/assorted/server_selection_spec_helper.js +++ b/test/unit/assorted/server_selection_spec_helper.js @@ -109,7 +109,7 @@ function executeServerSelectionTest(testDefinition, testDone) { }); function done(err) { - topology.close(e => testDone(e || err)); + topology.close({}, e => testDone(e || err)); } topology.connect(err => { diff --git a/test/unit/error.test.ts b/test/unit/error.test.ts index 3ae43e35ea..14a624738f 100644 --- a/test/unit/error.test.ts +++ b/test/unit/error.test.ts @@ -379,7 +379,7 @@ describe('MongoErrors', () => { makeAndConnectReplSet((err, topology) => { // cleanup the server before calling done - const cleanup = err => topology.close(err2 => done(err || err2)); + const cleanup = err => topology.close({}, err2 => done(err || err2)); if (err) { return cleanup(err); diff --git a/test/unit/sdam/monitor.test.ts b/test/unit/sdam/monitor.test.ts index be7998d224..4bb611155d 100644 --- a/test/unit/sdam/monitor.test.ts +++ b/test/unit/sdam/monitor.test.ts @@ -52,7 +52,7 @@ describe('monitoring', function () { const serverDescription = Array.from(topology.description.servers.values())[0]; expect(serverDescription).property('roundTripTime').to.be.greaterThan(0); - topology.close(done as any); + topology.close({}, done as any); }, 500); }); }).skipReason = 'TODO(NODE-3819): Unskip flaky tests'; @@ -92,7 +92,7 @@ describe('monitoring', function () { const serverDescription = Array.from(topology.description.servers.values())[0]; expect(serverDescription).property('roundTripTime').to.be.greaterThan(0); - topology.close(done); + topology.close({}, done); }); }).skipReason = 'TODO(NODE-3600): Unskip flaky tests'; diff --git a/test/unit/sdam/topology.test.js b/test/unit/sdam/topology.test.js index 978386697b..d3313010f3 100644 --- a/test/unit/sdam/topology.test.js +++ b/test/unit/sdam/topology.test.js @@ -26,7 +26,7 @@ describe('Topology (unit)', function () { } if (topology) { - topology.close(); + topology.close({}); } }); @@ -107,7 +107,7 @@ describe('Topology (unit)', function () { topology.connect(() => { expect(topology.shouldCheckForSessionSupport()).to.be.true; - topology.close(done); + topology.close({}, done); }); }); @@ -127,7 +127,7 @@ describe('Topology (unit)', function () { topology.connect(() => { expect(topology.shouldCheckForSessionSupport()).to.be.false; - topology.close(done); + topology.close({}, done); }); }); @@ -147,7 +147,7 @@ describe('Topology (unit)', function () { topology.connect(() => { expect(topology.shouldCheckForSessionSupport()).to.be.false; - topology.close(done); + topology.close({}, done); }); }); }); @@ -182,7 +182,7 @@ describe('Topology (unit)', function () { expect(err).to.exist; expect(err).to.match(/timed out/); - topology.close(done); + topology.close({}, done); }); }); }); @@ -325,7 +325,7 @@ describe('Topology (unit)', function () { expect(err).to.exist; expect(err).to.eql(serverDescription.error); expect(poolCleared).to.be.false; - topology.close(done); + topology.close({}, done); }); }); }); @@ -467,7 +467,7 @@ describe('Topology (unit)', function () { it('should clean up listeners on close', function (done) { topology.s.state = 'connected'; // fake state to test clean up logic - topology.close(e => { + topology.close({}, e => { const srvPollerListeners = topology.s.srvPoller.listeners( SrvPoller.SRV_RECORD_DISCOVERY ); @@ -547,7 +547,7 @@ describe('Topology (unit)', function () { // occurs `requestCheck` will be called for an immediate check. expect(requestCheck).property('callCount').to.equal(1); - topology.close(done); + topology.close({}, done); }); }); }); @@ -559,7 +559,7 @@ describe('Topology (unit)', function () { this.emit('connect'); }); - topology.close(() => { + topology.close({}, () => { topology.selectServer(ReadPreference.primary, { serverSelectionTimeoutMS: 2000 }, err => { expect(err).to.exist; expect(err).to.match(/Topology is closed/); From 2cdcc21e3aab4b86215e77a52eba1d67c7415e89 Mon Sep 17 00:00:00 2001 From: Warren James Date: Tue, 13 Dec 2022 13:36:26 -0500 Subject: [PATCH 27/36] test(NODE-4834): remove debug messages --- test/integration/node-specific/topology.test.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/integration/node-specific/topology.test.js b/test/integration/node-specific/topology.test.js index 912c1443c4..e8891db7bf 100644 --- a/test/integration/node-specific/topology.test.js +++ b/test/integration/node-specific/topology.test.js @@ -5,6 +5,7 @@ describe('Topology', function () { it('should correctly track states of a topology', { metadata: { requires: { apiVersion: false, topology: '!load-balanced' } }, // apiVersion not supported by newTopology() test: function (done) { + debugger; const topology = this.configuration.newTopology(); const states = []; @@ -15,7 +16,9 @@ describe('Topology', function () { } catch (error) { done(error); } + console.log('before close'); topology.close({}, err => { + console.log('In close'); try { expect(err).to.not.exist; expect(topology.isDestroyed()).to.be.true; From 8d66f61e0c71dad71f37f4247e723dbed13766ce Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 13 Dec 2022 14:34:33 -0500 Subject: [PATCH 28/36] fix: only invoke callback when it is defined --- test/unit/cmap/connection.test.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/unit/cmap/connection.test.ts b/test/unit/cmap/connection.test.ts index 5f2ee16496..8c6fe98ddc 100644 --- a/test/unit/cmap/connection.test.ts +++ b/test/unit/cmap/connection.test.ts @@ -40,7 +40,9 @@ class FakeSocket extends EventEmitter { end(cb) { this.writableEnded = true; // nextTick to simulate I/O delay - process.nextTick(cb); + if (typeof cb === 'function') { + process.nextTick(cb); + } } get remoteAddress() { return 'iLoveJavaScript'; @@ -58,7 +60,9 @@ class InputStream extends Readable { end(cb) { this.writableEnded = true; - process.nextTick(cb); + if (typeof cb === 'function') { + process.nextTick(cb); + } } } From 44d5b05a38d4d0691c38e64b6ea871dfc15f3098 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 13 Dec 2022 15:14:54 -0500 Subject: [PATCH 29/36] fix: lint --- test/integration/node-specific/topology.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/integration/node-specific/topology.test.js b/test/integration/node-specific/topology.test.js index e8891db7bf..72df055e42 100644 --- a/test/integration/node-specific/topology.test.js +++ b/test/integration/node-specific/topology.test.js @@ -5,7 +5,6 @@ describe('Topology', function () { it('should correctly track states of a topology', { metadata: { requires: { apiVersion: false, topology: '!load-balanced' } }, // apiVersion not supported by newTopology() test: function (done) { - debugger; const topology = this.configuration.newTopology(); const states = []; From 860d741a0ec27b13d9c2b949fa9842f176565c5b Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 13 Dec 2022 16:44:49 -0500 Subject: [PATCH 30/36] rm: prints --- test/integration/node-specific/topology.test.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/integration/node-specific/topology.test.js b/test/integration/node-specific/topology.test.js index 72df055e42..912c1443c4 100644 --- a/test/integration/node-specific/topology.test.js +++ b/test/integration/node-specific/topology.test.js @@ -15,9 +15,7 @@ describe('Topology', function () { } catch (error) { done(error); } - console.log('before close'); topology.close({}, err => { - console.log('In close'); try { expect(err).to.not.exist; expect(topology.isDestroyed()).to.be.true; From 3b59a122eeeb941b090709eb000f1110f66a4039 Mon Sep 17 00:00:00 2001 From: Warren James Date: Thu, 15 Dec 2022 16:04:24 -0500 Subject: [PATCH 31/36] test(NODE-4834): Update tests to be more coherent --- test/unit/cmap/connection.test.ts | 35 ++++++++++++++++++++++++++++--- 1 file changed, 32 insertions(+), 3 deletions(-) diff --git a/test/unit/cmap/connection.test.ts b/test/unit/cmap/connection.test.ts index 8c6fe98ddc..aeeb59947b 100644 --- a/test/unit/cmap/connection.test.ts +++ b/test/unit/cmap/connection.test.ts @@ -620,15 +620,44 @@ describe('new Connection()', function () { expect(driverSocket.end).to.have.been.calledOnce; }); - it('does not call stream.end after onClose, onTimeout, or onError', () => { + it('calls stream.end exactly once when onError is called', () => { messageStream.emit('error'); clock.tick(1); expect(connection.onError).to.have.been.calledOnce; + connection.destroy({ force: false }); + clock.tick(1); + expect(driverSocket.destroy).to.not.have.been.called; + expect(driverSocket.end).to.have.been.calledOnce; + }); + + it('calls stream.end exactly once when onClose is called', () => { + driverSocket.emit('close'); + clock.tick(1); + expect(connection.onClose).to.have.been.calledOnce; + connection.destroy({ force: false }); + clock.tick(1); + expect(driverSocket.destroy).to.not.have.been.called; + expect(driverSocket.end).to.have.been.calledOnce; + }); + + it('calls stream.end exactly once when onTimeout is called', () => { + driverSocket.emit('timeout'); clock.tick(1); - expect(driverSocket.destroy).to.not.have.been.calledOnce; + expect(connection.onTimeout).to.have.been.calledOnce; + connection.destroy({ force: false }); + clock.tick(1); + expect(driverSocket.destroy).to.not.have.been.called; + expect(driverSocket.end).to.have.been.calledOnce; + }); + + it('calls stream.end exactly once when destroy is called multiple times', () => { + connection.destroy({ force: false }); + connection.destroy({ force: false }); + connection.destroy({ force: false }); connection.destroy({ force: false }); clock.tick(1); - expect(driverSocket.end).to.have.been.called; + expect(driverSocket.destroy).to.not.have.been.called; + expect(driverSocket.end).to.have.been.calledOnce; }); it('does not call stream.end if options.force == true', () => { From 50fc706a115a140c20030d2152c681f145cc3640 Mon Sep 17 00:00:00 2001 From: Warren James Date: Thu, 15 Dec 2022 16:13:23 -0500 Subject: [PATCH 32/36] test(NODE-4834): Reorganize tests --- test/unit/cmap/connection.test.ts | 49 +++++++------------------------ 1 file changed, 11 insertions(+), 38 deletions(-) diff --git a/test/unit/cmap/connection.test.ts b/test/unit/cmap/connection.test.ts index aeeb59947b..f32f148517 100644 --- a/test/unit/cmap/connection.test.ts +++ b/test/unit/cmap/connection.test.ts @@ -470,6 +470,7 @@ describe('new Connection()', function () { expect(connection).to.have.property(kDelayedTimeoutId).that.is.instanceOf(NodeJSTimeoutClass); expect(messageStream.destroy).to.have.been.calledOnce; + expect(driverSocket.destroy).to.not.have.been.calledOnce; expect(driverSocket.end).to.have.been.calledOnce; }); }); @@ -497,12 +498,12 @@ describe('new Connection()', function () { it('destroys the message stream and socket', () => { messageStream.emit('error'); - clock.tick(1); - expect(connection.onError).to.have.been.calledOnce; - - expect(messageStream.destroy).to.have.been.calledOnce; + connection.destroy({ force: false }); + clock.tick(1); + expect(messageStream.destroy).to.not.have.been.calledOnce; + expect(driverSocket.destroy).to.not.have.been.called; expect(driverSocket.end).to.have.been.calledOnce; }); }); @@ -529,13 +530,15 @@ describe('new Connection()', function () { clock.restore(); }); - it('destroys the message stream', () => { + it('destroys the message stream and socket', () => { driverSocket.emit('close'); - clock.tick(1); - expect(connection.onClose).to.have.been.calledOnce; - expect(messageStream.destroy).to.have.been.calledOnce; + connection.destroy({ force: false }); + clock.tick(1); + expect(messageStream.destroy).to.not.have.been.calledOnce; + expect(driverSocket.destroy).to.not.have.been.called; + expect(driverSocket.end).to.have.been.calledOnce; }); }); @@ -620,36 +623,6 @@ describe('new Connection()', function () { expect(driverSocket.end).to.have.been.calledOnce; }); - it('calls stream.end exactly once when onError is called', () => { - messageStream.emit('error'); - clock.tick(1); - expect(connection.onError).to.have.been.calledOnce; - connection.destroy({ force: false }); - clock.tick(1); - expect(driverSocket.destroy).to.not.have.been.called; - expect(driverSocket.end).to.have.been.calledOnce; - }); - - it('calls stream.end exactly once when onClose is called', () => { - driverSocket.emit('close'); - clock.tick(1); - expect(connection.onClose).to.have.been.calledOnce; - connection.destroy({ force: false }); - clock.tick(1); - expect(driverSocket.destroy).to.not.have.been.called; - expect(driverSocket.end).to.have.been.calledOnce; - }); - - it('calls stream.end exactly once when onTimeout is called', () => { - driverSocket.emit('timeout'); - clock.tick(1); - expect(connection.onTimeout).to.have.been.calledOnce; - connection.destroy({ force: false }); - clock.tick(1); - expect(driverSocket.destroy).to.not.have.been.called; - expect(driverSocket.end).to.have.been.calledOnce; - }); - it('calls stream.end exactly once when destroy is called multiple times', () => { connection.destroy({ force: false }); connection.destroy({ force: false }); From a11a4f6731e14290bd4b62966a8b9b5dd0e25acd Mon Sep 17 00:00:00 2001 From: Warren James Date: Thu, 15 Dec 2022 16:38:51 -0500 Subject: [PATCH 33/36] test(NODE-4834): remove to.not.have.been.calledOnce instances --- test/unit/cmap/connection.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/unit/cmap/connection.test.ts b/test/unit/cmap/connection.test.ts index f32f148517..e73e994041 100644 --- a/test/unit/cmap/connection.test.ts +++ b/test/unit/cmap/connection.test.ts @@ -470,7 +470,7 @@ describe('new Connection()', function () { expect(connection).to.have.property(kDelayedTimeoutId).that.is.instanceOf(NodeJSTimeoutClass); expect(messageStream.destroy).to.have.been.calledOnce; - expect(driverSocket.destroy).to.not.have.been.calledOnce; + expect(driverSocket.destroy).to.not.have.been.called; expect(driverSocket.end).to.have.been.calledOnce; }); }); @@ -502,7 +502,7 @@ describe('new Connection()', function () { expect(connection.onError).to.have.been.calledOnce; connection.destroy({ force: false }); clock.tick(1); - expect(messageStream.destroy).to.not.have.been.calledOnce; + expect(messageStream.destroy).to.have.been.called; expect(driverSocket.destroy).to.not.have.been.called; expect(driverSocket.end).to.have.been.calledOnce; }); @@ -536,7 +536,7 @@ describe('new Connection()', function () { expect(connection.onClose).to.have.been.calledOnce; connection.destroy({ force: false }); clock.tick(1); - expect(messageStream.destroy).to.not.have.been.calledOnce; + expect(messageStream.destroy).to.have.been.called; expect(driverSocket.destroy).to.not.have.been.called; expect(driverSocket.end).to.have.been.calledOnce; }); From 23755ca7669c270fd29502838b0bf042b826839a Mon Sep 17 00:00:00 2001 From: Warren James Date: Fri, 16 Dec 2022 09:47:31 -0500 Subject: [PATCH 34/36] test(NODE-4834): Separate tests more cleanly --- test/unit/cmap/connection.test.ts | 34 ++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 10 deletions(-) diff --git a/test/unit/cmap/connection.test.ts b/test/unit/cmap/connection.test.ts index e73e994041..6319afece5 100644 --- a/test/unit/cmap/connection.test.ts +++ b/test/unit/cmap/connection.test.ts @@ -633,18 +633,32 @@ describe('new Connection()', function () { expect(driverSocket.end).to.have.been.calledOnce; }); - it('does not call stream.end if options.force == true', () => { - connection.destroy({ force: true }); - clock.tick(1); - expect(driverSocket.destroy).to.have.been.calledOnce; - expect(driverSocket.end).to.not.have.been.called; + context('when options.force == true', function () { + it('calls steam.destroy', () => { + connection.destroy({ force: true }); + clock.tick(1); + expect(driverSocket.destroy).to.have.been.calledOnce; + }); + + it('does not call stream.end', () => { + connection.destroy({ force: true }); + clock.tick(1); + expect(driverSocket.end).to.not.have.been.called; + }); }); - it('does not call stream.destroy if options.force == false', () => { - connection.destroy({ force: false }); - clock.tick(1); - expect(driverSocket.destroy).to.not.have.been.called; - expect(driverSocket.end).to.have.been.calledOnce; + context('when options.force == false', function () { + it('calls stream.end', () => { + connection.destroy({ force: false }); + clock.tick(1); + expect(driverSocket.end).to.have.been.calledOnce; + }); + + it('does not call stream.destroy', () => { + connection.destroy({ force: false }); + clock.tick(1); + expect(driverSocket.destroy).to.not.have.been.called; + }); }); }); }); From 878c7649311e938cdb05a93eac3b5b9c2e925409 Mon Sep 17 00:00:00 2001 From: Warren James Date: Fri, 16 Dec 2022 10:39:13 -0500 Subject: [PATCH 35/36] test(NODE-4834): Split up tests more cleanly --- test/unit/cmap/connection.test.ts | 58 ++++++++++++++++++++++--------- 1 file changed, 41 insertions(+), 17 deletions(-) diff --git a/test/unit/cmap/connection.test.ts b/test/unit/cmap/connection.test.ts index 6319afece5..c9d34c0419 100644 --- a/test/unit/cmap/connection.test.ts +++ b/test/unit/cmap/connection.test.ts @@ -616,23 +616,6 @@ describe('new Connection()', function () { clock.restore(); }); - it('ends the tcp socket and destroys the messageStream', () => { - connection.destroy({ force: false }); - clock.tick(1); - expect(messageStream.destroy).to.have.been.calledOnce; - expect(driverSocket.end).to.have.been.calledOnce; - }); - - it('calls stream.end exactly once when destroy is called multiple times', () => { - connection.destroy({ force: false }); - connection.destroy({ force: false }); - connection.destroy({ force: false }); - connection.destroy({ force: false }); - clock.tick(1); - expect(driverSocket.destroy).to.not.have.been.called; - expect(driverSocket.end).to.have.been.calledOnce; - }); - context('when options.force == true', function () { it('calls steam.destroy', () => { connection.destroy({ force: true }); @@ -645,6 +628,26 @@ describe('new Connection()', function () { clock.tick(1); expect(driverSocket.end).to.not.have.been.called; }); + + it('destroys the tcp socket', () => { + connection.destroy({ force: true }); + clock.tick(1); + expect(driverSocket.destroy).to.have.been.calledOnce; + }); + + it('destroys the messageStream', () => { + connection.destroy({ force: true }); + clock.tick(1); + expect(messageStream.destroy).to.have.been.calledOnce; + }); + + it('calls stream.destroy whenever destroy is called ', () => { + connection.destroy({ force: true }); + connection.destroy({ force: true }); + connection.destroy({ force: true }); + clock.tick(1); + expect(driverSocket.destroy).to.have.been.calledThrice; + }); }); context('when options.force == false', function () { @@ -659,6 +662,27 @@ describe('new Connection()', function () { clock.tick(1); expect(driverSocket.destroy).to.not.have.been.called; }); + + it('ends the tcp socket', () => { + connection.destroy({ force: false }); + clock.tick(1); + expect(driverSocket.end).to.have.been.calledOnce; + }); + + it('destroys the messageStream', () => { + connection.destroy({ force: false }); + clock.tick(1); + expect(messageStream.destroy).to.have.been.calledOnce; + }); + + it('calls stream.end exactly once when destroy is called multiple times', () => { + connection.destroy({ force: false }); + connection.destroy({ force: false }); + connection.destroy({ force: false }); + connection.destroy({ force: false }); + clock.tick(1); + expect(driverSocket.end).to.have.been.calledOnce; + }); }); }); }); From 995700b661d1ebafc907e2b473325fb60a32abd3 Mon Sep 17 00:00:00 2001 From: Warren James Date: Fri, 16 Dec 2022 10:50:18 -0500 Subject: [PATCH 36/36] test(NODE-4834): fix test name typo --- test/unit/cmap/connection.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/cmap/connection.test.ts b/test/unit/cmap/connection.test.ts index c9d34c0419..b29bf3a2e9 100644 --- a/test/unit/cmap/connection.test.ts +++ b/test/unit/cmap/connection.test.ts @@ -617,7 +617,7 @@ describe('new Connection()', function () { }); context('when options.force == true', function () { - it('calls steam.destroy', () => { + it('calls stream.destroy', () => { connection.destroy({ force: true }); clock.tick(1); expect(driverSocket.destroy).to.have.been.calledOnce;