From 4501a1ce55ba4adf141fdb851ccd33ff4f2a4e59 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 1 Jun 2022 16:50:42 +0200 Subject: [PATCH] fix(NODE-4281): ensure that the driver always uses Node.js timers (#3275) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ensure that the driver always uses the Node.js timers API, rather than the global one, in case they diverge. This affects Compass, where the `setTimeout(...).unref()` usage currently results in uncaught exceptions because `setTimeout()` returns a number in browsers. This change adds `import ... from 'timers';` where necessary and adds a linter rule to prevent regressions. If this is not an acceptable solution, we can go back to the drawing board, but this seems like a good solution that doesn’t add too much overhead when writing driver code. --- .eslintrc.json | 15 +++++++++++++ src/change_stream.ts | 1 + src/cmap/connection.ts | 2 ++ src/cmap/connection_pool.ts | 2 ++ src/sdam/monitor.ts | 2 ++ src/sdam/srv_polling.ts | 1 + src/sdam/topology.ts | 2 ++ src/utils.ts | 1 + .../change-streams/change_stream.test.ts | 1 + .../change_streams.prose.test.ts | 1 + test/integration/crud/find.test.js | 1 + test/integration/crud/misc_cursors.test.js | 1 + .../node-specific/cursor_stream.test.js | 1 + .../examples/change_streams.test.js | 1 + .../node-specific/operation_example.test.js | 1 + test/integration/objectid.test.js | 1 + ...er_selection.prose.operation_count.test.ts | 1 + test/integration/shared.js | 1 + test/tools/mongodb-mock/src/server.js | 1 + test/tools/spec-runner/context.js | 1 + test/tools/utils.ts | 1 + ...records_for_mongos_discovery.prose.test.ts | 4 ++++ test/unit/cmap/connect.test.js | 1 + test/unit/cmap/connection.test.ts | 5 +++++ test/unit/cmap/connection_pool.test.js | 1 + test/unit/error.test.ts | 1 + test/unit/sdam/monitor.test.js | 1 + test/unit/sdam/topology.test.js | 1 + test/unit/timer_sandbox.js | 22 +++++++++++++++++++ test/unit/utils.test.js | 5 ++++- 30 files changed, 79 insertions(+), 1 deletion(-) create mode 100644 test/unit/timer_sandbox.js diff --git a/.eslintrc.json b/.eslintrc.json index 3eb33dd8b9..96c9230949 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -38,6 +38,21 @@ "property": "only" } ], + "no-restricted-globals": [ + "error", + { + "name": "setTimeout", + "message": "Use `import { setTimeout } from 'timers';` instead" + }, + { + "name": "setImmediate", + "message": "Use `import { setImmediate } from 'timers';` instead" + }, + { + "name": "setInterval", + "message": "Use `import { setInterval } from 'timers';` instead" + } + ], "prettier/prettier": "error", "tsdoc/syntax": "warn", "no-console": "error", diff --git a/src/change_stream.ts b/src/change_stream.ts index ca846167ec..5ce07fda95 100644 --- a/src/change_stream.ts +++ b/src/change_stream.ts @@ -1,5 +1,6 @@ import Denque = require('denque'); import type { Readable } from 'stream'; +import { setTimeout } from 'timers'; import type { Document, Long, Timestamp } from './bson'; import { Collection } from './collection'; diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index 881105548e..4140cad637 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -1,3 +1,5 @@ +import { setTimeout } from 'timers'; + import { BSONSerializeOptions, Document, Long, ObjectId, pluckBSONSerializeOptions } from '../bson'; import { CLOSE, diff --git a/src/cmap/connection_pool.ts b/src/cmap/connection_pool.ts index 6ef29184c2..e8888ccc54 100644 --- a/src/cmap/connection_pool.ts +++ b/src/cmap/connection_pool.ts @@ -1,4 +1,6 @@ import Denque = require('denque'); +import { setTimeout } from 'timers'; + import type { ObjectId } from '../bson'; import { APM_EVENTS, diff --git a/src/sdam/monitor.ts b/src/sdam/monitor.ts index 1cab4b5d16..cd1bc092f4 100644 --- a/src/sdam/monitor.ts +++ b/src/sdam/monitor.ts @@ -1,3 +1,5 @@ +import { setTimeout } from 'timers'; + import { Document, Long } from '../bson'; import { connect } from '../cmap/connect'; import { Connection, ConnectionOptions } from '../cmap/connection'; diff --git a/src/sdam/srv_polling.ts b/src/sdam/srv_polling.ts index e867f4f932..71ee20a477 100644 --- a/src/sdam/srv_polling.ts +++ b/src/sdam/srv_polling.ts @@ -1,4 +1,5 @@ import * as dns from 'dns'; +import { setTimeout } from 'timers'; import { MongoRuntimeError } from '../error'; import { Logger, LoggerOptions } from '../logger'; diff --git a/src/sdam/topology.ts b/src/sdam/topology.ts index edda138bd4..c489385c2d 100644 --- a/src/sdam/topology.ts +++ b/src/sdam/topology.ts @@ -1,4 +1,6 @@ import Denque = require('denque'); +import { setTimeout } from 'timers'; + import type { BSONSerializeOptions, Document } from '../bson'; import { deserialize, serialize } from '../bson'; import type { MongoCredentials } from '../cmap/auth/mongo_credentials'; diff --git a/src/utils.ts b/src/utils.ts index 9268b13562..a21ef49c90 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -1,6 +1,7 @@ import * as crypto from 'crypto'; import type { SrvRecord } from 'dns'; import * as os from 'os'; +import { setTimeout } from 'timers'; import { URL } from 'url'; import { Document, ObjectId, resolveBSONOptions } from './bson'; diff --git a/test/integration/change-streams/change_stream.test.ts b/test/integration/change-streams/change_stream.test.ts index 8e6462dc09..7e588843c1 100644 --- a/test/integration/change-streams/change_stream.test.ts +++ b/test/integration/change-streams/change_stream.test.ts @@ -2,6 +2,7 @@ import { strict as assert } from 'assert'; import { expect } from 'chai'; import { on, once } from 'events'; import { PassThrough } from 'stream'; +import { setTimeout } from 'timers'; import { promisify } from 'util'; import { diff --git a/test/integration/change-streams/change_streams.prose.test.ts b/test/integration/change-streams/change_streams.prose.test.ts index 4bb72fcd9c..45c75fb86d 100644 --- a/test/integration/change-streams/change_streams.prose.test.ts +++ b/test/integration/change-streams/change_streams.prose.test.ts @@ -1,5 +1,6 @@ import { expect } from 'chai'; import * as sinon from 'sinon'; +import { setTimeout } from 'timers'; import { ChangeStream, diff --git a/test/integration/crud/find.test.js b/test/integration/crud/find.test.js index 7b68434149..3e742fe40b 100644 --- a/test/integration/crud/find.test.js +++ b/test/integration/crud/find.test.js @@ -2,6 +2,7 @@ const { assert: test, setupDatabase, withMonitoredClient } = require('../shared'); const { expect } = require('chai'); const sinon = require('sinon'); +const { setTimeout } = require('timers'); const { Code, ObjectId, Long, Binary, ReturnDocument } = require('../../../src'); describe('Find', function () { diff --git a/test/integration/crud/misc_cursors.test.js b/test/integration/crud/misc_cursors.test.js index b866d75499..9e7b09058f 100644 --- a/test/integration/crud/misc_cursors.test.js +++ b/test/integration/crud/misc_cursors.test.js @@ -14,6 +14,7 @@ const { expect } = require('chai'); const BSON = require('bson'); const sinon = require('sinon'); const { Writable } = require('stream'); +const { setTimeout } = require('timers'); const { ReadPreference } = require('../../../src/read_preference'); const { ServerType } = require('../../../src/sdam/common'); const { formatSort } = require('../../../src/sort'); diff --git a/test/integration/node-specific/cursor_stream.test.js b/test/integration/node-specific/cursor_stream.test.js index 72cee7edbc..0df94d32e1 100644 --- a/test/integration/node-specific/cursor_stream.test.js +++ b/test/integration/node-specific/cursor_stream.test.js @@ -2,6 +2,7 @@ var expect = require('chai').expect; const { setupDatabase } = require('../shared'); const { Binary } = require('../../../src'); +const { setTimeout, setImmediate } = require('timers'); describe('Cursor Streams', function () { before(function () { diff --git a/test/integration/node-specific/examples/change_streams.test.js b/test/integration/node-specific/examples/change_streams.test.js index aef5ecbf26..ad287ffb0c 100644 --- a/test/integration/node-specific/examples/change_streams.test.js +++ b/test/integration/node-specific/examples/change_streams.test.js @@ -1,6 +1,7 @@ /* eslint no-unused-vars: 0 */ 'use strict'; +const { setTimeout } = require('timers'); const setupDatabase = require('../../shared').setupDatabase; const expect = require('chai').expect; diff --git a/test/integration/node-specific/operation_example.test.js b/test/integration/node-specific/operation_example.test.js index 9b6e9c2dcf..9eb2fc5c49 100644 --- a/test/integration/node-specific/operation_example.test.js +++ b/test/integration/node-specific/operation_example.test.js @@ -1,5 +1,6 @@ 'use strict'; const { assert: test, setupDatabase } = require('../shared'); +const { setTimeout } = require('timers'); const { format: f } = require('util'); const { Topology } = require('../../../src/sdam/topology'); const { Code, ObjectId, ReturnDocument } = require('../../../src'); diff --git a/test/integration/objectid.test.js b/test/integration/objectid.test.js index 2c1ec2e5e4..5c461f6ab1 100644 --- a/test/integration/objectid.test.js +++ b/test/integration/objectid.test.js @@ -3,6 +3,7 @@ var test = require('./shared').assert; const { expect } = require('chai'); var setupDatabase = require('./shared').setupDatabase; const { ObjectId } = require('../../src'); +const { setTimeout, setInterval } = require('timers'); describe('ObjectId', function () { before(function () { diff --git a/test/integration/server-selection/server_selection.prose.operation_count.test.ts b/test/integration/server-selection/server_selection.prose.operation_count.test.ts index 11de8eeab2..39bb7d2566 100644 --- a/test/integration/server-selection/server_selection.prose.operation_count.test.ts +++ b/test/integration/server-selection/server_selection.prose.operation_count.test.ts @@ -1,4 +1,5 @@ import { expect } from 'chai'; +import { setTimeout } from 'timers'; import { promisify } from 'util'; import { CommandStartedEvent } from '../../../src'; diff --git a/test/integration/shared.js b/test/integration/shared.js index 78d0ccaa6a..4e6d741260 100644 --- a/test/integration/shared.js +++ b/test/integration/shared.js @@ -1,6 +1,7 @@ 'use strict'; const expect = require('chai').expect; +const { setTimeout } = require('timers'); // helpers for using chai.expect in the assert style const assert = { diff --git a/test/tools/mongodb-mock/src/server.js b/test/tools/mongodb-mock/src/server.js index 62dda123e2..8cf74183b0 100644 --- a/test/tools/mongodb-mock/src/server.js +++ b/test/tools/mongodb-mock/src/server.js @@ -8,6 +8,7 @@ const compressorIDs = require('./utils').compressorIDs; const Request = require('./request'); const { Query } = require('./protocol'); const EventEmitter = require('events'); +const { setTimeout } = require('timers'); const { HostAddress } = require('../../../../src/utils'); /* diff --git a/test/tools/spec-runner/context.js b/test/tools/spec-runner/context.js index c5ba154268..273cceba5f 100644 --- a/test/tools/spec-runner/context.js +++ b/test/tools/spec-runner/context.js @@ -1,5 +1,6 @@ 'use strict'; const { expect } = require('chai'); +const { setTimeout } = require('timers'); const { resolveConnectionString } = require('./utils'); const { ns } = require('../../../src/utils'); const { extractAuthFromConnectionString } = require('../utils'); diff --git a/test/tools/utils.ts b/test/tools/utils.ts index 95b61773d3..04b082a64c 100644 --- a/test/tools/utils.ts +++ b/test/tools/utils.ts @@ -1,6 +1,7 @@ import { EJSON } from 'bson'; import * as BSON from 'bson'; import { expect } from 'chai'; +import { setTimeout } from 'timers'; import { inspect, promisify } from 'util'; import { OP_MSG } from '../../src/cmap/wire_protocol/constants'; 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 317f071e0f..4c7f59b920 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 @@ -10,6 +10,7 @@ import { HostAddress, isHello } from '../../../src/utils'; import * as mock from '../../tools/mongodb-mock/index'; import type { MockServer } from '../../tools/mongodb-mock/src/server'; import { processTick } from '../../tools/utils'; +import { createTimerSandbox } from '../timer_sandbox'; /* The SRV Prose Tests make use of the following REAL DNS records. @@ -215,17 +216,20 @@ describe('Polling Srv Records for Mongos Discovery', () => { let lookupStub: sinon.SinonStub; let client: MongoClient; let clock: sinon.SinonFakeTimers; + let timerSandbox: sinon.SinonSandbox; const initialRecords = Object.freeze([ { name: 'localhost.test.mock.test.build.10gen.cc', port: 2017 }, { name: 'localhost.test.mock.test.build.10gen.cc', port: 2018 } ]); beforeEach(() => { + timerSandbox = createTimerSandbox(); clock = sinon.useFakeTimers(); }); afterEach(() => { if (clock) { + timerSandbox.restore(); clock.restore(); clock = undefined; } diff --git a/test/unit/cmap/connect.test.js b/test/unit/cmap/connect.test.js index 88e1911330..c4fab156c6 100644 --- a/test/unit/cmap/connect.test.js +++ b/test/unit/cmap/connect.test.js @@ -3,6 +3,7 @@ const mock = require('../../tools/mongodb-mock/index'); const { expect } = require('chai'); const EventEmitter = require('events'); +const { setTimeout } = require('timers'); const { connect } = require('../../../src/cmap/connect'); const { MongoCredentials } = require('../../../src/cmap/auth/mongo_credentials'); diff --git a/test/unit/cmap/connection.test.ts b/test/unit/cmap/connection.test.ts index 952b138349..b1b25db28d 100644 --- a/test/unit/cmap/connection.test.ts +++ b/test/unit/cmap/connection.test.ts @@ -2,6 +2,7 @@ import { expect } from 'chai'; import { EventEmitter } from 'events'; import { Socket } from 'net'; import * as sinon from 'sinon'; +import { setTimeout } from 'timers'; import { connect } from '../../../src/cmap/connect'; import { Connection, hasSessionSupport } from '../../../src/cmap/connection'; @@ -10,6 +11,7 @@ import { MongoNetworkTimeoutError } from '../../../src/error'; import { isHello, ns } from '../../../src/utils'; import * as mock from '../../tools/mongodb-mock/index'; import { getSymbolFrom } from '../../tools/utils'; +import { createTimerSandbox } from '../timer_sandbox'; const connectionOptionsDefaults = { id: 0, @@ -138,6 +140,7 @@ describe('new Connection()', function () { describe('onTimeout()', () => { let connection: sinon.SinonSpiedInstance; let clock: sinon.SinonFakeTimers; + let timerSandbox: sinon.SinonFakeTimers; let driverSocket: sinon.SinonSpiedInstance; let messageStream: MessageStream; let kDelayedTimeoutId: symbol; @@ -163,6 +166,7 @@ describe('new Connection()', function () { } beforeEach(() => { + timerSandbox = createTimerSandbox(); clock = sinon.useFakeTimers(); NodeJSTimeoutClass = setTimeout(() => null, 1).constructor; @@ -176,6 +180,7 @@ describe('new Connection()', function () { }); afterEach(() => { + timerSandbox.restore(); clock.restore(); }); diff --git a/test/unit/cmap/connection_pool.test.js b/test/unit/cmap/connection_pool.test.js index 38e1f9c0e7..fdee3bde66 100644 --- a/test/unit/cmap/connection_pool.test.js +++ b/test/unit/cmap/connection_pool.test.js @@ -6,6 +6,7 @@ const mock = require('../../tools/mongodb-mock/index'); const cmapEvents = require('../../../src/cmap/connection_pool_events'); const sinon = require('sinon'); const { expect } = require('chai'); +const { setImmediate } = require('timers'); const { ns, isHello } = require('../../../src/utils'); const { LEGACY_HELLO_COMMAND } = require('../../../src/constants'); diff --git a/test/unit/error.test.ts b/test/unit/error.test.ts index bf8746afea..8f5d10fb14 100644 --- a/test/unit/error.test.ts +++ b/test/unit/error.test.ts @@ -1,4 +1,5 @@ import { expect } from 'chai'; +import { setTimeout } from 'timers'; import { PoolClosedError as MongoPoolClosedError, diff --git a/test/unit/sdam/monitor.test.js b/test/unit/sdam/monitor.test.js index a161202dbe..40d5f0a7fe 100644 --- a/test/unit/sdam/monitor.test.js +++ b/test/unit/sdam/monitor.test.js @@ -1,4 +1,5 @@ 'use strict'; +const { setTimeout } = require('timers'); const mock = require('../../tools/mongodb-mock/index'); const { ServerType } = require('../../../src/sdam/common'); const { Topology } = require('../../../src/sdam/topology'); diff --git a/test/unit/sdam/topology.test.js b/test/unit/sdam/topology.test.js index d34ec54b03..d4f0617504 100644 --- a/test/unit/sdam/topology.test.js +++ b/test/unit/sdam/topology.test.js @@ -1,5 +1,6 @@ 'use strict'; +const { setTimeout } = require('timers'); const mock = require('../../tools/mongodb-mock/index'); const { expect } = require('chai'); const sinon = require('sinon'); diff --git a/test/unit/timer_sandbox.js b/test/unit/timer_sandbox.js new file mode 100644 index 0000000000..b8f9387d67 --- /dev/null +++ b/test/unit/timer_sandbox.js @@ -0,0 +1,22 @@ +'use strict'; +const sinon = require('sinon'); + +/** + * sinon.useFakeTimers() only affects global methods, this function + * creates a sinon sandbox that ensures that require('timers') + * also uses the mocked variants. + * + * @returns {sinon.SinonSandbox} + */ +exports.createTimerSandbox = () => { + const timerSandbox = sinon.createSandbox(); + const timers = require('timers'); + for (const method in timers) { + if (method in global) { + timerSandbox.replace(timers, method, (...args) => { + return global[method](...args); + }); + } + } + return timerSandbox; +}; diff --git a/test/unit/utils.test.js b/test/unit/utils.test.js index bccadeab76..edcc7557dc 100644 --- a/test/unit/utils.test.js +++ b/test/unit/utils.test.js @@ -10,6 +10,7 @@ const { expect } = require('chai'); const sinon = require('sinon'); const { MongoRuntimeError } = require('../../src/error'); const { LEGACY_HELLO_COMMAND } = require('../../src/constants'); +const { createTimerSandbox } = require('./timer_sandbox'); describe('driver utils', function () { context('eachAsync()', function () { @@ -44,9 +45,10 @@ describe('driver utils', function () { }); describe('#makeInterruptibleAsyncInterval', function () { - let clock, executor, fnSpy; + let timerSandbox, clock, executor, fnSpy; beforeEach(function () { + timerSandbox = createTimerSandbox(); clock = sinon.useFakeTimers(); fnSpy = sinon.spy(cb => { cb(); @@ -58,6 +60,7 @@ describe('driver utils', function () { executor.stop(); } clock.restore(); + timerSandbox.restore(); }); context('when the immediate option is provided', function () {