Skip to content

Commit

Permalink
refactor: improve error message for MongoTimeoutErrors
Browse files Browse the repository at this point in the history
We will now surface the first encountered error in the set of
known server descriptions as the primary error message for a
timeout error. The `reason` field for server selection errors
will now include the `TopologyDescription` for better traceability

NODE-2397
  • Loading branch information
mbroadst committed Dec 30, 2019
1 parent 8c89b89 commit aee8f57
Show file tree
Hide file tree
Showing 5 changed files with 14 additions and 16 deletions.
9 changes: 7 additions & 2 deletions lib/core/error.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,14 @@ class MongoParseError extends MongoError {
*/
class MongoTimeoutError extends MongoError {
constructor(message, reason) {
super(message);
if (reason && reason.error) {
super(reason.error);
} else {
super(message);
}

this.name = 'MongoTimeoutError';
if (reason != null) {
if (reason) {
this.reason = reason;
}
}
Expand Down
4 changes: 2 additions & 2 deletions lib/core/sdam/server_selection.js
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ function selectServers(topology, selector, timeout, start, callback) {
if (duration >= timeout) {
return callback(
new MongoTimeoutError(`Server selection timed out after ${timeout} ms`),
topology.description.error
topology.description
);
}

Expand Down Expand Up @@ -308,7 +308,7 @@ function selectServers(topology, selector, timeout, start, callback) {
callback(
new MongoTimeoutError(
`Server selection timed out after ${timeout} ms`,
topology.description.error
topology.description
)
);
}, timeout - duration);
Expand Down
3 changes: 1 addition & 2 deletions test/functional/core/server.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1002,8 +1002,7 @@ describe('Server tests', function() {
let err;
try {
expect(error).to.be.an.instanceOf(Error);
const errorMessage = error.reason ? error.reason.message : error.message;
expect(errorMessage).to.match(/but this version of the Node.js Driver requires/);
expect(error).to.match(/but this version of the Node.js Driver requires/);
} catch (e) {
err = e;
}
Expand Down
10 changes: 2 additions & 8 deletions test/functional/scram_sha_256.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -186,10 +186,7 @@ describe('SCRAM-SHA-256 auth', function() {
return withClient(
this.configuration.newClient({}, options),
() => Promise.reject(new Error('This request should have failed to authenticate')),
err => {
const errMessage = err.reason ? err.reason.message : err;
expect(errMessage).to.match(/Authentication failed/);
}
err => expect(err).to.match(/Authentication failed/)
);
}
});
Expand Down Expand Up @@ -223,10 +220,7 @@ describe('SCRAM-SHA-256 auth', function() {
withClient(
this.configuration.newClient({}, options),
() => Promise.reject(new Error('This request should have failed to authenticate')),
err => {
const errMessage = err.reason ? err.reason.message : err;
expect(errMessage).to.match(/Authentication failed/);
}
err => expect(err).to.match(/Authentication failed/)
);

return Promise.all([getErrorMsg(noUsernameOptions), getErrorMsg(badPasswordOptions)]);
Expand Down
4 changes: 2 additions & 2 deletions test/unit/sdam/server_selection/select_servers.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ describe('selectServers', function() {
selectServers(topology, ReadPreference.primary, 500, process.hrtime(), err => {
expect(err).to.exist;
expect(err).to.match(/Server selection timed out/);
expect(err).to.not.have.property('reason');
expect(err).to.have.property('reason');

done();
});
Expand All @@ -60,7 +60,7 @@ describe('selectServers', function() {
selectServers(topology, ReadPreference.primary, 1000, process.hrtime(), err => {
expect(err).to.exist;
expect(err).to.match(/Server selection timed out/);
expect(err).to.not.have.property('reason');
expect(err).to.have.property('reason');

// expect a call to monitor for initial server creation, and another for the server selection
expect(serverMonitor)
Expand Down

0 comments on commit aee8f57

Please sign in to comment.