Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(NODE-4281): ensure that the driver always uses Node.js timers #3275

Merged
merged 2 commits into from
Jun 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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');

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with this approach but could another approach be only linting for global setTimeout/Immediate/Intervals in the src directory? As long as Compass isn't running our tests I think that should be okay.

I don't have a strong preference, but the benefit of only linting the src directory is one fewer custom test configurations to maintain. @addaleax

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@baileympearson I’d also be okay with applying the linting only there, I guess what would mean a new .eslintrc file inside src? That should be easy enough.

That being said, this particular file would still be necessary, because it’s about how sinon mocks the timers functions (which are then picked up by files in the src/ directory)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right. That makes sense.

If we need this file either way, I'd rather lint everything for consistency 😃

/**
* 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