Skip to content

Commit 57f158f

Browse files
mbroadstmbroadst
committed
fix(topology): always emit SDAM unrecoverable errors
The SDAM changes introduced for the "Connections survive primary stepdown" project were erroneously implemented in this driver as guarding whether a server should be transitioned to an `Unknown` state, where the real intent is around whether a connection pool is cleared or not. NODE-2408
1 parent d0bfd8a commit 57f158f

File tree

5 files changed

+94
-20
lines changed

5 files changed

+94
-20
lines changed

‎lib/cmap/connection.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,7 @@ function messageHandler(conn) {
248248
return;
249249
}
250250

251-
if (document.ok === 0 || document.$err || document.errmsg) {
251+
if (document.ok === 0 || document.$err || document.errmsg || document.code) {
252252
callback(new MongoError(document));
253253
return;
254254
}

‎lib/core/error.js

+3-8
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
'use strict';
22

33
const mongoErrorContextSymbol = Symbol('mongoErrorContextSymbol');
4-
const maxWireVersion = require('./utils').maxWireVersion;
54

65
/**
76
* Creates a new MongoError
@@ -237,20 +236,15 @@ function isNodeShuttingDownError(err) {
237236
* @ignore
238237
* @see https://github.com./mongodb/specifications/blob/master/source/server-discovery-and-monitoring/server-discovery-and-monitoring.rst#not-master-and-node-is-recovering
239238
* @param {MongoError|Error} error
240-
* @param {Server} server
241239
*/
242-
function isSDAMUnrecoverableError(error, server) {
240+
function isSDAMUnrecoverableError(error) {
243241
// NOTE: null check is here for a strictly pre-CMAP world, a timeout or
244242
// close event are considered unrecoverable
245243
if (error instanceof MongoParseError || error == null) {
246244
return true;
247245
}
248246

249247
if (isRecoveringError(error) || isNotMasterError(error)) {
250-
if (maxWireVersion(server) >= 8 && !isNodeShuttingDownError(error)) {
251-
return false;
252-
}
253-
254248
return true;
255249
}
256250

@@ -266,5 +260,6 @@ module.exports = {
266260
MongoWriteConcernError,
267261
mongoErrorContextSymbol,
268262
isRetryableError,
269-
isSDAMUnrecoverableError
263+
isSDAMUnrecoverableError,
264+
isNodeShuttingDownError
270265
};

‎lib/core/sdam/server.js

+5-5
Original file line numberDiff line numberDiff line change
@@ -286,7 +286,7 @@ class Server extends EventEmitter {
286286
options.session.serverSession.isDirty = true;
287287
}
288288

289-
if (isSDAMUnrecoverableError(err, this)) {
289+
if (isSDAMUnrecoverableError(err)) {
290290
this.emit('error', err);
291291
}
292292
}
@@ -319,7 +319,7 @@ class Server extends EventEmitter {
319319
options.session.serverSession.isDirty = true;
320320
}
321321

322-
if (isSDAMUnrecoverableError(err, this)) {
322+
if (isSDAMUnrecoverableError(err)) {
323323
this.emit('error', err);
324324
}
325325
}
@@ -352,7 +352,7 @@ class Server extends EventEmitter {
352352
options.session.serverSession.isDirty = true;
353353
}
354354

355-
if (isSDAMUnrecoverableError(err, this)) {
355+
if (isSDAMUnrecoverableError(err)) {
356356
this.emit('error', err);
357357
}
358358
}
@@ -382,7 +382,7 @@ class Server extends EventEmitter {
382382
if (err) return cb(err);
383383

384384
conn.killCursors(ns, cursorState, (err, result) => {
385-
if (err && isSDAMUnrecoverableError(err, this)) {
385+
if (err && isSDAMUnrecoverableError(err)) {
386386
this.emit('error', err);
387387
}
388388

@@ -489,7 +489,7 @@ function executeWriteOperation(args, options, callback) {
489489
options.session.serverSession.isDirty = true;
490490
}
491491

492-
if (isSDAMUnrecoverableError(err, server)) {
492+
if (isSDAMUnrecoverableError(err)) {
493493
server.emit('error', err);
494494
}
495495
}

‎lib/core/sdam/topology.js

+5-6
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,8 @@ const deprecate = require('util').deprecate;
1414
const BSON = require('../connection/utils').retrieveBSON();
1515
const createCompressionInfo = require('../topologies/shared').createCompressionInfo;
1616
const isRetryableError = require('../error').isRetryableError;
17-
const isSDAMUnrecoverableError = require('../error').isSDAMUnrecoverableError;
17+
const isNodeShuttingDownError = require('../error').isNodeShuttingDownError;
18+
const maxWireVersion = require('../utils').maxWireVersion;
1819
const ClientSession = require('../sessions').ClientSession;
1920
const MongoError = require('../error').MongoError;
2021
const resolveClusterTime = require('../topologies/shared').resolveClusterTime;
@@ -881,14 +882,12 @@ function serverErrorEventHandler(server, topology) {
881882
return;
882883
}
883884

884-
if (isSDAMUnrecoverableError(err, server)) {
885-
// NOTE: this must be commented out until we switch to the new CMAP pool because
886-
// we presently _always_ clear the pool on error.
887-
resetServerState(server, err, { clearPool: true });
885+
if (maxWireVersion(server) >= 8 && !isNodeShuttingDownError(err)) {
886+
resetServerState(server, err);
888887
return;
889888
}
890889

891-
resetServerState(server, err);
890+
resetServerState(server, err, { clearPool: true });
892891
};
893892
}
894893

‎test/unit/sdam/topology.test.js

+80
Original file line numberDiff line numberDiff line change
@@ -113,4 +113,84 @@ describe('Topology (unit)', function() {
113113
});
114114
});
115115
});
116+
117+
describe('error handling', function() {
118+
let mockServer;
119+
beforeEach(() => mock.createServer().then(server => (mockServer = server)));
120+
afterEach(() => mock.cleanup());
121+
122+
it('should set server to unknown and reset pool on `node is recovering` error', function(done) {
123+
mockServer.setMessageHandler(request => {
124+
const doc = request.document;
125+
if (doc.ismaster) {
126+
request.reply(Object.assign({}, mock.DEFAULT_ISMASTER, { maxWireVersion: 9 }));
127+
} else if (doc.insert) {
128+
request.reply({ ok: 0, message: 'node is recovering', code: 11600 });
129+
} else {
130+
request.reply({ ok: 1 });
131+
}
132+
});
133+
134+
const topology = new Topology(mockServer.uri());
135+
topology.connect(err => {
136+
expect(err).to.not.exist;
137+
138+
topology.selectServer('primary', (err, server) => {
139+
expect(err).to.not.exist;
140+
this.defer(() => topology.close());
141+
142+
let serverError;
143+
server.on('error', err => (serverError = err));
144+
145+
let poolCleared = false;
146+
topology.on('connectionPoolCleared', () => (poolCleared = true));
147+
148+
server.command('test.test', { insert: { a: 42 } }, (err, result) => {
149+
expect(result).to.not.exist;
150+
expect(err).to.exist;
151+
expect(err).to.eql(serverError);
152+
expect(poolCleared).to.be.true;
153+
done();
154+
});
155+
});
156+
});
157+
});
158+
159+
it('should set server to unknown and NOT reset pool on stepdown errors', function(done) {
160+
mockServer.setMessageHandler(request => {
161+
const doc = request.document;
162+
if (doc.ismaster) {
163+
request.reply(Object.assign({}, mock.DEFAULT_ISMASTER, { maxWireVersion: 9 }));
164+
} else if (doc.insert) {
165+
request.reply({ ok: 0, message: 'not master' });
166+
} else {
167+
request.reply({ ok: 1 });
168+
}
169+
});
170+
171+
const topology = new Topology(mockServer.uri());
172+
topology.connect(err => {
173+
expect(err).to.not.exist;
174+
175+
topology.selectServer('primary', (err, server) => {
176+
expect(err).to.not.exist;
177+
this.defer(() => topology.close());
178+
179+
let serverError;
180+
server.on('error', err => (serverError = err));
181+
182+
let poolCleared = false;
183+
topology.on('connectionPoolCleared', () => (poolCleared = true));
184+
185+
server.command('test.test', { insert: { a: 42 } }, (err, result) => {
186+
expect(result).to.not.exist;
187+
expect(err).to.exist;
188+
expect(err).to.eql(serverError);
189+
expect(poolCleared).to.be.false;
190+
done();
191+
});
192+
});
193+
});
194+
});
195+
});
116196
});

0 commit comments

Comments
 (0)