From cdce4bfe432728d0b7393a18466e4b905a3a078c Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Fri, 16 Sep 2022 12:00:12 -0400 Subject: [PATCH 1/6] test: add tests for MonitorInterval --- test/unit/sdam/monitor.test.ts | 74 ++++++++++------------------------ 1 file changed, 22 insertions(+), 52 deletions(-) diff --git a/test/unit/sdam/monitor.test.ts b/test/unit/sdam/monitor.test.ts index be7998d224..a475409fa5 100644 --- a/test/unit/sdam/monitor.test.ts +++ b/test/unit/sdam/monitor.test.ts @@ -400,71 +400,41 @@ describe('monitoring', function () { }); }); - context( - 'when it has been < minHeartbeatFrequencyMS and >= 0 since fn() last completed', - function () { - beforeEach(function () { - executor = new MonitorInterval(fnSpy, { - minHeartbeatFrequencyMS: 10, - heartbeatFrequencyMS: 30 - }); - - // call fn() once - clock.tick(30); - expect(fnSpy.calledOnce).to.be.true; - - fnSpy.callCount = 0; - - // advance less than minHeartbeatFrequency - clock.tick(5); - - executor.wake(); - }); - - it('reschedules fn() to minHeartbeatFrequencyMS after the last call', function () { - expect(fnSpy.callCount).to.equal(0); - clock.tick(5); - expect(fnSpy.calledOnce).to.be.true; - }); - - context('when wake() is called more than once', function () { - it('schedules fn() minHeartbeatFrequencyMS after the last call to fn()', function () { - executor.wake(); - executor.wake(); - executor.wake(); - - expect(fnSpy.callCount).to.equal(0); - clock.tick(5); - expect(fnSpy.calledOnce).to.be.true; - }); - }); - } - ); - - context('when it has been <0 since fn() has last executed', function () { + context('when it has been < minHeartbeatFrequencyMS since fn() last completed', function () { beforeEach(function () { executor = new MonitorInterval(fnSpy, { minHeartbeatFrequencyMS: 10, heartbeatFrequencyMS: 30 }); - // negative ticks aren't supported, so manually set execution time - executor.lastExecutionEnded = Infinity; + // call fn() once + clock.tick(30); + expect(fnSpy.calledOnce).to.be.true; + + fnSpy.callCount = 0; + + // advance less than minHeartbeatFrequency + clock.tick(5); + executor.wake(); }); - it('executes fn() immediately', function () { + it('reschedules fn() to minHeartbeatFrequencyMS after the last call', function () { + expect(fnSpy.callCount).to.equal(0); + clock.tick(5); expect(fnSpy.calledOnce).to.be.true; }); - it('reschedules fn() to minHeartbeatFrequency away', function () { - fnSpy.callCount = 0; - - clock.tick(29); - expect(fnSpy.callCount).to.equal(0); + context('when wake() is called more than once', function () { + it('schedules fn() minHeartbeatFrequencyMS after the last call to fn()', function () { + executor.wake(); + executor.wake(); + executor.wake(); - clock.tick(1); - expect(fnSpy.calledOnce).to.be.true; + expect(fnSpy.callCount).to.equal(0); + clock.tick(5); + expect(fnSpy.calledOnce).to.be.true; + }); }); }); }); From 0ace76c65d9346dab0669aadfec0bab8dd03bc25 Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Fri, 16 Sep 2022 12:14:44 -0400 Subject: [PATCH 2/6] fix: prevent parallel monitor checks --- src/sdam/monitor.ts | 4 + ...rver_discovery_and_monitoring.spec.test.ts | 4 +- test/unit/sdam/monitor.test.ts | 74 +++++++++++++------ 3 files changed, 58 insertions(+), 24 deletions(-) diff --git a/src/sdam/monitor.ts b/src/sdam/monitor.ts index 909727c9e7..7f8f4561f5 100644 --- a/src/sdam/monitor.ts +++ b/src/sdam/monitor.ts @@ -328,6 +328,10 @@ function checkServer(monitor: Monitor, callback: Callback) { function monitorServer(monitor: Monitor) { return (callback: Callback) => { + if (monitor.s.state === STATE_MONITORING) { + callback(); + return; + } stateTransition(monitor, STATE_MONITORING); function done() { if (!isInCloseState(monitor)) { diff --git a/test/integration/server-discovery-and-monitoring/server_discovery_and_monitoring.spec.test.ts b/test/integration/server-discovery-and-monitoring/server_discovery_and_monitoring.spec.test.ts index e99be158ca..d93400f28b 100644 --- a/test/integration/server-discovery-and-monitoring/server_discovery_and_monitoring.spec.test.ts +++ b/test/integration/server-discovery-and-monitoring/server_discovery_and_monitoring.spec.test.ts @@ -25,8 +25,8 @@ describe('SDAM Unified Tests', function () { // TODO(NODE-4573): fix socket leaks const LEAKY_TESTS = [ 'Command error on Monitor handshake', - 'Network error on Monitor check', - 'Network timeout on Monitor check', + // 'Network error on Monitor check', + // 'Network timeout on Monitor check', 'Network error on Monitor handshake', 'Network timeout on Monitor handshake' ]; diff --git a/test/unit/sdam/monitor.test.ts b/test/unit/sdam/monitor.test.ts index a475409fa5..be7998d224 100644 --- a/test/unit/sdam/monitor.test.ts +++ b/test/unit/sdam/monitor.test.ts @@ -400,41 +400,71 @@ describe('monitoring', function () { }); }); - context('when it has been < minHeartbeatFrequencyMS since fn() last completed', function () { + context( + 'when it has been < minHeartbeatFrequencyMS and >= 0 since fn() last completed', + function () { + beforeEach(function () { + executor = new MonitorInterval(fnSpy, { + minHeartbeatFrequencyMS: 10, + heartbeatFrequencyMS: 30 + }); + + // call fn() once + clock.tick(30); + expect(fnSpy.calledOnce).to.be.true; + + fnSpy.callCount = 0; + + // advance less than minHeartbeatFrequency + clock.tick(5); + + executor.wake(); + }); + + it('reschedules fn() to minHeartbeatFrequencyMS after the last call', function () { + expect(fnSpy.callCount).to.equal(0); + clock.tick(5); + expect(fnSpy.calledOnce).to.be.true; + }); + + context('when wake() is called more than once', function () { + it('schedules fn() minHeartbeatFrequencyMS after the last call to fn()', function () { + executor.wake(); + executor.wake(); + executor.wake(); + + expect(fnSpy.callCount).to.equal(0); + clock.tick(5); + expect(fnSpy.calledOnce).to.be.true; + }); + }); + } + ); + + context('when it has been <0 since fn() has last executed', function () { beforeEach(function () { executor = new MonitorInterval(fnSpy, { minHeartbeatFrequencyMS: 10, heartbeatFrequencyMS: 30 }); - // call fn() once - clock.tick(30); - expect(fnSpy.calledOnce).to.be.true; - - fnSpy.callCount = 0; - - // advance less than minHeartbeatFrequency - clock.tick(5); - + // negative ticks aren't supported, so manually set execution time + executor.lastExecutionEnded = Infinity; executor.wake(); }); - it('reschedules fn() to minHeartbeatFrequencyMS after the last call', function () { - expect(fnSpy.callCount).to.equal(0); - clock.tick(5); + it('executes fn() immediately', function () { expect(fnSpy.calledOnce).to.be.true; }); - context('when wake() is called more than once', function () { - it('schedules fn() minHeartbeatFrequencyMS after the last call to fn()', function () { - executor.wake(); - executor.wake(); - executor.wake(); + it('reschedules fn() to minHeartbeatFrequency away', function () { + fnSpy.callCount = 0; - expect(fnSpy.callCount).to.equal(0); - clock.tick(5); - expect(fnSpy.calledOnce).to.be.true; - }); + clock.tick(29); + expect(fnSpy.callCount).to.equal(0); + + clock.tick(1); + expect(fnSpy.calledOnce).to.be.true; }); }); }); From ecd4aa6a9b6b79c6e9e38fbeb0008db7ed557666 Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Mon, 3 Oct 2022 10:50:57 -0400 Subject: [PATCH 3/6] unskip sdam tests --- .../server_discovery_and_monitoring.spec.test.ts | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/test/integration/server-discovery-and-monitoring/server_discovery_and_monitoring.spec.test.ts b/test/integration/server-discovery-and-monitoring/server_discovery_and_monitoring.spec.test.ts index d93400f28b..baec5ba3e9 100644 --- a/test/integration/server-discovery-and-monitoring/server_discovery_and_monitoring.spec.test.ts +++ b/test/integration/server-discovery-and-monitoring/server_discovery_and_monitoring.spec.test.ts @@ -4,19 +4,8 @@ import * as path from 'path'; import { loadSpecTests } from '../../spec'; import { runUnifiedSuite } from '../../tools/unified-spec-runner/runner'; -import { TestFilter } from '../../tools/unified-spec-runner/schema'; import { sleep } from '../../tools/utils'; -const filter: TestFilter = ({ description }) => { - switch (description) { - case 'Network error on Monitor check': - case 'Network timeout on Monitor check': - return 'TODO(NODE-4608): Disallow parallel monitor checks'; - default: - return false; - } -}; - describe('SDAM Unified Tests', function () { afterEach(async function () { if (this.currentTest!.pending) { @@ -46,5 +35,5 @@ describe('SDAM Unified Tests', function () { this.test!.error(new Error(`Test failed to clean up ${sockArray.length} socket(s)`)); } }); - runUnifiedSuite(loadSpecTests(path.join('server-discovery-and-monitoring', 'unified')), filter); + runUnifiedSuite(loadSpecTests(path.join('server-discovery-and-monitoring', 'unified'))); }); From 61a9a0ab566849c40585df6c875841cbe0522f17 Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Mon, 3 Oct 2022 11:32:56 -0400 Subject: [PATCH 4/6] Unskip flakey tests --- .../server_discovery_and_monitoring.spec.test.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/integration/server-discovery-and-monitoring/server_discovery_and_monitoring.spec.test.ts b/test/integration/server-discovery-and-monitoring/server_discovery_and_monitoring.spec.test.ts index baec5ba3e9..57c90ee7d2 100644 --- a/test/integration/server-discovery-and-monitoring/server_discovery_and_monitoring.spec.test.ts +++ b/test/integration/server-discovery-and-monitoring/server_discovery_and_monitoring.spec.test.ts @@ -14,8 +14,6 @@ describe('SDAM Unified Tests', function () { // TODO(NODE-4573): fix socket leaks const LEAKY_TESTS = [ 'Command error on Monitor handshake', - // 'Network error on Monitor check', - // 'Network timeout on Monitor check', 'Network error on Monitor handshake', 'Network timeout on Monitor handshake' ]; From 52f8e205a1319cad1bd7525730ef8278ba8d84ab Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Mon, 3 Oct 2022 12:07:21 -0400 Subject: [PATCH 5/6] unskip ALL tests --- ...rver_discovery_and_monitoring.spec.test.ts | 29 ------------------- 1 file changed, 29 deletions(-) diff --git a/test/integration/server-discovery-and-monitoring/server_discovery_and_monitoring.spec.test.ts b/test/integration/server-discovery-and-monitoring/server_discovery_and_monitoring.spec.test.ts index 57c90ee7d2..74fc67efc6 100644 --- a/test/integration/server-discovery-and-monitoring/server_discovery_and_monitoring.spec.test.ts +++ b/test/integration/server-discovery-and-monitoring/server_discovery_and_monitoring.spec.test.ts @@ -1,37 +1,8 @@ -/* eslint-disable @typescript-eslint/no-non-null-assertion */ -import { Socket } from 'net'; import * as path from 'path'; import { loadSpecTests } from '../../spec'; import { runUnifiedSuite } from '../../tools/unified-spec-runner/runner'; -import { sleep } from '../../tools/utils'; describe('SDAM Unified Tests', function () { - afterEach(async function () { - if (this.currentTest!.pending) { - return; - } - // TODO(NODE-4573): fix socket leaks - const LEAKY_TESTS = [ - 'Command error on Monitor handshake', - 'Network error on Monitor handshake', - 'Network timeout on Monitor handshake' - ]; - - await sleep(250); - const sockArray = (process as any)._getActiveHandles().filter(handle => { - // Stdio are instanceof Socket so look for fd to be null - return handle.fd == null && handle instanceof Socket && handle.destroyed !== true; - }); - if (!sockArray.length) { - return; - } - for (const sock of sockArray) { - sock.destroy(); - } - if (!LEAKY_TESTS.some(test => test === this.currentTest!.title)) { - this.test!.error(new Error(`Test failed to clean up ${sockArray.length} socket(s)`)); - } - }); runUnifiedSuite(loadSpecTests(path.join('server-discovery-and-monitoring', 'unified'))); }); From d0af115c45d1020ebbbae785542b50cefcaacc4f Mon Sep 17 00:00:00 2001 From: Bailey Pearson Date: Tue, 4 Oct 2022 09:39:00 -0400 Subject: [PATCH 6/6] fix: call callback asynchronously --- src/sdam/monitor.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sdam/monitor.ts b/src/sdam/monitor.ts index 7f8f4561f5..3711dc59ed 100644 --- a/src/sdam/monitor.ts +++ b/src/sdam/monitor.ts @@ -329,7 +329,7 @@ function checkServer(monitor: Monitor, callback: Callback) { function monitorServer(monitor: Monitor) { return (callback: Callback) => { if (monitor.s.state === STATE_MONITORING) { - callback(); + process.nextTick(callback); return; } stateTransition(monitor, STATE_MONITORING);