From 89b77edbdd89d40c8a908a3ae4d615f5e332fe9c Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 6 Nov 2020 06:21:54 -0500 Subject: [PATCH] fix: Change socket timeout default to 0 (#2572) We make an implicit assumption with our default socket timeout that most operations will complete within the current default of 5min. This is not the case for services like Atlas Data Lake, which may regularly take far longer to complete operations. In general we can't make any assumptions about how long an operation will take to complete, and so the default socket timeout is now 0 (infinity) unless otherwise indicated. 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/collection.test.js | 2 +- test/functional/mongo_client.test.js | 7 ++-- test/functional/mongo_client_options.test.js | 20 ++++++++++-- test/functional/operation_example.test.js | 16 ++++++---- .../operation_generators_example.test.js | 1 + .../operation_promises_example.test.js | 32 +++++++++++-------- test/functional/replset_connection.test.js | 3 +- 17 files changed, 70 insertions(+), 44 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/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.test.js b/test/functional/mongo_client.test.js index f42193f4c0..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(); @@ -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 } ); diff --git a/test/functional/mongo_client_options.test.js b/test/functional/mongo_client_options.test.js index efdf2c6c40..9eb5ff59a6 100644 --- a/test/functional/mongo_client_options.test.js +++ b/test/functional/mongo_client_options.test.js @@ -1,7 +1,7 @@ '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; describe('MongoClient Options', function() { before(function() { @@ -96,6 +96,20 @@ describe('MongoClient Options', function() { } }); + it('should default socketTimeout to infinity', function(done) { + const client = this.configuration.newClient(); + client.connect(() => { + expect(client.s.options.socketTimeoutMS).to.deep.equal(0); + 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); + }); + }); + /** * @ignore */ diff --git a/test/functional/operation_example.test.js b/test/functional/operation_example.test.js index e874267a84..599c0f687f 100644 --- a/test/functional/operation_example.test.js +++ b/test/functional/operation_example.test.js @@ -3936,15 +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); - 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 diff --git a/test/functional/operation_generators_example.test.js b/test/functional/operation_generators_example.test.js index f3d9637715..1125da9db9 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 f88d9d2b01..b73d8c0680 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/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 },