From 9b3da782c1577025f4989544fd4aa53e8a645bed Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 9 Oct 2020 13:13:17 -0400 Subject: [PATCH 1/8] fix: :fire: Change socket timeout default to 0 Socket timeout by default is infinity. NODE-2835 --- lib/cmap/connection.js | 2 +- lib/core/connection/connect.js | 2 +- lib/core/connection/connection.js | 7 +++---- lib/core/connection/pool.js | 6 +++--- lib/core/topologies/replset.js | 2 +- lib/core/topologies/server.js | 2 +- lib/mongo_client.js | 4 ++-- lib/operations/connect.js | 4 ++-- lib/topologies/mongos.js | 2 +- lib/topologies/server.js | 2 +- test/functional/mongo_client.test.js | 5 ++++- 11 files changed, 20 insertions(+), 18 deletions(-) diff --git a/lib/cmap/connection.js b/lib/cmap/connection.js index e30e202dce..bf715625b2 100644 --- a/lib/cmap/connection.js +++ b/lib/cmap/connection.js @@ -32,7 +32,7 @@ class Connection extends EventEmitter { this.id = options.id; this.address = streamIdentifier(stream); this.bson = options.bson; - this.socketTimeout = typeof options.socketTimeout === 'number' ? options.socketTimeout : 360000; + this.socketTimeout = typeof options.socketTimeout === 'number' ? options.socketTimeout : 0; this.host = options.host || 'localhost'; this.port = options.port || 27017; this.monitorCommands = diff --git a/lib/core/connection/connect.js b/lib/core/connection/connect.js index f8598e4a0a..05125a3d4e 100644 --- a/lib/core/connection/connect.js +++ b/lib/core/connection/connect.js @@ -263,7 +263,7 @@ function makeConnection(family, options, cancellationToken, _callback) { : typeof options.connectTimeoutMS === 'number' ? options.connectTimeoutMS : 30000; - const socketTimeout = typeof options.socketTimeout === 'number' ? options.socketTimeout : 360000; + const socketTimeout = typeof options.socketTimeout === 'number' ? options.socketTimeout : 0; const rejectUnauthorized = typeof options.rejectUnauthorized === 'boolean' ? options.rejectUnauthorized : true; diff --git a/lib/core/connection/connection.js b/lib/core/connection/connection.js index cf34c890db..5b22513b2a 100644 --- a/lib/core/connection/connection.js +++ b/lib/core/connection/connection.js @@ -69,7 +69,7 @@ class Connection extends EventEmitter { * @param {boolean} [options.keepAlive=true] TCP Connection keep alive enabled * @param {number} [options.keepAliveInitialDelay=120000] Initial delay before TCP keep alive enabled * @param {number} [options.connectionTimeout=30000] TCP Connection timeout setting - * @param {number} [options.socketTimeout=360000] TCP Socket timeout setting + * @param {number} [options.socketTimeout=0] TCP Socket timeout setting * @param {boolean} [options.promoteLongs] Convert Long values from the db into Numbers if they fit into 53 bits * @param {boolean} [options.promoteValues] Promotes BSON values to native types where possible, set to false to only receive wrapper types. * @param {boolean} [options.promoteBuffers] Promotes Binary BSON values to native Node Buffers. @@ -92,7 +92,7 @@ class Connection extends EventEmitter { this.port = options.port || 27017; this.host = options.host || 'localhost'; - this.socketTimeout = typeof options.socketTimeout === 'number' ? options.socketTimeout : 360000; + this.socketTimeout = typeof options.socketTimeout === 'number' ? options.socketTimeout : 0; // These values are inspected directly in tests, but maybe not necessary to keep around this.keepAlive = typeof options.keepAlive === 'boolean' ? options.keepAlive : true; @@ -316,8 +316,7 @@ class Connection extends EventEmitter { if (typeof options === 'function') (callback = options), (options = {}); const conn = this; - const socketTimeout = - typeof options.socketTimeout === 'number' ? options.socketTimeout : 360000; + const socketTimeout = typeof options.socketTimeout === 'number' ? options.socketTimeout : 0; const bson = conn.options.bson; const query = new Query(bson, ns, command, { numberToSkip: 0, diff --git a/lib/core/connection/pool.js b/lib/core/connection/pool.js index f4e908ce07..f0061ee914 100644 --- a/lib/core/connection/pool.js +++ b/lib/core/connection/pool.js @@ -63,8 +63,8 @@ var _id = 0; * @param {number} [options.keepAliveInitialDelay=120000] Initial delay before TCP keep alive enabled * @param {boolean} [options.noDelay=true] TCP Connection no delay * @param {number} [options.connectionTimeout=30000] TCP Connection timeout setting - * @param {number} [options.socketTimeout=360000] TCP Socket timeout setting - * @param {number} [options.monitoringSocketTimeout=30000] TCP Socket timeout setting for replicaset monitoring socket + * @param {number} [options.socketTimeout=0] TCP Socket timeout setting + * @param {number} [options.monitoringSocketTimeout=0] TCP Socket timeout setting for replicaset monitoring socket * @param {boolean} [options.ssl=false] Use SSL for connection * @param {boolean|function} [options.checkServerIdentity=true] Ensure we check server identify during SSL, set to false to disable checking. Only works for Node 0.12.x or higher. You can pass in a boolean or your own checkServerIdentity override function. * @param {Buffer} [options.ca] SSL Certificate store binary buffer @@ -111,7 +111,7 @@ var Pool = function(topology, options) { minSize: 0, // socket settings connectionTimeout: 30000, - socketTimeout: 360000, + socketTimeout: 0, keepAlive: true, keepAliveInitialDelay: 120000, noDelay: true, diff --git a/lib/core/topologies/replset.js b/lib/core/topologies/replset.js index cf276df0cb..deaa60fa7a 100644 --- a/lib/core/topologies/replset.js +++ b/lib/core/topologies/replset.js @@ -919,7 +919,7 @@ ReplSet.prototype.connect = function(options) { ); }); - // Error out as high availbility interval must be < than socketTimeout + // Error out as high availability interval must be < than socketTimeout if ( this.s.options.socketTimeout > 0 && this.s.options.socketTimeout <= this.s.options.haInterval diff --git a/lib/core/topologies/server.js b/lib/core/topologies/server.js index d0ade6bace..ecf46135ca 100644 --- a/lib/core/topologies/server.js +++ b/lib/core/topologies/server.js @@ -75,7 +75,7 @@ function topologyId(server) { * @param {number} [options.keepAliveInitialDelay=120000] Initial delay before TCP keep alive enabled * @param {boolean} [options.noDelay=true] TCP Connection no delay * @param {number} [options.connectionTimeout=30000] TCP Connection timeout setting - * @param {number} [options.socketTimeout=360000] TCP Socket timeout setting + * @param {number} [options.socketTimeout=0] TCP Socket timeout setting * @param {boolean} [options.ssl=false] Use SSL for connection * @param {boolean|function} [options.checkServerIdentity=true] Ensure we check server identify during SSL, set to false to disable checking. Only works for Node 0.12.x or higher. You can pass in a boolean or your own checkServerIdentity override function. * @param {Buffer} [options.ca] SSL Certificate store binary buffer diff --git a/lib/mongo_client.js b/lib/mongo_client.js index c74c547e4b..8195890773 100644 --- a/lib/mongo_client.js +++ b/lib/mongo_client.js @@ -97,7 +97,7 @@ const validOptions = require('./operations/connect').validOptions; * @param {boolean} [options.keepAlive=true] TCP Connection keep alive enabled * @param {number} [options.keepAliveInitialDelay=120000] The number of milliseconds to wait before initiating keepAlive on the TCP socket * @param {number} [options.connectTimeoutMS=10000] How long to wait for a connection to be established before timing out - * @param {number} [options.socketTimeoutMS=360000] How long a send or receive on a socket can take before timing out + * @param {number} [options.socketTimeoutMS=0] How long a send or receive on a socket can take before timing out * @param {number} [options.family] Version of IP stack. Can be 4, 6 or null (default). * If null, will attempt to connect with IPv6, and will fall back to IPv4 on failure * @param {number} [options.reconnectTries=30] Server attempt to reconnect #times @@ -372,7 +372,7 @@ MongoClient.prototype.isConnected = function(options) { * @param {boolean} [options.keepAlive=true] TCP Connection keep alive enabled * @param {number} [options.keepAliveInitialDelay=120000] The number of milliseconds to wait before initiating keepAlive on the TCP socket * @param {number} [options.connectTimeoutMS=10000] How long to wait for a connection to be established before timing out - * @param {number} [options.socketTimeoutMS=360000] How long a send or receive on a socket can take before timing out + * @param {number} [options.socketTimeoutMS=0] How long a send or receive on a socket can take before timing out * @param {number} [options.family] Version of IP stack. Can be 4, 6 or null (default). * If null, will attempt to connect with IPv6, and will fall back to IPv4 on failure * @param {number} [options.reconnectTries=30] Server attempt to reconnect #times diff --git a/lib/operations/connect.js b/lib/operations/connect.js index e307b459b9..2c3e8bd7a4 100644 --- a/lib/operations/connect.js +++ b/lib/operations/connect.js @@ -290,7 +290,7 @@ function connect(mongoClient, url, options, callback) { const _finalOptions = createUnifiedOptions(object, options); // Check if we have connection and socket timeout set - if (_finalOptions.socketTimeoutMS == null) _finalOptions.socketTimeoutMS = 360000; + if (_finalOptions.socketTimeoutMS == null) _finalOptions.socketTimeoutMS = 0; if (_finalOptions.connectTimeoutMS == null) _finalOptions.connectTimeoutMS = 10000; if (_finalOptions.retryWrites == null) _finalOptions.retryWrites = true; if (_finalOptions.useRecoveryToken == null) _finalOptions.useRecoveryToken = true; @@ -788,7 +788,7 @@ function translateOptions(options, translationOptions) { } // Set the socket and connection timeouts - if (options.socketTimeoutMS == null) options.socketTimeoutMS = 360000; + if (options.socketTimeoutMS == null) options.socketTimeoutMS = 0; if (options.connectTimeoutMS == null) options.connectTimeoutMS = 10000; if (!translationOptions.createServers) { diff --git a/lib/topologies/mongos.js b/lib/topologies/mongos.js index 26b24a5381..bf30d20ebe 100644 --- a/lib/topologies/mongos.js +++ b/lib/topologies/mongos.js @@ -84,7 +84,7 @@ var legalOptionNames = [ * @param {boolean} [options.socketOptions.keepAlive=true] TCP Connection keep alive enabled * @param {number} [options.socketOptions.keepAliveInitialDelay=120000] The number of milliseconds to wait before initiating keepAlive on the TCP socket * @param {number} [options.socketOptions.connectTimeoutMS=10000] How long to wait for a connection to be established before timing out - * @param {number} [options.socketOptions.socketTimeoutMS=360000] How long a send or receive on a socket can take before timing out + * @param {number} [options.socketOptions.socketTimeoutMS=0] How long a send or receive on a socket can take before timing out * @param {boolean} [options.domainsEnabled=false] Enable the wrapping of the callback in the current domain, disabled by default to avoid perf hit. * @param {boolean} [options.monitorCommands=false] Enable command monitoring for this topology * @fires Mongos#connect diff --git a/lib/topologies/server.js b/lib/topologies/server.js index 0bd85ed206..0abaad3dba 100644 --- a/lib/topologies/server.js +++ b/lib/topologies/server.js @@ -86,7 +86,7 @@ var legalOptionNames = [ * @param {boolean} [options.socketOptions.keepAlive=true] TCP Connection keep alive enabled * @param {number} [options.socketOptions.keepAliveInitialDelay=120000] The number of milliseconds to wait before initiating keepAlive on the TCP socket * @param {number} [options.socketOptions.connectTimeoutMS=10000] How long to wait for a connection to be established before timing out - * @param {number} [options.socketOptions.socketTimeoutMS=360000] How long a send or receive on a socket can take before timing out + * @param {number} [options.socketOptions.socketTimeoutMS=0] How long a send or receive on a socket can take before timing out * @param {number} [options.reconnectTries=30] Server attempt to reconnect #times * @param {number} [options.reconnectInterval=1000] Server will wait # milliseconds between retries * @param {boolean} [options.monitoring=true] Triggers the server instance to call ismaster diff --git a/test/functional/mongo_client.test.js b/test/functional/mongo_client.test.js index f42193f4c0..758aeec850 100644 --- a/test/functional/mongo_client.test.js +++ b/test/functional/mongo_client.test.js @@ -518,7 +518,10 @@ describe('MongoClient', function() { {}, { keepAlive: true, - keepAliveInitialDelay: 100 + keepAliveInitialDelay: 100, + // keepAliveInitialDelay is clamped to half the size of socketTimeout + // if socketTimeout is less than keepAliveInitialDelay + socketTimeout: 101 } ); From 98f46e35b97b30569f44e44713b7d415df666589 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 12 Oct 2020 11:45:54 -0400 Subject: [PATCH 2/8] test: :sparkles: Add test to ensure default socketTimeout is 0 --- test/functional/mongo_client_options.test.js | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/test/functional/mongo_client_options.test.js b/test/functional/mongo_client_options.test.js index efdf2c6c40..6b81c52a23 100644 --- a/test/functional/mongo_client_options.test.js +++ b/test/functional/mongo_client_options.test.js @@ -1,7 +1,8 @@ 'use strict'; -const test = require('./shared').assert, - setupDatabase = require('./shared').setupDatabase, - expect = require('chai').expect; +const test = require('./shared').assert; +const setupDatabase = require('./shared').setupDatabase; +const expect = require('chai').expect; +const MongoClient = require('../../lib/mongo_client'); describe('MongoClient Options', function() { before(function() { @@ -96,6 +97,17 @@ describe('MongoClient Options', function() { } }); + it('should default socketTimeout to infinity', function(done) { + const client = new MongoClient(this.configuration.url()); + client.connect(() => { + expect(client.s.options.socketTimeoutMS).to.deep.equal(0); + for (const connection of client.topology.s.coreTopology.connections()) { + expect(connection.socketTimeout).to.deep.equal(0); + } + done(); + }); + }); + /** * @ignore */ From 37862cc44fd0ff6ddbd8f6d0d691473306e7772a Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 14 Oct 2020 10:24:08 -0400 Subject: [PATCH 3/8] fix: :fire: fix tests expecting 6min timeout --- test/functional/mongo_client.test.js | 2 +- test/functional/replset_connection.test.js | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/test/functional/mongo_client.test.js b/test/functional/mongo_client.test.js index 758aeec850..3943a04bcb 100644 --- a/test/functional/mongo_client.test.js +++ b/test/functional/mongo_client.test.js @@ -368,7 +368,7 @@ describe('MongoClient', function() { for (var i = 0; i < connections.length; i++) { test.equal(10000, connections[i].connectionTimeout); - test.equal(360000, connections[i].socketTimeout); + expect(connections[i].socketTimeout).to.equal(0); } client.close(); diff --git a/test/functional/replset_connection.test.js b/test/functional/replset_connection.test.js index 6950b2a9cd..6b8c7d7930 100644 --- a/test/functional/replset_connection.test.js +++ b/test/functional/replset_connection.test.js @@ -1,6 +1,7 @@ 'use strict'; var f = require('util').format; var test = require('./shared').assert; +const expect = require('chai').expect; var setupDatabase = require('./shared').setupDatabase; var restartAndDone = function(configuration, done) { @@ -595,7 +596,7 @@ describe.skip('ReplSet (Connection)', function() { var db = client.db(configuration.db); test.equal(500, client.topology.connections()[0].connectionTimeout); - test.equal(360000, client.topology.connections()[0].socketTimeout); + expect(client.topology.connections()[0].socketTimeout).to.equal(0); db.collection('replicaset_mongo_client_collection').update( { a: 1 }, From 3a39a610f26a319ddf1a5ff3d4322fd26196c237 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 2 Nov 2020 18:11:59 -0500 Subject: [PATCH 4/8] Hanging test fix --- test/functional/collection.test.js | 2 +- test/functional/mongo_client_options.test.js | 2 +- test/functional/operation_example.test.js | 1 + .../operation_generators_example.test.js | 1 + .../operation_promises_example.test.js | 32 +++++++++++-------- test/tools/runner/index.js | 2 +- 6 files changed, 24 insertions(+), 16 deletions(-) diff --git a/test/functional/collection.test.js b/test/functional/collection.test.js index a149757e88..3cfe482cf3 100644 --- a/test/functional/collection.test.js +++ b/test/functional/collection.test.js @@ -799,7 +799,7 @@ describe('Collection', function() { db.listCollections().toArray((err, documents) => { expect(err).to.not.exist; - expect(documents.length > 1).to.be.true; + expect(documents.length >= 1).to.be.true; let found = false; documents.forEach(document => { diff --git a/test/functional/mongo_client_options.test.js b/test/functional/mongo_client_options.test.js index 6b81c52a23..66e0dd3dae 100644 --- a/test/functional/mongo_client_options.test.js +++ b/test/functional/mongo_client_options.test.js @@ -104,7 +104,7 @@ describe('MongoClient Options', function() { for (const connection of client.topology.s.coreTopology.connections()) { expect(connection.socketTimeout).to.deep.equal(0); } - done(); + client.close(done); }); }); diff --git a/test/functional/operation_example.test.js b/test/functional/operation_example.test.js index 86d85fa4f7..80263c7566 100644 --- a/test/functional/operation_example.test.js +++ b/test/functional/operation_example.test.js @@ -3937,6 +3937,7 @@ describe('Operation Examples', function() { // REMOVE-LINE var db = client.db(configuration.db); // BEGIN var db = client.db(configuration.db); + db.createCollection('example'); test.equal(null, err); // Retry to get the collection, should work as it's now created diff --git a/test/functional/operation_generators_example.test.js b/test/functional/operation_generators_example.test.js index 30a144d756..ec4a023a7a 100644 --- a/test/functional/operation_generators_example.test.js +++ b/test/functional/operation_generators_example.test.js @@ -2931,6 +2931,7 @@ describe('Operation (Generators)', function() { .newClient(configuration.writeConcernMax(), { poolSize: 1 }) .connect(); var db = client.db(configuration.db); + yield db.createCollection('example'); // LINE var MongoClient = require('mongodb').MongoClient, // LINE co = require('co'); // LINE test = require('assert'); diff --git a/test/functional/operation_promises_example.test.js b/test/functional/operation_promises_example.test.js index 8c14dc7a94..0d47f4492d 100644 --- a/test/functional/operation_promises_example.test.js +++ b/test/functional/operation_promises_example.test.js @@ -3149,22 +3149,28 @@ describe('Operation (Promises)', function() { auto_reconnect: false }); - return client.connect().then(function(client) { - var db = client.db(configuration.db); - // LINE var MongoClient = require('mongodb').MongoClient, - // LINE test = require('assert'); - // LINE const client = new MongoClient('mongodb://localhost:27017/test'); - // LINE client.connect().then(() => { - // LINE var db = client.db('test); - // REPLACE configuration.writeConcernMax() WITH {w:1} - // REMOVE-LINE done(); - // BEGIN - // Retry to get the collection, should work as it's now created - return db.collections().then(function(collections) { + return client + .connect() + .then(function(client) { + var db = client.db(configuration.db); + return db.createCollection('example'); + }) + .then(() => { + // LINE var MongoClient = require('mongodb').MongoClient, + // LINE test = require('assert'); + // LINE const client = new MongoClient('mongodb://localhost:27017/test'); + // LINE client.connect().then(() => { + // LINE var db = client.db('test); + // REPLACE configuration.writeConcernMax() WITH {w:1} + // REMOVE-LINE done(); + // BEGIN + // Retry to get the collection, should work as it's now created + return client.db(configuration.db).collections(); + }) + .then(function(collections) { test.ok(collections.length > 0); return client.close(); }); - }); // END } }); diff --git a/test/tools/runner/index.js b/test/tools/runner/index.js index 4b23b7e31d..4104765453 100644 --- a/test/tools/runner/index.js +++ b/test/tools/runner/index.js @@ -86,7 +86,7 @@ before(function(_done) { } this.configuration = new TestConfiguration(parsedURI, context); - done(); + client.close(done); }); }); }); From e6eeff5ead1faf23388bf1e124b9d0c34aeca7ba Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 3 Nov 2020 11:40:18 -0500 Subject: [PATCH 5/8] use newClient helper --- test/functional/mongo_client_options.test.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/functional/mongo_client_options.test.js b/test/functional/mongo_client_options.test.js index 66e0dd3dae..f1f1b16696 100644 --- a/test/functional/mongo_client_options.test.js +++ b/test/functional/mongo_client_options.test.js @@ -2,7 +2,6 @@ const test = require('./shared').assert; const setupDatabase = require('./shared').setupDatabase; const expect = require('chai').expect; -const MongoClient = require('../../lib/mongo_client'); describe('MongoClient Options', function() { before(function() { @@ -98,7 +97,7 @@ describe('MongoClient Options', function() { }); it('should default socketTimeout to infinity', function(done) { - const client = new MongoClient(this.configuration.url()); + const client = this.configuration.newClient(); client.connect(() => { expect(client.s.options.socketTimeoutMS).to.deep.equal(0); for (const connection of client.topology.s.coreTopology.connections()) { From 0a6f4be72d074115af4ad4d176695b7ea2361476 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 3 Nov 2020 12:30:33 -0500 Subject: [PATCH 6/8] add callback to test --- test/functional/operation_example.test.js | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/test/functional/operation_example.test.js b/test/functional/operation_example.test.js index 80263c7566..7c80d3c750 100644 --- a/test/functional/operation_example.test.js +++ b/test/functional/operation_example.test.js @@ -3936,16 +3936,17 @@ describe('Operation Examples', function() { // REMOVE-LINE done(); // REMOVE-LINE var db = client.db(configuration.db); // BEGIN + expect(err).to.not.exist; var db = client.db(configuration.db); - db.createCollection('example'); - test.equal(null, err); - - // Retry to get the collection, should work as it's now created - db.collections(function(err, collections) { - test.equal(null, err); - test.ok(collections.length > 0); + db.createCollection('example', err => { + expect(err).to.not.exist; + // Retry to get the collection, should work as it's now created + db.collections(function(err, collections) { + expect(err).to.not.exist; + test.ok(collections.length > 0); - client.close(done); + client.close(done); + }); }); }); // END From 5935a1729dd823846dcb9f29974a35c301c20299 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 3 Nov 2020 12:54:46 -0500 Subject: [PATCH 7/8] nullish check --- test/functional/mongo_client_options.test.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/functional/mongo_client_options.test.js b/test/functional/mongo_client_options.test.js index f1f1b16696..9eb5ff59a6 100644 --- a/test/functional/mongo_client_options.test.js +++ b/test/functional/mongo_client_options.test.js @@ -100,7 +100,10 @@ describe('MongoClient Options', function() { const client = this.configuration.newClient(); client.connect(() => { expect(client.s.options.socketTimeoutMS).to.deep.equal(0); - for (const connection of client.topology.s.coreTopology.connections()) { + const connections = client.topology.s.coreTopology + ? client.topology.s.coreTopology.connections() + : []; + for (const connection of connections) { expect(connection.socketTimeout).to.deep.equal(0); } client.close(done); From a0d94cc2bac695a2af9e7b3371577d94c77f316f Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 5 Nov 2020 13:50:57 -0500 Subject: [PATCH 8/8] Fix done call --- test/tools/runner/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/tools/runner/index.js b/test/tools/runner/index.js index 4104765453..4b23b7e31d 100644 --- a/test/tools/runner/index.js +++ b/test/tools/runner/index.js @@ -86,7 +86,7 @@ before(function(_done) { } this.configuration = new TestConfiguration(parsedURI, context); - client.close(done); + done(); }); }); });