From cbe75332d767a921551d71ea5725fac250906b00 Mon Sep 17 00:00:00 2001 From: Daria Pardue Date: Thu, 29 Sep 2022 14:33:35 -0400 Subject: [PATCH] feat(NODE-4650): handle handshake errors with SDAM (#3426) --- src/cmap/connection_pool.ts | 11 +- src/error.ts | 1 + src/sdam/server.ts | 36 ++-- ...rver_discovery_and_monitoring.spec.test.ts | 10 -- test/unit/sdam/server.test.ts | 162 ++++++++++++++++++ 5 files changed, 197 insertions(+), 23 deletions(-) create mode 100644 test/unit/sdam/server.test.ts diff --git a/src/cmap/connection_pool.ts b/src/cmap/connection_pool.ts index 0a0c1f417d..6bd65cce56 100644 --- a/src/cmap/connection_pool.ts +++ b/src/cmap/connection_pool.ts @@ -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'; @@ -601,6 +607,9 @@ export class ConnectionPool extends TypedEventEmitter { 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; } diff --git a/src/error.ts b/src/error.ts index 2a387a0d47..6a5d687ca8 100644 --- a/src/error.ts +++ b/src/error.ts @@ -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 diff --git a/src/sdam/server.ts b/src/sdam/server.ts index 2363717672..a693bf3bc8 100644 --- a/src/sdam/server.ts +++ b/src/sdam/server.ts @@ -29,6 +29,7 @@ import { MongoInvalidArgumentError, MongoNetworkError, MongoNetworkTimeoutError, + MongoRuntimeError, MongoServerClosedError, MongoServerError, MongoUnexpectedServerResponseError, @@ -348,8 +349,11 @@ export class Server extends TypedEventEmitter { (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); } @@ -378,17 +382,25 @@ export class Server extends TypedEventEmitter { 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)) { diff --git a/test/integration/server-discovery-and-monitoring/server_discovery_and_monitoring.spec.test.ts b/test/integration/server-discovery-and-monitoring/server_discovery_and_monitoring.spec.test.ts index 3a95f9493b..e99be158ca 100644 --- a/test/integration/server-discovery-and-monitoring/server_discovery_and_monitoring.spec.test.ts +++ b/test/integration/server-discovery-and-monitoring/server_discovery_and_monitoring.spec.test.ts @@ -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'; diff --git a/test/unit/sdam/server.test.ts b/test/unit/sdam/server.test.ts new file mode 100644 index 0000000000..cf810d8754 --- /dev/null +++ b/test/unit/sdam/server.test.ts @@ -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; + }); + } + }); + } + }); +});