Skip to content

Commit

Permalink
fix(topology): don't early abort server selection on network errors
Browse files Browse the repository at this point in the history
Network errors encountered during server selection are meant to be
stored on the topology description, in order to eventually convey
the reason for failed selection to the user. We were reporting this
error as soon as it was encountered erroneously. Now, we wait until
selection has failed to report the newly added `reason` property of
the `MongoTimeoutError` returned.
  • Loading branch information
mbroadst committed Oct 9, 2019
1 parent dc70c2d commit 2b6a359
Show file tree
Hide file tree
Showing 10 changed files with 60 additions and 23 deletions.
7 changes: 6 additions & 1 deletion lib/core/error.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,12 +83,17 @@ class MongoParseError extends MongoError {
* An error signifying a timeout event
*
* @param {Error|string|object} message The error message
* @param {string|object} [reason] The reason the timeout occured
* @property {string} message The error message
* @property {string} [reason] An optional reason context for the timeout, generally an error saved during flow of monitoring and selecting servers
*/
class MongoTimeoutError extends MongoError {
constructor(message) {
constructor(message, reason) {
super(message);
this.name = 'MongoTimeoutError';
if (reason != null) {
this.reason = reason;
}
}
}

Expand Down
12 changes: 6 additions & 6 deletions lib/core/sdam/topology.js
Original file line number Diff line number Diff line change
Expand Up @@ -836,18 +836,18 @@ function selectServers(topology, selector, timeout, start, callback) {
clearTimeout(iterationTimer);
topology.s.iterationTimers.splice(timerIndex, 1);

if (topology.description.error) {
callback(topology.description.error, null);
return;
}

// topology description has changed due to monitoring, reattempt server selection
selectServers(topology, selector, timeout, start, callback);
};

const iterationTimer = setTimeout(() => {
topology.removeListener('topologyDescriptionChanged', descriptionChangedHandler);
callback(new MongoTimeoutError(`Server selection timed out after ${timeout} ms`));
callback(
new MongoTimeoutError(
`Server selection timed out after ${timeout} ms`,
topology.description.error
)
);
}, timeout - duration);

// track this timer in case we need to clean it up outside this loop
Expand Down
9 changes: 7 additions & 2 deletions test/core/functional/server_tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -993,12 +993,17 @@ describe('Server tests', function() {
});

const config = this.configuration;
var client = config.newTopology(server.address().host, server.address().port);
var client = config.newTopology(server.address().host, server.address().port, {
serverSelectionTimeoutMS: 10
});

client.on('error', error => {
let err;
try {
expect(error).to.be.an.instanceOf(Error);
expect(error.message).to.match(/but this version of the Node.js Driver requires/);

const errorMessage = error.reason ? error.reason.message : error.message;
expect(errorMessage).to.match(/but this version of the Node.js Driver requires/);
} catch (e) {
err = e;
}
Expand Down
4 changes: 3 additions & 1 deletion test/functional/connection_tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -423,7 +423,9 @@ describe('Connection', function() {
// The actual test we wish to run
test: function(done) {
var configuration = this.configuration;
const client = configuration.newClient(configuration.url('slithy', 'toves'));
const client = configuration.newClient(configuration.url('slithy', 'toves'), {
serverSelectionTimeoutMS: 10
});

client.connect(function(err, client) {
expect(err).to.exist;
Expand Down
6 changes: 4 additions & 2 deletions test/functional/db_tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,8 @@ describe('Db', function() {
test: function(done) {
var configuration = this.configuration;
var fs_client = configuration.newClient('mongodb://127.0.0.1:25117/test', {
auto_reconnect: false
auto_reconnect: false,
serverSelectionTimeoutMS: 10
});

fs_client.connect(function(err) {
Expand Down Expand Up @@ -435,7 +436,8 @@ describe('Db', function() {
var configuration = this.configuration;
var client = configuration.newClient(`mongodb://127.0.0.1:27088/test`, {
auto_reconnect: false,
poolSize: 4
poolSize: 4,
serverSelectionTimeoutMS: 10
});

// Establish connection to db
Expand Down
15 changes: 12 additions & 3 deletions test/functional/mongo_client_tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,10 @@ describe('MongoClient', function() {
// The actual test we wish to run
test: function(done) {
var configuration = this.configuration;
const client = configuration.newClient('mongodb://localhost:27088/test');
const client = configuration.newClient('mongodb://localhost:27088/test', {
serverSelectionTimeoutMS: 10
});

client.connect(function(err) {
test.ok(err != null);

Expand Down Expand Up @@ -508,7 +511,10 @@ describe('MongoClient', function() {
// The actual test we wish to run
test: function(done) {
var configuration = this.configuration;
const client = configuration.newClient('mongodb://test.does.not.exist.com:80/test');
const client = configuration.newClient('mongodb://test.does.not.exist.com:80/test', {
serverSelectionTimeoutMS: 10
});

client.connect(function(err) {
test.ok(err != null);

Expand Down Expand Up @@ -593,7 +599,10 @@ describe('MongoClient', function() {
// The actual test we wish to run
test: function(done) {
var configuration = this.configuration;
const client = configuration.newClient('mongodb://unknownhost:36363/ddddd');
const client = configuration.newClient('mongodb://unknownhost:36363/ddddd', {
serverSelectionTimeoutMS: 10
});

client.connect(function(err) {
test.ok(err != null);
done();
Expand Down
3 changes: 2 additions & 1 deletion test/functional/operation_example_tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -4027,7 +4027,8 @@ describe('Operation Examples', function() {

const oldClient = secondClient;
const thirdClient = configuration.newClient(
'mongodb://user:name@localhost:27017/integration_tests'
'mongodb://user:name@localhost:27017/integration_tests',
{ serverSelectionTimeoutMS: 10 }
);

// Authenticate
Expand Down
5 changes: 4 additions & 1 deletion test/functional/operation_generators_example_tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -2966,7 +2966,10 @@ describe('Operation (Generators)', function() {

try {
// Authenticate
const client = configuration.newClient('mongodb://user:name@localhost:27017/admin');
const client = configuration.newClient('mongodb://user:name@localhost:27017/admin', {
serverSelectionTimeoutMS: 10
});

yield client.connect();
test.ok(false);
} catch (err) {} // eslint-disable-line
Expand Down
3 changes: 2 additions & 1 deletion test/functional/operation_promises_example_tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -3174,7 +3174,8 @@ describe('Operation (Promises)', function() {

// Should error out due to user no longer existing
const thirdClient = configuration.newClient(
'mongodb://user3:name@localhost:27017/integration_tests'
'mongodb://user3:name@localhost:27017/integration_tests',
{ serverSelectionTimeoutMS: 10 }
);

return thirdClient.connect();
Expand Down
19 changes: 14 additions & 5 deletions test/functional/scram_sha_256_tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -179,13 +179,17 @@ describe('SCRAM-SHA-256 auth', function() {
password: userMap.sha256.password
},
authSource: this.configuration.db,
authMechanism: 'SCRAM-SHA-1'
authMechanism: 'SCRAM-SHA-1',
serverSelectionTimeoutMS: 10
};

return withClient(
this.configuration.newClient({}, options),
() => Promise.reject(new Error('This request should have failed to authenticate')),
err => expect(err).to.match(/Authentication failed/)
err => {
const errMessage = err.reason ? err.reason.message : err;
expect(errMessage).to.match(/Authentication failed/);
}
);
}
});
Expand All @@ -202,22 +206,27 @@ describe('SCRAM-SHA-256 auth', function() {
user: 'roth',
password: 'pencil'
},
authSource: 'admin'
authSource: 'admin',
serverSelectionTimeoutMS: 1000
};

const badPasswordOptions = {
auth: {
user: 'both',
password: 'pencil'
},
authSource: 'admin'
authSource: 'admin',
serverSelectionTimeoutMS: 1000
};

const getErrorMsg = options =>
withClient(
this.configuration.newClient({}, options),
() => Promise.reject(new Error('This request should have failed to authenticate')),
err => expect(err).to.match(/Authentication failed/)
err => {
const errMessage = err.reason ? err.reason.message : err;
expect(errMessage).to.match(/Authentication failed/);
}
);

return Promise.all([getErrorMsg(noUsernameOptions), getErrorMsg(badPasswordOptions)]);
Expand Down

0 comments on commit 2b6a359

Please sign in to comment.