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: Change socket timeout default to 0 [PORT] #2572

Merged
merged 8 commits into from Nov 6, 2020
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
2 changes: 1 addition & 1 deletion lib/cmap/connection.js
Expand Up @@ -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 =
Expand Down
2 changes: 1 addition & 1 deletion lib/core/connection/connect.js
Expand Up @@ -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;

Expand Down
7 changes: 3 additions & 4 deletions lib/core/connection/connection.js
Expand Up @@ -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.
Expand All @@ -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;
Expand Down Expand Up @@ -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,
Expand Down
6 changes: 3 additions & 3 deletions lib/core/connection/pool.js
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion lib/core/topologies/replset.js
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion lib/core/topologies/server.js
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions lib/mongo_client.js
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions lib/operations/connect.js
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion lib/topologies/mongos.js
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion lib/topologies/server.js
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion test/functional/collection.test.js
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

what changed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test fails if you run it in isolation, it depends upon other tests to create multiple collections when it only creates one itself.

let found = false;

documents.forEach(document => {
Expand Down
7 changes: 5 additions & 2 deletions test/functional/mongo_client.test.js
Expand Up @@ -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();
Expand Down Expand Up @@ -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
}
);

Expand Down
20 changes: 17 additions & 3 deletions 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() {
Expand Down Expand Up @@ -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
*/
Expand Down
16 changes: 9 additions & 7 deletions test/functional/operation_example.test.js
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions test/functional/operation_generators_example.test.js
Expand Up @@ -2931,6 +2931,7 @@ describe('Operation (Generators)', function() {
.newClient(configuration.writeConcernMax(), { poolSize: 1 })
.connect();
var db = client.db(configuration.db);
yield db.createCollection('example');
mbroadst marked this conversation as resolved.
Show resolved Hide resolved
// LINE var MongoClient = require('mongodb').MongoClient,
// LINE co = require('co');
// LINE test = require('assert');
Expand Down
32 changes: 19 additions & 13 deletions test/functional/operation_promises_example.test.js
Expand Up @@ -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
mbroadst marked this conversation as resolved.
Show resolved Hide resolved
.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
}
});
Expand Down
3 changes: 2 additions & 1 deletion 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) {
Expand Down Expand Up @@ -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 },
Expand Down