Skip to content

Commit

Permalink
fix(server): non-timeout network errors transition to Unknown state
Browse files Browse the repository at this point in the history
Per the SDAM rules in the error handling section, a non-timeout
error encountered during operation execution MUST transition the
server to an `Unknown` state and kick off the SDAM flow.

NODE-2435
  • Loading branch information
mbroadst committed Jan 24, 2020
1 parent 3efa4c7 commit fa4b01b
Show file tree
Hide file tree
Showing 4 changed files with 48 additions and 3 deletions.
7 changes: 6 additions & 1 deletion lib/core/error.js
Expand Up @@ -251,6 +251,10 @@ function isSDAMUnrecoverableError(error) {
return false;
}

function isNetworkTimeoutError(err) {
return err instanceof MongoNetworkError && err.message.match(/timed out/);
}

module.exports = {
MongoError,
MongoNetworkError,
Expand All @@ -261,5 +265,6 @@ module.exports = {
mongoErrorContextSymbol,
isRetryableError,
isSDAMUnrecoverableError,
isNodeShuttingDownError
isNodeShuttingDownError,
isNetworkTimeoutError
};
7 changes: 5 additions & 2 deletions lib/core/sdam/server.js
Expand Up @@ -13,6 +13,7 @@ const MongoNetworkError = require('../error').MongoNetworkError;
const collationNotSupported = require('../utils').collationNotSupported;
const debugOptions = require('../connection/utils').debugOptions;
const isSDAMUnrecoverableError = require('../error').isSDAMUnrecoverableError;
const isNetworkTimeoutError = require('../error').isNetworkTimeoutError;
const makeStateMachine = require('../utils').makeStateMachine;
const common = require('./common');

Expand Down Expand Up @@ -447,9 +448,11 @@ function makeOperationHandler(server, options, callback) {
if (options && options.session) {
options.session.serverSession.isDirty = true;
}
}

if (isSDAMUnrecoverableError(err)) {
if (!isNetworkTimeoutError(err)) {
server.emit('error', err);
}
} else if (isSDAMUnrecoverableError(err)) {
server.emit('error', err);
}
}
Expand Down
3 changes: 3 additions & 0 deletions lib/core/sdam/topology.js
Expand Up @@ -795,6 +795,9 @@ function destroyServer(server, topology, options, callback) {
options = options || {};
LOCAL_SERVER_EVENTS.forEach(event => server.removeAllListeners(event));

// register a no-op for errors, we don't care now that we are destroying the server
server.on('error', () => {});

server.destroy(options, () => {
topology.emit(
'serverClosed',
Expand Down
34 changes: 34 additions & 0 deletions test/unit/sdam/topology.test.js
Expand Up @@ -192,5 +192,39 @@ describe('Topology (unit)', function() {
});
});
});

it('should set server to unknown on non-timeout network error', function(done) {
mockServer.setMessageHandler(request => {
const doc = request.document;
if (doc.ismaster) {
request.reply(Object.assign({}, mock.DEFAULT_ISMASTER, { maxWireVersion: 9 }));
} else if (doc.insert) {
request.connection.destroy();
} else {
request.reply({ ok: 1 });
}
});

const topology = new Topology(mockServer.uri());
topology.connect(err => {
expect(err).to.not.exist;

topology.selectServer('primary', (err, server) => {
expect(err).to.not.exist;
this.defer(() => topology.close());

let serverError;
server.on('error', err => (serverError = err));

server.command('test.test', { insert: { a: 42 } }, (err, result) => {
expect(result).to.not.exist;
expect(err).to.exist;
expect(err).to.eql(serverError);
expect(server.description.type).to.equal('Unknown');
done();
});
});
});
});
});
});

0 comments on commit fa4b01b

Please sign in to comment.