Skip to content

Commit

Permalink
fix(NODE-4281): ensure that the driver always uses Node.js timers (#3275
Browse files Browse the repository at this point in the history
)

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.
  • Loading branch information
addaleax authored Jun 1, 2022
1 parent 47adfb3 commit 4501a1c
Show file tree
Hide file tree
Showing 30 changed files with 79 additions and 1 deletion.
15 changes: 15 additions & 0 deletions .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
1 change: 1 addition & 0 deletions src/change_stream.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down
2 changes: 2 additions & 0 deletions src/cmap/connection.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { setTimeout } from 'timers';

import { BSONSerializeOptions, Document, Long, ObjectId, pluckBSONSerializeOptions } from '../bson';
import {
CLOSE,
Expand Down
2 changes: 2 additions & 0 deletions src/cmap/connection_pool.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import Denque = require('denque');
import { setTimeout } from 'timers';

import type { ObjectId } from '../bson';
import {
APM_EVENTS,
Expand Down
2 changes: 2 additions & 0 deletions src/sdam/monitor.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { setTimeout } from 'timers';

import { Document, Long } from '../bson';
import { connect } from '../cmap/connect';
import { Connection, ConnectionOptions } from '../cmap/connection';
Expand Down
1 change: 1 addition & 0 deletions src/sdam/srv_polling.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as dns from 'dns';
import { setTimeout } from 'timers';

import { MongoRuntimeError } from '../error';
import { Logger, LoggerOptions } from '../logger';
Expand Down
2 changes: 2 additions & 0 deletions src/sdam/topology.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down
1 change: 1 addition & 0 deletions src/utils.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down
1 change: 1 addition & 0 deletions test/integration/change-streams/change_stream.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { expect } from 'chai';
import * as sinon from 'sinon';
import { setTimeout } from 'timers';

import {
ChangeStream,
Expand Down
1 change: 1 addition & 0 deletions test/integration/crud/find.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down
1 change: 1 addition & 0 deletions test/integration/crud/misc_cursors.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
1 change: 1 addition & 0 deletions test/integration/node-specific/cursor_stream.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down
Original file line number Diff line number Diff line change
@@ -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;

Expand Down
1 change: 1 addition & 0 deletions test/integration/node-specific/operation_example.test.js
Original file line number Diff line number Diff line change
@@ -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');
Expand Down
1 change: 1 addition & 0 deletions test/integration/objectid.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { expect } from 'chai';
import { setTimeout } from 'timers';
import { promisify } from 'util';

import { CommandStartedEvent } from '../../../src';
Expand Down
1 change: 1 addition & 0 deletions test/integration/shared.js
Original file line number Diff line number Diff line change
@@ -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 = {
Expand Down
1 change: 1 addition & 0 deletions test/tools/mongodb-mock/src/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');

/*
Expand Down
1 change: 1 addition & 0 deletions test/tools/spec-runner/context.js
Original file line number Diff line number Diff line change
@@ -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');
Expand Down
1 change: 1 addition & 0 deletions test/tools/utils.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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;
}
Expand Down
1 change: 1 addition & 0 deletions test/unit/cmap/connect.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
5 changes: 5 additions & 0 deletions test/unit/cmap/connection.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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,
Expand Down Expand Up @@ -138,6 +140,7 @@ describe('new Connection()', function () {
describe('onTimeout()', () => {
let connection: sinon.SinonSpiedInstance<Connection>;
let clock: sinon.SinonFakeTimers;
let timerSandbox: sinon.SinonFakeTimers;
let driverSocket: sinon.SinonSpiedInstance<FakeSocket>;
let messageStream: MessageStream;
let kDelayedTimeoutId: symbol;
Expand All @@ -163,6 +166,7 @@ describe('new Connection()', function () {
}

beforeEach(() => {
timerSandbox = createTimerSandbox();
clock = sinon.useFakeTimers();

NodeJSTimeoutClass = setTimeout(() => null, 1).constructor;
Expand All @@ -176,6 +180,7 @@ describe('new Connection()', function () {
});

afterEach(() => {
timerSandbox.restore();
clock.restore();
});

Expand Down
1 change: 1 addition & 0 deletions test/unit/cmap/connection_pool.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Expand Down
1 change: 1 addition & 0 deletions test/unit/error.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { expect } from 'chai';
import { setTimeout } from 'timers';

import {
PoolClosedError as MongoPoolClosedError,
Expand Down
1 change: 1 addition & 0 deletions test/unit/sdam/monitor.test.js
Original file line number Diff line number Diff line change
@@ -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');
Expand Down
1 change: 1 addition & 0 deletions test/unit/sdam/topology.test.js
Original file line number Diff line number Diff line change
@@ -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');
Expand Down
22 changes: 22 additions & 0 deletions test/unit/timer_sandbox.js
Original file line number Diff line number Diff line change
@@ -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;
};
5 changes: 4 additions & 1 deletion test/unit/utils.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down Expand Up @@ -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();
Expand All @@ -58,6 +60,7 @@ describe('driver utils', function () {
executor.stop();
}
clock.restore();
timerSandbox.restore();
});

context('when the immediate option is provided', function () {
Expand Down

0 comments on commit 4501a1c

Please sign in to comment.