Skip to content

Commit

Permalink
feat(NODE-4650): handle handshake errors with SDAM (#3426)
Browse files Browse the repository at this point in the history
  • Loading branch information
dariakp authored Sep 29, 2022
1 parent 6b1cf88 commit cbe7533
Show file tree
Hide file tree
Showing 5 changed files with 197 additions and 23 deletions.
11 changes: 10 additions & 1 deletion src/cmap/connection_pool.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,13 @@ import {
CONNECTION_POOL_READY,
CONNECTION_READY
} from '../constants';
import { MongoError, MongoInvalidArgumentError, MongoRuntimeError } from '../error';
import {
MongoError,
MongoInvalidArgumentError,
MongoNetworkError,
MongoRuntimeError,
MongoServerError
} from '../error';
import { Logger } from '../logger';
import { CancellationToken, TypedEventEmitter } from '../mongo_types';
import type { Server } from '../sdam/server';
Expand Down Expand Up @@ -601,6 +607,9 @@ export class ConnectionPool extends TypedEventEmitter<ConnectionPoolEvents> {
ConnectionPool.CONNECTION_CLOSED,
new ConnectionClosedEvent(this, { id: connectOptions.id, serviceId: undefined }, 'error')
);
if (err instanceof MongoNetworkError || err instanceof MongoServerError) {
err.connectionGeneration = connectOptions.generation;
}
callback(err ?? new MongoRuntimeError('Connection creation failed without error'));
return;
}
Expand Down
1 change: 1 addition & 0 deletions src/error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ export class MongoError extends Error {
*/
code?: number | string;
topologyVersion?: TopologyVersion;
connectionGeneration?: number;
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
cause?: Error; // depending on the node version, this may or may not exist on the base
Expand Down
36 changes: 24 additions & 12 deletions src/sdam/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
MongoInvalidArgumentError,
MongoNetworkError,
MongoNetworkTimeoutError,
MongoRuntimeError,
MongoServerClosedError,
MongoServerError,
MongoUnexpectedServerResponseError,
Expand Down Expand Up @@ -348,8 +349,11 @@ export class Server extends TypedEventEmitter<ServerEvents> {
(err, conn, cb) => {
if (err || !conn) {
this.s.operationCount -= 1;
if (!err) {
return cb(new MongoRuntimeError('Failed to create connection without error'));
}
if (!(err instanceof PoolClearedError)) {
markServerUnknown(this, err);
this.handleError(err);
} else {
err.addErrorLabel(MongoErrorLabel.RetryableWriteError);
}
Expand Down Expand Up @@ -378,17 +382,25 @@ export class Server extends TypedEventEmitter<ServerEvents> {
if (!(error instanceof MongoError)) {
return;
}
if (error instanceof MongoNetworkError) {
if (!(error instanceof MongoNetworkTimeoutError) || isNetworkErrorBeforeHandshake(error)) {
// In load balanced mode we never mark the server as unknown and always
// clear for the specific service id.

if (!this.loadBalanced) {
error.addErrorLabel(MongoErrorLabel.ResetPool);
markServerUnknown(this, error);
} else if (connection) {
this.s.pool.clear(connection.serviceId);
}

const isStaleError =
error.connectionGeneration && error.connectionGeneration < this.s.pool.generation;
if (isStaleError) {
return;
}

const isNetworkNonTimeoutError =
error instanceof MongoNetworkError && !(error instanceof MongoNetworkTimeoutError);
const isNetworkTimeoutBeforeHandshakeError = isNetworkErrorBeforeHandshake(error);
const isAuthHandshakeError = error.hasErrorLabel(MongoErrorLabel.HandshakeError);
if (isNetworkNonTimeoutError || isNetworkTimeoutBeforeHandshakeError || isAuthHandshakeError) {
// In load balanced mode we never mark the server as unknown and always
// clear for the specific service id.
if (!this.loadBalanced) {
error.addErrorLabel(MongoErrorLabel.ResetPool);
markServerUnknown(this, error);
} else if (connection) {
this.s.pool.clear(connection.serviceId);
}
} else {
if (isSDAMUnrecoverableError(error)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,7 @@ import { TestFilter } from '../../tools/unified-spec-runner/schema';
import { sleep } from '../../tools/utils';

const filter: TestFilter = ({ description }) => {
const isAuthEnabled = process.env.AUTH === 'auth';
switch (description) {
case 'Reset server and pool after AuthenticationFailure error':
case 'Reset server and pool after misc command error':
case 'Reset server and pool after network error during authentication':
case 'Reset server and pool after network timeout error during authentication':
case 'Reset server and pool after shutdown error during authentication':
// These tests time out waiting for the PoolCleared event
return isAuthEnabled
? 'TODO(NODE-3135): handle auth errors, also see NODE-3891: fix tests broken when AUTH enabled'
: false;
case 'Network error on Monitor check':
case 'Network timeout on Monitor check':
return 'TODO(NODE-4608): Disallow parallel monitor checks';
Expand Down
162 changes: 162 additions & 0 deletions test/unit/sdam/server.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,162 @@
/* eslint-disable @typescript-eslint/no-non-null-assertion */
import { expect } from 'chai';
import { once } from 'events';
import * as sinon from 'sinon';

import {
Connection,
MongoError,
MongoErrorLabel,
MongoNetworkError,
MongoNetworkTimeoutError,
ObjectId,
ServerType,
TopologyType
} from '../../../src';
import { Server } from '../../../src/sdam/server';
import { ServerDescription } from '../../../src/sdam/server_description';
import { Topology } from '../../../src/sdam/topology';
import { sleep } from '../../tools/utils';

const handledErrors = [
{
description: 'any non-timeout network error',
errorClass: MongoNetworkError,
errorArgs: ['TestError']
},
{
description: 'a network timeout error before handshake',
errorClass: MongoNetworkTimeoutError,
errorArgs: ['TestError', { beforeHandshake: true }]
},
{
description: 'an auth handshake error',
errorClass: MongoError,
errorArgs: ['TestError'],
errorLabel: MongoErrorLabel.HandshakeError
}
];

const unhandledErrors = [
{
description: 'a non-MongoError',
errorClass: Error,
errorArgs: ['TestError']
},
{
description: 'a network timeout error after handshake',
errorClass: MongoNetworkTimeoutError,
errorArgs: ['TestError', { beforeHandshake: false }]
},
{
description: 'a non-network non-handshake MongoError',
errorClass: MongoError,
errorArgs: ['TestError']
}
];

describe('Server', () => {
describe('#handleError', () => {
let server: Server, connection: Connection | undefined;
beforeEach(() => {
server = new Server(new Topology([], {} as any), new ServerDescription('a:1'), {} as any);
});
for (const loadBalanced of [true, false]) {
const mode = loadBalanced ? 'loadBalanced' : 'non-loadBalanced';
const contextSuffix = loadBalanced ? ' with connection provided' : '';
context(`in ${mode} mode${contextSuffix}`, () => {
beforeEach(() => {
if (loadBalanced) {
server.s.topology.description.type = TopologyType.LoadBalanced;
connection = { serviceId: new ObjectId() } as Connection;
server.s.pool.clear = sinon.stub();
} else {
connection = undefined;
}
});
for (const { description, errorClass, errorArgs, errorLabel } of handledErrors) {
const handledDescription = loadBalanced
? `should reset the pool but not attach a ResetPool label to the error or mark the server unknown on ${description}`
: `should attach a ResetPool label to the error and mark the server unknown on ${description}`;
it(`${handledDescription}`, async () => {
// @ts-expect-error because of varied number of args
const error = new errorClass(...errorArgs);
if (errorLabel) {
error.addErrorLabel(errorLabel);
}
const newDescriptionEvent = Promise.race([
once(server, Server.DESCRIPTION_RECEIVED),
sleep(1000)
]);
server.handleError(error, connection);
if (!loadBalanced) {
expect(
error.hasErrorLabel(MongoErrorLabel.ResetPool),
'expected error to have a ResetPool label'
).to.be.true;
} else {
expect(
error.hasErrorLabel(MongoErrorLabel.ResetPool),
'expected error NOT to have a ResetPool label'
).to.be.false;
}
const newDescription = await newDescriptionEvent;
if (!loadBalanced) {
expect(newDescription).to.have.nested.property('[0].type', ServerType.Unknown);
} else {
expect(newDescription).to.be.undefined;
expect(server.s.pool.clear).to.have.been.calledOnceWith(connection!.serviceId);
}
});

it(`should not attach a ResetPool label to the error or mark the server unknown on ${description} if it is stale`, async () => {
// @ts-expect-error because of varied number of args
const error = new errorClass(...errorArgs);
if (errorLabel) {
error.addErrorLabel(errorLabel);
}

error.connectionGeneration = -1;
expect(
server.s.pool.generation,
'expected test server to have a pool of generation 0'
).to.equal(0); // sanity check

const newDescriptionEvent = Promise.race([
once(server, Server.DESCRIPTION_RECEIVED),
sleep(1000)
]);
server.handleError(error, connection);
expect(
error.hasErrorLabel(MongoErrorLabel.ResetPool),
'expected error NOT to have a ResetPool label'
).to.be.false;
const newDescription = await newDescriptionEvent;
expect(newDescription).to.be.undefined;
});
}

for (const { description, errorClass, errorArgs } of unhandledErrors) {
it(`should not attach a ResetPool label to the error or mark the server unknown on ${description}`, async () => {
// @ts-expect-error because of varied number of args
const error = new errorClass(...errorArgs);

const newDescriptionEvent = Promise.race([
once(server, Server.DESCRIPTION_RECEIVED),
sleep(1000)
]);
server.handleError(error, connection);
if (error instanceof MongoError) {
expect(
error.hasErrorLabel(MongoErrorLabel.ResetPool),
'expected error NOT to have a ResetPool label'
).to.be.false;
}
const newDescription = await newDescriptionEvent;
expect(newDescription).to.be.undefined;
});
}
});
}
});
});

0 comments on commit cbe7533

Please sign in to comment.