From e8001b4fe2cd89a711fca56677d1c1f25db75fff Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 5 Aug 2024 13:33:11 -0400 Subject: [PATCH 01/28] test: sync latest spec tests --- ...lient_side_operations_timeout.spec.test.ts | 5 ++- .../sessions-inherit-timeoutMS.json | 28 +++++++++++++--- .../sessions-inherit-timeoutMS.yml | 19 +++++++---- ...sessions-override-operation-timeoutMS.json | 32 +++++++++++++++---- .../sessions-override-operation-timeoutMS.yml | 23 +++++++------ .../sessions-override-timeoutMS.json | 28 +++++++++++++--- .../sessions-override-timeoutMS.yml | 19 +++++++---- 7 files changed, 116 insertions(+), 38 deletions(-) diff --git a/test/integration/client-side-operations-timeout/client_side_operations_timeout.spec.test.ts b/test/integration/client-side-operations-timeout/client_side_operations_timeout.spec.test.ts index e4c9eb3027..b6f562a02e 100644 --- a/test/integration/client-side-operations-timeout/client_side_operations_timeout.spec.test.ts +++ b/test/integration/client-side-operations-timeout/client_side_operations_timeout.spec.test.ts @@ -8,7 +8,10 @@ const enabled = [ 'override-database-timeoutMS', 'override-operation-timeoutMS', 'retryability-legacy-timeouts', - 'retryability-timeoutMS' + 'retryability-timeoutMS', + 'sessions-override-operation-timeoutMS', + 'sessions-override-timeoutMS', + 'sessions-inherit-timeoutMS' ]; const cursorOperations = [ diff --git a/test/spec/client-side-operations-timeout/sessions-inherit-timeoutMS.json b/test/spec/client-side-operations-timeout/sessions-inherit-timeoutMS.json index abbc321732..13ea91c794 100644 --- a/test/spec/client-side-operations-timeout/sessions-inherit-timeoutMS.json +++ b/test/spec/client-side-operations-timeout/sessions-inherit-timeoutMS.json @@ -21,7 +21,7 @@ "client": { "id": "client", "uriOptions": { - "timeoutMS": 50 + "timeoutMS": 500 }, "useMultipleMongoses": false, "observeEvents": [ @@ -78,7 +78,7 @@ "commitTransaction" ], "blockConnection": true, - "blockTimeMS": 60 + "blockTimeMS": 600 } } } @@ -165,7 +165,7 @@ "abortTransaction" ], "blockConnection": true, - "blockTimeMS": 60 + "blockTimeMS": 600 } } } @@ -249,7 +249,7 @@ "insert" ], "blockConnection": true, - "blockTimeMS": 60 + "blockTimeMS": 600 } } } @@ -302,6 +302,26 @@ "commandFailedEvent": { "commandName": "insert" } + }, + { + "commandStartedEvent": { + "commandName": "abortTransaction", + "databaseName": "admin", + "command": { + "abortTransaction": 1, + "maxTimeMS": { + "$$type": [ + "int", + "long" + ] + } + } + } + }, + { + "commandFailedEvent": { + "commandName": "abortTransaction" + } } ] } diff --git a/test/spec/client-side-operations-timeout/sessions-inherit-timeoutMS.yml b/test/spec/client-side-operations-timeout/sessions-inherit-timeoutMS.yml index 184ef7eb9e..c79384e5f0 100644 --- a/test/spec/client-side-operations-timeout/sessions-inherit-timeoutMS.yml +++ b/test/spec/client-side-operations-timeout/sessions-inherit-timeoutMS.yml @@ -13,7 +13,7 @@ createEntities: - client: id: &client client uriOptions: - timeoutMS: 50 + timeoutMS: 500 useMultipleMongoses: false observeEvents: - commandStartedEvent @@ -52,7 +52,7 @@ tests: data: failCommands: ["commitTransaction"] blockConnection: true - blockTimeMS: 60 + blockTimeMS: 600 - name: startTransaction object: *session - name: insertOne @@ -95,7 +95,7 @@ tests: data: failCommands: ["abortTransaction"] blockConnection: true - blockTimeMS: 60 + blockTimeMS: 600 - name: startTransaction object: *session - name: insertOne @@ -136,7 +136,7 @@ tests: data: failCommands: ["insert"] blockConnection: true - blockTimeMS: 60 + blockTimeMS: 600 - name: withTransaction object: *session arguments: @@ -153,9 +153,6 @@ tests: expectEvents: - client: *client events: - # Because the insert expects an error and gets an error, it technically succeeds, so withTransaction will - # try to run commitTransaction. This will fail client-side, though, because the timeout has already expired, - # so no command is sent. - commandStartedEvent: commandName: insert databaseName: *databaseName @@ -166,3 +163,11 @@ tests: maxTimeMS: { $$type: ["int", "long"] } - commandFailedEvent: commandName: insert + - commandStartedEvent: + commandName: abortTransaction + databaseName: admin + command: + abortTransaction: 1 + maxTimeMS: { $$type: [ "int", "long" ] } + - commandFailedEvent: + commandName: abortTransaction diff --git a/test/spec/client-side-operations-timeout/sessions-override-operation-timeoutMS.json b/test/spec/client-side-operations-timeout/sessions-override-operation-timeoutMS.json index 0254b184a1..441c698328 100644 --- a/test/spec/client-side-operations-timeout/sessions-override-operation-timeoutMS.json +++ b/test/spec/client-side-operations-timeout/sessions-override-operation-timeoutMS.json @@ -75,7 +75,7 @@ "commitTransaction" ], "blockConnection": true, - "blockTimeMS": 60 + "blockTimeMS": 600 } } } @@ -98,7 +98,7 @@ "name": "commitTransaction", "object": "session", "arguments": { - "timeoutMS": 50 + "timeoutMS": 500 }, "expectError": { "isTimeoutError": true @@ -165,7 +165,7 @@ "abortTransaction" ], "blockConnection": true, - "blockTimeMS": 60 + "blockTimeMS": 600 } } } @@ -188,7 +188,7 @@ "name": "abortTransaction", "object": "session", "arguments": { - "timeoutMS": 50 + "timeoutMS": 500 } } ], @@ -252,7 +252,7 @@ "insert" ], "blockConnection": true, - "blockTimeMS": 60 + "blockTimeMS": 600 } } } @@ -261,7 +261,7 @@ "name": "withTransaction", "object": "session", "arguments": { - "timeoutMS": 50, + "timeoutMS": 500, "callback": [ { "name": "insertOne", @@ -306,6 +306,26 @@ "commandFailedEvent": { "commandName": "insert" } + }, + { + "commandStartedEvent": { + "commandName": "abortTransaction", + "databaseName": "admin", + "command": { + "abortTransaction": 1, + "maxTimeMS": { + "$$type": [ + "int", + "long" + ] + } + } + } + }, + { + "commandFailedEvent": { + "commandName": "abortTransaction" + } } ] } diff --git a/test/spec/client-side-operations-timeout/sessions-override-operation-timeoutMS.yml b/test/spec/client-side-operations-timeout/sessions-override-operation-timeoutMS.yml index 8a80a65720..bee91dc4cb 100644 --- a/test/spec/client-side-operations-timeout/sessions-override-operation-timeoutMS.yml +++ b/test/spec/client-side-operations-timeout/sessions-override-operation-timeoutMS.yml @@ -50,7 +50,7 @@ tests: data: failCommands: ["commitTransaction"] blockConnection: true - blockTimeMS: 60 + blockTimeMS: 600 - name: startTransaction object: *session - name: insertOne @@ -61,7 +61,7 @@ tests: - name: commitTransaction object: *session arguments: - timeoutMS: 50 + timeoutMS: 500 expectError: isTimeoutError: true expectEvents: @@ -95,7 +95,7 @@ tests: data: failCommands: ["abortTransaction"] blockConnection: true - blockTimeMS: 60 + blockTimeMS: 600 - name: startTransaction object: *session - name: insertOne @@ -106,7 +106,7 @@ tests: - name: abortTransaction object: *session arguments: - timeoutMS: 50 + timeoutMS: 500 expectEvents: - client: *client events: @@ -138,11 +138,11 @@ tests: data: failCommands: ["insert"] blockConnection: true - blockTimeMS: 60 + blockTimeMS: 600 - name: withTransaction object: *session arguments: - timeoutMS: 50 + timeoutMS: 500 callback: - name: insertOne object: *collection @@ -156,9 +156,6 @@ tests: expectEvents: - client: *client events: - # Because the insert expects an error and gets an error, it technically succeeds, so withTransaction will - # try to run commitTransaction. This will fail client-side, though, because the timeout has already expired, - # so no command is sent. - commandStartedEvent: commandName: insert databaseName: *databaseName @@ -169,3 +166,11 @@ tests: maxTimeMS: { $$type: ["int", "long"] } - commandFailedEvent: commandName: insert + - commandStartedEvent: + commandName: abortTransaction + databaseName: admin + command: + abortTransaction: 1 + maxTimeMS: { $$type: ["int", "long"] } + - commandFailedEvent: + commandName: abortTransaction diff --git a/test/spec/client-side-operations-timeout/sessions-override-timeoutMS.json b/test/spec/client-side-operations-timeout/sessions-override-timeoutMS.json index c46ae4dd50..d90152e909 100644 --- a/test/spec/client-side-operations-timeout/sessions-override-timeoutMS.json +++ b/test/spec/client-side-operations-timeout/sessions-override-timeoutMS.json @@ -47,7 +47,7 @@ "id": "session", "client": "client", "sessionOptions": { - "defaultTimeoutMS": 50 + "defaultTimeoutMS": 500 } } } @@ -78,7 +78,7 @@ "commitTransaction" ], "blockConnection": true, - "blockTimeMS": 60 + "blockTimeMS": 600 } } } @@ -165,7 +165,7 @@ "abortTransaction" ], "blockConnection": true, - "blockTimeMS": 60 + "blockTimeMS": 600 } } } @@ -249,7 +249,7 @@ "insert" ], "blockConnection": true, - "blockTimeMS": 60 + "blockTimeMS": 600 } } } @@ -302,6 +302,26 @@ "commandFailedEvent": { "commandName": "insert" } + }, + { + "commandStartedEvent": { + "commandName": "abortTransaction", + "databaseName": "admin", + "command": { + "abortTransaction": 1, + "maxTimeMS": { + "$$type": [ + "int", + "long" + ] + } + } + } + }, + { + "commandFailedEvent": { + "commandName": "abortTransaction" + } } ] } diff --git a/test/spec/client-side-operations-timeout/sessions-override-timeoutMS.yml b/test/spec/client-side-operations-timeout/sessions-override-timeoutMS.yml index 61aaab4d97..73aaf9ff2a 100644 --- a/test/spec/client-side-operations-timeout/sessions-override-timeoutMS.yml +++ b/test/spec/client-side-operations-timeout/sessions-override-timeoutMS.yml @@ -29,7 +29,7 @@ createEntities: id: &session session client: *client sessionOptions: - defaultTimeoutMS: 50 + defaultTimeoutMS: 500 initialData: - collectionName: *collectionName @@ -52,7 +52,7 @@ tests: data: failCommands: ["commitTransaction"] blockConnection: true - blockTimeMS: 60 + blockTimeMS: 600 - name: startTransaction object: *session - name: insertOne @@ -95,7 +95,7 @@ tests: data: failCommands: ["abortTransaction"] blockConnection: true - blockTimeMS: 60 + blockTimeMS: 600 - name: startTransaction object: *session - name: insertOne @@ -136,7 +136,7 @@ tests: data: failCommands: ["insert"] blockConnection: true - blockTimeMS: 60 + blockTimeMS: 600 - name: withTransaction object: *session arguments: @@ -153,9 +153,6 @@ tests: expectEvents: - client: *client events: - # Because the insert expects an error and gets an error, it technically succeeds, so withTransaction will - # try to run commitTransaction. This will fail client-side, though, because the timeout has already expired, - # so no command is sent. - commandStartedEvent: commandName: insert databaseName: *databaseName @@ -166,3 +163,11 @@ tests: maxTimeMS: { $$type: ["int", "long"] } - commandFailedEvent: commandName: insert + - commandStartedEvent: + commandName: abortTransaction + databaseName: admin + command: + abortTransaction: 1 + maxTimeMS: { $$type: [ "int", "long" ] } + - commandFailedEvent: + commandName: abortTransaction From a12f2b6120501107640ec121eff021af24433b2d Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 5 Aug 2024 13:57:16 -0400 Subject: [PATCH 02/28] test: set defaultTimeoutMS on session options --- test/tools/unified-spec-runner/entities.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/tools/unified-spec-runner/entities.ts b/test/tools/unified-spec-runner/entities.ts index 5b7a606280..54247f35c7 100644 --- a/test/tools/unified-spec-runner/entities.ts +++ b/test/tools/unified-spec-runner/entities.ts @@ -619,6 +619,10 @@ export class EntitiesMap extends Map { const options = Object.create(null); + if (entity.session.sessionOptions?.defaultTimeoutMS != null) { + options.defaultTimeoutMS = entity.session.sessionOptions?.defaultTimeoutMS; + } + if (entity.session.sessionOptions?.causalConsistency) { options.causalConsistency = entity.session.sessionOptions?.causalConsistency; } From 46e6bfd08dd4a9775bd81a0af49d2a506c5fb83e Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 5 Aug 2024 13:59:38 -0400 Subject: [PATCH 03/28] docs: fix api doc comment --- src/sessions.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/sessions.ts b/src/sessions.ts index 1fc88b468f..ff63472d4a 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -58,8 +58,11 @@ export interface ClientSessionOptions { snapshot?: boolean; /** The default TransactionOptions to use for transactions started on this session. */ defaultTransactionOptions?: TransactionOptions; - /** @internal - * The value of timeoutMS used for CSOT. Used to override client timeoutMS */ + /** + * @public + * An overriding timeoutMS value to use for a client-side timeout. + * If not provided the session uses the timeoutMS specified on the MongoClient. + */ defaultTimeoutMS?: number; /** @internal */ From 1e115e9d9c7170078802086d7e6487a2831ac23d Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 14 Aug 2024 15:39:48 -0400 Subject: [PATCH 04/28] chore: create and pass around timeout contexts --- package-lock.json | 9 +- package.json | 2 +- src/error.ts | 13 + src/operations/execute_operation.ts | 17 +- src/sessions.ts | 23 +- src/timeout.ts | 2 +- src/utils.ts | 6 + ...ient_side_operations_timeout.prose.test.ts | 225 ++++++++++++++---- test/tools/unified-spec-runner/operations.ts | 31 ++- 9 files changed, 248 insertions(+), 80 deletions(-) diff --git a/package-lock.json b/package-lock.json index e4c84ca29c..f2f85f68e2 100644 --- a/package-lock.json +++ b/package-lock.json @@ -51,7 +51,7 @@ "mocha": "^10.4.0", "mocha-sinon": "^2.1.2", "mongodb-client-encryption": "^6.1.0", - "mongodb-legacy": "^6.0.1", + "mongodb-legacy": "^6.1.1", "nyc": "^15.1.0", "prettier": "^2.8.8", "semver": "^7.6.0", @@ -7754,10 +7754,11 @@ } }, "node_modules/mongodb-legacy": { - "version": "6.0.1", - "resolved": "https://registry.npmjs.org/mongodb-legacy/-/mongodb-legacy-6.0.1.tgz", - "integrity": "sha512-cbm3UOqAYo6yElNk9CHNp5tQwRl1PCpBpcSkVvlvyg7S+gG6vSt1zrFiEniNaWOwwWnIr/IsAeSo48Nm701B/A==", + "version": "6.1.1", + "resolved": "https://registry.npmjs.org/mongodb-legacy/-/mongodb-legacy-6.1.1.tgz", + "integrity": "sha512-u9Cl8UEzdtf7mhWrAEHHhfU0OCqahaOB5midwtyudWIuEz5t18DJFXfqJq3cbEypVfLkfF3zi6rkolKMU9uPjQ==", "dev": true, + "license": "Apache-2.0", "dependencies": { "mongodb": "^6.0.0" }, diff --git a/package.json b/package.json index c57bbe76bb..1ab28643be 100644 --- a/package.json +++ b/package.json @@ -99,7 +99,7 @@ "mocha": "^10.4.0", "mocha-sinon": "^2.1.2", "mongodb-client-encryption": "^6.1.0", - "mongodb-legacy": "^6.0.1", + "mongodb-legacy": "^6.1.1", "nyc": "^15.1.0", "prettier": "^2.8.8", "semver": "^7.6.0", diff --git a/src/error.ts b/src/error.ts index b526ba24f9..4f9b507e95 100644 --- a/src/error.ts +++ b/src/error.ts @@ -765,9 +765,22 @@ export class MongoUnexpectedServerResponseError extends MongoRuntimeError { * @internal */ export class MongoOperationTimeoutError extends MongoRuntimeError { + get [Symbol.toStringTag]() { + return 'MongoOperationTimeoutError'; + } + override get name(): string { return 'MongoOperationTimeoutError'; } + + static is(error: unknown): error is MongoOperationTimeoutError { + return ( + error != null && + typeof error === 'object' && + Symbol.toStringTag in error && + error[Symbol.toStringTag] === 'MongoOperationTimeoutError' + ); + } } /** diff --git a/src/operations/execute_operation.ts b/src/operations/execute_operation.ts index 695e4ab92a..18c70eec0d 100644 --- a/src/operations/execute_operation.ts +++ b/src/operations/execute_operation.ts @@ -82,11 +82,6 @@ export async function executeOperation< } else if (session.client !== client) { throw new MongoInvalidArgumentError('ClientSession must be from the same MongoClient'); } - if (session.explicit && session?.timeoutMS != null && operation.options.timeoutMS != null) { - throw new MongoInvalidArgumentError( - 'Do not specify timeoutMS on operation if already specified on an explicit session' - ); - } const readPreference = operation.readPreference ?? ReadPreference.primary; const inTransaction = !!session?.inTransaction(); @@ -107,11 +102,13 @@ export async function executeOperation< session.unpin(); } - timeoutContext ??= TimeoutContext.create({ - serverSelectionTimeoutMS: client.s.options.serverSelectionTimeoutMS, - waitQueueTimeoutMS: client.s.options.waitQueueTimeoutMS, - timeoutMS: operation.options.timeoutMS - }); + timeoutContext ??= + session.timeoutContext ?? + TimeoutContext.create({ + serverSelectionTimeoutMS: client.s.options.serverSelectionTimeoutMS, + waitQueueTimeoutMS: client.s.options.waitQueueTimeoutMS, + timeoutMS: operation.options.timeoutMS ?? session.timeoutMS + }); try { return await tryOperation(operation, { diff --git a/src/sessions.ts b/src/sessions.ts index ff63472d4a..20dfd85a6d 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -16,6 +16,7 @@ import { MongoErrorLabel, MongoExpiredSessionError, MongoInvalidArgumentError, + MongoOperationTimeoutError, MongoRuntimeError, MongoServerError, MongoTransactionError, @@ -29,6 +30,7 @@ import { ReadConcernLevel } from './read_concern'; import { ReadPreference } from './read_preference'; import { type AsyncDisposable, configureResourceManagement } from './resource_management'; import { _advanceClusterTime, type ClusterTime, TopologyType } from './sdam/common'; +import { type TimeoutContext } from './timeout'; import { isTransactionCommand, Transaction, @@ -101,6 +103,9 @@ export interface EndSessionOptions { error?: AnyError; force?: boolean; forceClear?: boolean; + + /** @internal */ + timeoutMS?: number; } /** @@ -118,7 +123,7 @@ export class ClientSession /** @internal */ sessionPool: ServerSessionPool; hasEnded: boolean; - clientOptions?: MongoOptions; + clientOptions: MongoOptions; supports: { causalConsistency: boolean }; clusterTime?: ClusterTime; operationTime?: Timestamp; @@ -140,6 +145,9 @@ export class ClientSession /** @internal */ timeoutMS?: number; + /** @internal */ + public timeoutContext: TimeoutContext | null = null; + /** * Create a client session. * @internal @@ -152,7 +160,7 @@ export class ClientSession client: MongoClient, sessionPool: ServerSessionPool, options: ClientSessionOptions, - clientOptions?: MongoOptions + clientOptions: MongoOptions ) { super(); @@ -272,7 +280,11 @@ export class ClientSession async endSession(options?: EndSessionOptions): Promise { try { if (this.inTransaction()) { - await this.abortTransaction(); + if (typeof options?.timeoutMS === 'number') { + await this.abortTransaction({ timeoutMS: options.timeoutMS }); + } else { + await this.abortTransaction(); + } } if (!this.hasEnded) { const serverSession = this[kServerSession]; @@ -291,6 +303,7 @@ export class ClientSession } } catch (error) { // spec indicates that we should ignore all errors for `endSessions` + if (MongoOperationTimeoutError.is(error)) throw error; squashError(error); } finally { maybeClearPinnedConnection(this, { force: true, ...options }); @@ -444,6 +457,8 @@ export class ClientSession /** * Commits the currently active transaction in this session. + * + * @param options - Optional options, can be used to override `defaultTimeoutMS`. */ async commitTransaction(): Promise { if (this.transaction.state === TxnState.NO_TRANSACTION) { @@ -538,6 +553,8 @@ export class ClientSession /** * Aborts the currently active transaction in this session. + * + * @param options - Optional options, can be used to override `defaultTimeoutMS`. */ async abortTransaction(): Promise { if (this.transaction.state === TxnState.NO_TRANSACTION) { diff --git a/src/timeout.ts b/src/timeout.ts index 4007ffa50a..a0dabc4d53 100644 --- a/src/timeout.ts +++ b/src/timeout.ts @@ -184,7 +184,7 @@ export class CSOTTimeoutContext extends TimeoutContext { private _serverSelectionTimeout?: Timeout | null; private _connectionCheckoutTimeout?: Timeout | null; public minRoundTripTime = 0; - private start: number; + public start: number; constructor(options: CSOTTimeoutContextOptions) { super(); diff --git a/src/utils.ts b/src/utils.ts index 5648b226ef..0b06e1d349 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -528,6 +528,12 @@ export function resolveOptions( result.readPreference = readPreference; } + if (session?.explicit && session.timeoutMS != null && options?.timeoutMS != null) { + throw new MongoInvalidArgumentError( + 'Do not specify timeoutMS on operation if already specified on an explicit session' + ); + } + const timeoutMS = options?.timeoutMS; result.timeoutMS = timeoutMS ?? parent?.timeoutMS; diff --git a/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts b/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts index 729bed4219..c14ecf797a 100644 --- a/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts +++ b/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts @@ -1,6 +1,7 @@ /* Specification prose tests */ import { expect } from 'chai'; +import * as semver from 'semver'; import * as sinon from 'sinon'; import { @@ -9,6 +10,7 @@ import { MongoServerSelectionError, now } from '../../mongodb'; +import { type FailPoint } from '../../tools/utils'; // TODO(NODE-5824): Implement CSOT prose tests describe('CSOT spec prose tests', function () { @@ -595,69 +597,192 @@ describe('CSOT spec prose tests', function () { 'TODO(DRIVERS-2347): Requires this ticket to be implemented before we can assert on connection CSOT behaviour'; }); - context.skip('9. endSession', () => { - /** - * This test MUST only be run against replica sets and sharded clusters with server version 4.4 or higher. It MUST be - * run three times: once with the timeout specified via the MongoClient `timeoutMS` option, once with the timeout - * specified via the ClientSession `defaultTimeoutMS` option, and once more with the timeout specified via the - * `timeoutMS` option for the `endSession` operation. In all cases, the timeout MUST be set to 10 milliseconds. - * - * 1. Using `internalClient`, drop the `db.coll` collection. - * 1. Using `internalClient`, set the following fail point: - * ```js - * { - * configureFailPoint: failCommand, - * mode: { times: 1 }, - * data: { - * failCommands: ["abortTransaction"], - * blockConnection: true, - * blockTimeMS: 15 - * } - * } - * ``` - * 1. Create a new MongoClient (referred to as `client`) and an explicit ClientSession derived from that MongoClient (referred to as `session`). - * 1. Execute the following code: - * ```ts - * coll = client.database("db").collection("coll") - * session.start_transaction() - * coll.insert_one({x: 1}, session=session) - * ``` - * 1. Using `session`, execute `session.end_session` - * - Expect this to fail with a timeout error after no more than 15ms. - */ - }); - - context.skip('10. Convenient Transactions', () => { - /** Tests in this section MUST only run against replica sets and sharded clusters with server versions 4.4 or higher. */ - - context('timeoutMS is refreshed for abortTransaction if the callback fails', () => { + describe( + '9. endSession', + { requires: { topology: ['replicaset', 'sharded', 'load-balanced'] } }, + () => { /** + * This test MUST only be run against replica sets and sharded clusters with server version 4.4 or higher. It MUST be + * run three times: once with the timeout specified via the MongoClient `timeoutMS` option, once with the timeout + * specified via the ClientSession `defaultTimeoutMS` option, and once more with the timeout specified via the + * `timeoutMS` option for the `endSession` operation. In all cases, the timeout MUST be set to 10 milliseconds. + * * 1. Using `internalClient`, drop the `db.coll` collection. * 1. Using `internalClient`, set the following fail point: * ```js * { * configureFailPoint: failCommand, - * mode: { times: 2 }, + * mode: { times: 1 }, * data: { - * failCommands: ["insert", "abortTransaction"], + * failCommands: ["abortTransaction"], * blockConnection: true, * blockTimeMS: 15 * } * } * ``` - * 1. Create a new MongoClient (referred to as `client`) configured with `timeoutMS=10` and an explicit ClientSession derived from that MongoClient (referred to as `session`). - * 1. Using `session`, execute a `withTransaction` operation with the following callback: - * ```js - * function callback() { + * 1. Create a new MongoClient (referred to as `client`) and an explicit ClientSession derived from that MongoClient (referred to as `session`). + * 1. Execute the following code: + * ```ts * coll = client.database("db").collection("coll") - * coll.insert_one({ _id: 1 }, session=session) - * } + * session.start_transaction() + * coll.insert_one({x: 1}, session=session) * ``` - * 1. Expect the previous `withTransaction` call to fail with a timeout error. - * 1. Verify that the following events were published during the `withTransaction` call: - * 1. `command_started` and `command_failed` events for an `insert` command. - * 1. `command_started` and `command_failed` events for an `abortTransaction` command. + * 1. Using `session`, execute `session.end_session` + * - Expect this to fail with a timeout error after no more than 15ms. */ - }); - }); + const failpoint: FailPoint = { + configureFailPoint: 'failCommand', + mode: { times: 1 }, + data: { failCommands: ['abortTransaction'], blockConnection: true, blockTimeMS: 15 } + }; + + beforeEach(async function () { + if (!semver.satisfies(this.configuration.version, '>=4.4')) { + this.skipReason = 'Requires server version 4.4+'; + this.skip(); + } + const internalClient = this.configuration.newClient(); + await internalClient + .db('db') + .collection('coll') + .drop() + .catch(() => null); + await internalClient.db('admin').command(failpoint); + await internalClient.close(); + }); + + let client: MongoClient; + + afterEach(async () => { + await client?.close(); + }); + + describe('when timeoutMS is provided to the client', () => { + it('throws a timeout error from endSession', async function () { + client = this.configuration.newClient({ timeoutMS: 10 }); + const coll = client.db('db').collection('coll'); + const session = client.startSession(); + session.startTransaction(); + await coll.insertOne({ x: 1 }, { session }); + const error = await session.endSession().catch(error => error); + expect(error).to.be.instanceOf(MongoOperationTimeoutError); + }); + }); + + describe('when defaultTimeoutMS is provided to startSession', () => { + it('throws a timeout error from endSession', async function () { + client = this.configuration.newClient(); + const coll = client.db('db').collection('coll'); + const session = client.startSession({ defaultTimeoutMS: 10 }); + session.startTransaction(); + await coll.insertOne({ x: 1 }, { session }); + const error = await session.endSession().catch(error => error); + expect(error).to.be.instanceOf(MongoOperationTimeoutError); + }); + }); + + describe('when timeoutMS is provided to endSession', () => { + it('throws a timeout error from endSession', async function () { + client = this.configuration.newClient(); + const coll = client.db('db').collection('coll'); + const session = client.startSession(); + session.startTransaction(); + await coll.insertOne({ x: 1 }, { session }); + const error = await session.endSession({ timeoutMS: 10 }).catch(error => error); + expect(error).to.be.instanceOf(MongoOperationTimeoutError); + }); + }); + } + ); + + describe( + '10. Convenient Transactions', + { requires: { topology: ['replicaset', 'sharded', 'load-balanced'] } }, + () => { + /** Tests in this section MUST only run against replica sets and sharded clusters with server versions 4.4 or higher. */ + + describe('when an operation fails inside withTransaction callback', () => { + /** + * 1. Using `internalClient`, drop the `db.coll` collection. + * 1. Using `internalClient`, set the following fail point: + * ```js + * { + * configureFailPoint: failCommand, + * mode: { times: 2 }, + * data: { + * failCommands: ["insert", "abortTransaction"], + * blockConnection: true, + * blockTimeMS: 15 + * } + * } + * ``` + * 1. Create a new MongoClient (referred to as `client`) configured with `timeoutMS=10` and an explicit ClientSession derived from that MongoClient (referred to as `session`). + * 1. Using `session`, execute a `withTransaction` operation with the following callback: + * ```js + * function callback() { + * coll = client.database("db").collection("coll") + * coll.insert_one({ _id: 1 }, session=session) + * } + * ``` + * 1. Expect the previous `withTransaction` call to fail with a timeout error. + * 1. Verify that the following events were published during the `withTransaction` call: + * 1. `command_started` and `command_failed` events for an `insert` command. + * 1. `command_started` and `command_failed` events for an `abortTransaction` command. + */ + + const failpoint: FailPoint = { + configureFailPoint: 'failCommand', + mode: { times: 2 }, + data: { + failCommands: ['insert', 'abortTransaction'], + blockConnection: true, + blockTimeMS: 15 + } + }; + + beforeEach(async function () { + if (!semver.satisfies(this.configuration.version, '>=4.4')) { + this.skipReason = 'Requires server version 4.4+'; + this.skip(); + } + const internalClient = this.configuration.newClient(); + await internalClient + .db('db') + .collection('coll') + .drop() + .catch(() => null); + await internalClient.db('admin').command(failpoint); + await internalClient.close(); + }); + + let client: MongoClient; + + afterEach(async () => { + await client?.close(); + }); + + it('timeoutMS is refreshed for abortTransaction', async function () { + const commandsFailed = []; + + client = this.configuration + .newClient({ timeoutMS: 10, monitorCommands: true }) + .on('commandFailed', e => commandsFailed.push(e)); + + const coll = client.db('db').collection('coll'); + + const error = await client + .withSession(async session => + session.withTransaction(async session => await coll.insertOne({ x: 1 }, { session })) + ) + .catch(error => error); + + expect(error).to.be.instanceOf(MongoOperationTimeoutError); + expect(commandsFailed.map(e => e.commandName)).to.deep.equal([ + 'insert', + 'abortTransaction' + ]); + }); + }); + } + ); }); diff --git a/test/tools/unified-spec-runner/operations.ts b/test/tools/unified-spec-runner/operations.ts index 0d7fc18970..cc4829b9dd 100644 --- a/test/tools/unified-spec-runner/operations.ts +++ b/test/tools/unified-spec-runner/operations.ts @@ -19,6 +19,7 @@ import { ServerType, type TopologyDescription, type TopologyType, + type TransactionOptions, WriteConcern } from '../../mongodb'; import { sleep } from '../../tools/utils'; @@ -49,11 +50,6 @@ operations.set('createEntities', async ({ entities, operation, testConfig }) => await EntitiesMap.createEntities(testConfig, null, operation.arguments.entities!, entities); }); -operations.set('abortTransaction', async ({ entities, operation }) => { - const session = entities.getEntity('session', operation.object); - return session.abortTransaction(); -}); - operations.set('aggregate', async ({ entities, operation }) => { const dbOrCollection = entities.get(operation.object) as Db | Collection; if (!(dbOrCollection instanceof Db || dbOrCollection instanceof Collection)) { @@ -231,7 +227,16 @@ operations.set('close', async ({ entities, operation }) => { operations.set('commitTransaction', async ({ entities, operation }) => { const session = entities.getEntity('session', operation.object); - return session.commitTransaction(); + if (operation.arguments?.timeoutMS != null) + return await session.commitTransaction({ timeoutMS: operation.arguments.timeoutMS }); + return await session.commitTransaction(); +}); + +operations.set('abortTransaction', async ({ entities, operation }) => { + const session = entities.getEntity('session', operation.object); + if (operation.arguments?.timeoutMS != null) + return await session.abortTransaction({ timeoutMS: operation.arguments.timeoutMS }); + return await session.abortTransaction(); }); operations.set('createChangeStream', async ({ entities, operation }) => { @@ -361,7 +366,7 @@ operations.set('insertOne', async ({ entities, operation }) => { // Looping exposes the fact that we can generate _ids for inserted // documents and we don't want the original operation to get modified // and use the same _id for each insert. - return collection.insertOne({ ...document }, opts); + return await collection.insertOne({ ...document }, opts); }); operations.set('insertMany', async ({ entities, operation }) => { @@ -708,13 +713,17 @@ operations.set('waitForThread', async ({ entities, operation }) => { operations.set('withTransaction', async ({ entities, operation, client, testConfig }) => { const session = entities.getEntity('session', operation.object); - const options = { + const options: TransactionOptions = { readConcern: ReadConcern.fromOptions(operation.arguments), writeConcern: WriteConcern.fromOptions(operation.arguments), readPreference: ReadPreference.fromOptions(operation.arguments), - maxCommitTimeMS: operation.arguments!.maxCommitTimeMS + maxCommitTimeMS: operation.arguments?.maxCommitTimeMS }; + if (typeof operation.arguments?.timeoutMS === 'number') { + options.timeoutMS = operation.arguments.timeoutMS; + } + await session.withTransaction(async () => { for (const callbackOperation of operation.arguments!.callback) { await executeOperationAndCheck(callbackOperation, entities, client, testConfig, true); @@ -935,7 +944,7 @@ export async function executeOperationAndCheck( rethrow = false ): Promise { const opFunc = operations.get(operation.name); - expect(opFunc, `Unknown operation: ${operation.name}`).to.exist; + if (opFunc == null) expect.fail(`Unknown operation: ${operation.name}`); if (operation.arguments && operation.arguments.session) { // The session could need to be either pulled from the entity map or in the case where @@ -949,7 +958,7 @@ export async function executeOperationAndCheck( let result; try { - result = await opFunc!({ entities, operation, client, testConfig }); + result = await opFunc({ entities, operation, client, testConfig }); } catch (error) { if (operation.expectError) { expectErrorCheck(error, operation.expectError, entities); From 0ea664d7d69d4279367769a1cdd95619b94c367d Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 14 Aug 2024 17:31:04 -0400 Subject: [PATCH 05/28] chore: adjust timeouts and correct promise chain --- src/cmap/wire_protocol/on_data.ts | 2 +- src/sessions.ts | 4 +-- src/timeout.ts | 35 +++++++++++++------ src/utils.ts | 6 ---- ...ient_side_operations_timeout.prose.test.ts | 30 +++++++++++----- 5 files changed, 50 insertions(+), 27 deletions(-) diff --git a/src/cmap/wire_protocol/on_data.ts b/src/cmap/wire_protocol/on_data.ts index a32c6b1b48..ddfafb45a4 100644 --- a/src/cmap/wire_protocol/on_data.ts +++ b/src/cmap/wire_protocol/on_data.ts @@ -105,7 +105,7 @@ export function onData( function errorHandler(err: Error) { const promise = unconsumedPromises.shift(); const timeoutError = TimeoutError.is(err) - ? new MongoOperationTimeoutError('Timed out during socket read') + ? new MongoOperationTimeoutError(`Timed out during socket read (${err.duration}ms)`) : undefined; if (promise != null) promise.reject(timeoutError ?? err); diff --git a/src/sessions.ts b/src/sessions.ts index 20dfd85a6d..ba76f0dfa9 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -281,9 +281,9 @@ export class ClientSession try { if (this.inTransaction()) { if (typeof options?.timeoutMS === 'number') { - await this.abortTransaction({ timeoutMS: options.timeoutMS }); + await endTransaction(this, 'abortTransaction', { timeoutMS: options.timeoutMS }); } else { - await this.abortTransaction(); + await endTransaction(this, 'abortTransaction'); } } if (!this.hasEnded) { diff --git a/src/timeout.ts b/src/timeout.ts index a0dabc4d53..54566ec8a8 100644 --- a/src/timeout.ts +++ b/src/timeout.ts @@ -5,12 +5,14 @@ import { csotMin, noop } from './utils'; /** @internal */ export class TimeoutError extends Error { + duration: number; override get name(): 'TimeoutError' { return 'TimeoutError'; } - constructor(message: string, options?: { cause?: Error }) { + constructor(message: string, options: { cause?: Error; duration: number }) { super(message, options); + this.duration = options.duration; } static is(error: unknown): error is TimeoutError { @@ -52,7 +54,12 @@ export class Timeout extends Promise { } /** Create a new timeout that expires in `duration` ms */ - private constructor(executor: Executor = () => null, duration: number, unref = true) { + private constructor( + executor: Executor = () => null, + duration: number, + unref = true, + rejection: Error | null = null + ) { let reject!: Reject; if (duration < 0) { throw new MongoInvalidArgumentError('Cannot create a Timeout with a negative duration'); @@ -71,13 +78,15 @@ export class Timeout extends Promise { this.id = setTimeout(() => { this.ended = Math.trunc(performance.now()); this.timedOut = true; - reject(new TimeoutError(`Expired after ${duration}ms`)); + reject(new TimeoutError(`Expired after ${duration}ms`, { duration })); }, this.duration); if (typeof this.id.unref === 'function' && unref) { // Ensure we do not keep the Node.js event loop running this.id.unref(); } } + + if (rejection != null) reject(rejection); } /** @@ -90,7 +99,7 @@ export class Timeout extends Promise { } throwIfExpired(): void { - if (this.timedOut) throw new TimeoutError('Timed out'); + if (this.timedOut) throw new TimeoutError('Timed out', { duration: this.duration }); } public static expires(durationMS: number, unref?: boolean): Timeout { @@ -108,6 +117,10 @@ export class Timeout extends Promise { typeof timeout.then === 'function' ); } + + static override reject(rejection?: Error): Timeout { + return new Timeout(undefined, 0, true, rejection); + } } /** @internal */ @@ -218,8 +231,8 @@ export class CSOTTimeoutContext extends TimeoutContext { if (typeof this._serverSelectionTimeout !== 'object' || this._serverSelectionTimeout?.cleared) { const { remainingTimeMS, serverSelectionTimeoutMS } = this; if (remainingTimeMS <= 0) - throw new MongoOperationTimeoutError( - `Timed out in server selection after ${this.timeoutMS}ms` + return Timeout.reject( + new MongoOperationTimeoutError(`Timed out in server selection after ${this.timeoutMS}ms`) ); const usingServerSelectionTimeoutMS = serverSelectionTimeoutMS !== 0 && @@ -247,8 +260,10 @@ export class CSOTTimeoutContext extends TimeoutContext { // null or Timeout this._connectionCheckoutTimeout = this._serverSelectionTimeout; } else { - throw new MongoRuntimeError( - 'Unreachable. If you are seeing this error, please file a ticket on the NODE driver project on Jira' + return Timeout.reject( + new MongoRuntimeError( + 'Unreachable. If you are seeing this error, please file a ticket on the NODE driver project on Jira' + ) ); } } @@ -259,14 +274,14 @@ export class CSOTTimeoutContext extends TimeoutContext { const { remainingTimeMS } = this; if (!Number.isFinite(remainingTimeMS)) return null; if (remainingTimeMS > 0) return Timeout.expires(remainingTimeMS); - throw new MongoOperationTimeoutError('Timed out before socket write'); + return Timeout.reject(new MongoOperationTimeoutError('Timed out before socket write')); } get timeoutForSocketRead(): Timeout | null { const { remainingTimeMS } = this; if (!Number.isFinite(remainingTimeMS)) return null; if (remainingTimeMS > 0) return Timeout.expires(remainingTimeMS); - throw new MongoOperationTimeoutError('Timed out before socket read'); + return Timeout.reject(new MongoOperationTimeoutError('Timed out before socket read')); } } diff --git a/src/utils.ts b/src/utils.ts index 0b06e1d349..5648b226ef 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -528,12 +528,6 @@ export function resolveOptions( result.readPreference = readPreference; } - if (session?.explicit && session.timeoutMS != null && options?.timeoutMS != null) { - throw new MongoInvalidArgumentError( - 'Do not specify timeoutMS on operation if already specified on an explicit session' - ); - } - const timeoutMS = options?.timeoutMS; result.timeoutMS = timeoutMS ?? parent?.timeoutMS; diff --git a/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts b/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts index c14ecf797a..0c250b5585 100644 --- a/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts +++ b/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts @@ -633,7 +633,11 @@ describe('CSOT spec prose tests', function () { const failpoint: FailPoint = { configureFailPoint: 'failCommand', mode: { times: 1 }, - data: { failCommands: ['abortTransaction'], blockConnection: true, blockTimeMS: 15 } + data: { + failCommands: ['abortTransaction'], + blockConnection: true, + blockTimeMS: 60 + } }; beforeEach(async function () { @@ -653,13 +657,18 @@ describe('CSOT spec prose tests', function () { let client: MongoClient; - afterEach(async () => { + afterEach(async function () { + if (semver.satisfies(this.configuration.version, '>=4.4')) { + const internalClient = this.configuration.newClient(); + await internalClient.db('admin').command({ ...failpoint, mode: 'off' }); + await internalClient.close(); + } await client?.close(); }); describe('when timeoutMS is provided to the client', () => { it('throws a timeout error from endSession', async function () { - client = this.configuration.newClient({ timeoutMS: 10 }); + client = this.configuration.newClient({ timeoutMS: 50, monitorCommands: true }); const coll = client.db('db').collection('coll'); const session = client.startSession(); session.startTransaction(); @@ -673,7 +682,7 @@ describe('CSOT spec prose tests', function () { it('throws a timeout error from endSession', async function () { client = this.configuration.newClient(); const coll = client.db('db').collection('coll'); - const session = client.startSession({ defaultTimeoutMS: 10 }); + const session = client.startSession({ defaultTimeoutMS: 50 }); session.startTransaction(); await coll.insertOne({ x: 1 }, { session }); const error = await session.endSession().catch(error => error); @@ -688,7 +697,7 @@ describe('CSOT spec prose tests', function () { const session = client.startSession(); session.startTransaction(); await coll.insertOne({ x: 1 }, { session }); - const error = await session.endSession({ timeoutMS: 10 }).catch(error => error); + const error = await session.endSession({ timeoutMS: 50 }).catch(error => error); expect(error).to.be.instanceOf(MongoOperationTimeoutError); }); }); @@ -736,7 +745,7 @@ describe('CSOT spec prose tests', function () { data: { failCommands: ['insert', 'abortTransaction'], blockConnection: true, - blockTimeMS: 15 + blockTimeMS: 60 } }; @@ -757,7 +766,12 @@ describe('CSOT spec prose tests', function () { let client: MongoClient; - afterEach(async () => { + afterEach(async function () { + if (semver.satisfies(this.configuration.version, '>=4.4')) { + const internalClient = this.configuration.newClient(); + await internalClient.db('admin').command({ ...failpoint, mode: 'off' }); + await internalClient.close(); + } await client?.close(); }); @@ -765,7 +779,7 @@ describe('CSOT spec prose tests', function () { const commandsFailed = []; client = this.configuration - .newClient({ timeoutMS: 10, monitorCommands: true }) + .newClient({ timeoutMS: 50, monitorCommands: true }) .on('commandFailed', e => commandsFailed.push(e)); const coll = client.db('db').collection('coll'); From 2ddf2ebff39d73763de60a218f1e23eef737cdc5 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 20 Aug 2024 16:15:58 -0400 Subject: [PATCH 06/28] chore: fix when timeout errors throw --- src/cmap/connection.ts | 7 + src/cmap/wire_protocol/on_data.ts | 15 +-- src/sessions.ts | 205 +++++++++++++++++------------- src/timeout.ts | 14 +- src/transactions.ts | 5 +- 5 files changed, 145 insertions(+), 101 deletions(-) diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index a5ab7071ef..4d3a974080 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -737,6 +737,13 @@ export class Connection extends TypedEventEmitter { return; } } + } catch (readError) { + if (TimeoutError.is(readError)) { + throw new MongoOperationTimeoutError( + `Timed out during socket read (${readError.duration}ms)` + ); + } + throw readError; } finally { this.dataEvents = null; this.throwIfAborted(); diff --git a/src/cmap/wire_protocol/on_data.ts b/src/cmap/wire_protocol/on_data.ts index ddfafb45a4..23fd88e282 100644 --- a/src/cmap/wire_protocol/on_data.ts +++ b/src/cmap/wire_protocol/on_data.ts @@ -1,7 +1,6 @@ import { type EventEmitter } from 'events'; -import { MongoOperationTimeoutError } from '../../error'; -import { type TimeoutContext, TimeoutError } from '../../timeout'; +import { type TimeoutContext } from '../../timeout'; import { List, promiseWithResolvers } from '../../utils'; /** @@ -91,8 +90,11 @@ export function onData( // Adding event handlers emitter.on('data', eventHandler); emitter.on('error', errorHandler); + + const timeoutForSocketRead = timeoutContext?.timeoutForSocketRead; + timeoutForSocketRead?.throwIfExpired(); // eslint-disable-next-line github/no-then - timeoutContext?.timeoutForSocketRead?.then(undefined, errorHandler); + timeoutForSocketRead?.then(undefined, errorHandler); return iterator; @@ -104,12 +106,9 @@ export function onData( function errorHandler(err: Error) { const promise = unconsumedPromises.shift(); - const timeoutError = TimeoutError.is(err) - ? new MongoOperationTimeoutError(`Timed out during socket read (${err.duration}ms)`) - : undefined; - if (promise != null) promise.reject(timeoutError ?? err); - else error = timeoutError ?? err; + if (promise != null) promise.reject(err); + else error = err; void closeHandler(); } diff --git a/src/sessions.ts b/src/sessions.ts index ba76f0dfa9..4127900f74 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -30,7 +30,7 @@ import { ReadConcernLevel } from './read_concern'; import { ReadPreference } from './read_preference'; import { type AsyncDisposable, configureResourceManagement } from './resource_management'; import { _advanceClusterTime, type ClusterTime, TopologyType } from './sdam/common'; -import { type TimeoutContext } from './timeout'; +import { TimeoutContext } from './timeout'; import { isTransactionCommand, Transaction, @@ -280,12 +280,13 @@ export class ClientSession async endSession(options?: EndSessionOptions): Promise { try { if (this.inTransaction()) { - if (typeof options?.timeoutMS === 'number') { - await endTransaction(this, 'abortTransaction', { timeoutMS: options.timeoutMS }); - } else { - await endTransaction(this, 'abortTransaction'); - } + await this.abortTransaction({ ...options, throwTimeout: true }); } + } catch (error) { + // spec indicates that we should ignore all errors for `endSessions` + if (MongoOperationTimeoutError.is(error)) throw error; + squashError(error); + } finally { if (!this.hasEnded) { const serverSession = this[kServerSession]; if (serverSession != null) { @@ -301,11 +302,6 @@ export class ClientSession this.hasEnded = true; this.emit('ended', this); } - } catch (error) { - // spec indicates that we should ignore all errors for `endSessions` - if (MongoOperationTimeoutError.is(error)) throw error; - squashError(error); - } finally { maybeClearPinnedConnection(this, { force: true, ...options }); } } @@ -460,7 +456,7 @@ export class ClientSession * * @param options - Optional options, can be used to override `defaultTimeoutMS`. */ - async commitTransaction(): Promise { + async commitTransaction(options?: { timeoutMS?: number }): Promise { if (this.transaction.state === TxnState.NO_TRANSACTION) { throw new MongoTransactionError('No transaction started'); } @@ -510,8 +506,19 @@ export class ClientSession bypassPinningCheck: true }); + const timeoutMS = + typeof options?.timeoutMS === 'number' + ? options.timeoutMS + : typeof this.timeoutMS === 'number' + ? this.timeoutMS + : null; + + const timeoutContext = this.timeoutContext?.csotEnabled() + ? this.timeoutContext + : TimeoutContext.create({ timeoutMS, ...this.clientOptions }); + try { - await executeOperation(this.client, operation); + await executeOperation(this.client, operation, timeoutContext); return; } catch (firstCommitError) { if (firstCommitError instanceof MongoError && isRetryableWriteError(firstCommitError)) { @@ -521,7 +528,7 @@ export class ClientSession this.unpin({ force: true }); try { - await executeOperation(this.client, operation); + await executeOperation(this.client, operation, timeoutContext); return; } catch (retryCommitError) { // If the retry failed, we process that error instead of the original @@ -556,7 +563,10 @@ export class ClientSession * * @param options - Optional options, can be used to override `defaultTimeoutMS`. */ - async abortTransaction(): Promise { + async abortTransaction(options?: { timeoutMS?: number }): Promise; + /** @internal */ + async abortTransaction(options?: { timeoutMS?: number; throwTimeout?: true }): Promise; + async abortTransaction(options?: { timeoutMS?: number; throwTimeout?: true }): Promise { if (this.transaction.state === TxnState.NO_TRANSACTION) { throw new MongoTransactionError('No transaction started'); } @@ -601,18 +611,34 @@ export class ClientSession bypassPinningCheck: true }); + const timeoutMS = + typeof options?.timeoutMS === 'number' + ? options.timeoutMS + : this.timeoutContext?.csotEnabled() + ? this.timeoutContext.timeoutMS // refresh timeoutMS for abort operation + : typeof this.timeoutMS === 'number' + ? this.timeoutMS + : null; + + const timeoutContext = TimeoutContext.create({ timeoutMS, ...this.clientOptions }); + try { - await executeOperation(this.client, operation); + await executeOperation(this.client, operation, timeoutContext); this.unpin(); return; } catch (firstAbortError) { this.unpin(); + if (options?.throwTimeout && MongoOperationTimeoutError.is(firstAbortError)) + throw firstAbortError; + if (firstAbortError instanceof MongoError && isRetryableWriteError(firstAbortError)) { try { - await executeOperation(this.client, operation); + await executeOperation(this.client, operation, timeoutContext); return; } catch (secondAbortError) { + if (options?.throwTimeout && MongoOperationTimeoutError.is(secondAbortError)) + throw secondAbortError; // we do not retry the retry } } @@ -670,93 +696,102 @@ export class ClientSession options?: TransactionOptions ): Promise { const MAX_TIMEOUT = 120000; - const startTime = now(); - - let committed = false; - let result: any; - while (!committed) { - this.startTransaction(options); // may throw on error + this.timeoutContext = + options != null && 'timeoutMS' in options && typeof options.timeoutMS === 'number' + ? TimeoutContext.create({ timeoutMS: options.timeoutMS, ...this.clientOptions }) + : null; - try { - const promise = fn(this); - if (!isPromiseLike(promise)) { - throw new MongoInvalidArgumentError( - 'Function provided to `withTransaction` must return a Promise' - ); - } + const startTime = this.timeoutContext?.csotEnabled() ? this.timeoutContext.start : now(); - result = await promise; + let committed = false; + let result: any; - if ( - this.transaction.state === TxnState.NO_TRANSACTION || - this.transaction.state === TxnState.TRANSACTION_COMMITTED || - this.transaction.state === TxnState.TRANSACTION_ABORTED - ) { - // Assume callback intentionally ended the transaction - return result; - } - } catch (fnError) { - if (!(fnError instanceof MongoError) || fnError instanceof MongoInvalidArgumentError) { - await this.abortTransaction(); - throw fnError; - } + try { + while (!committed) { + this.startTransaction(options); // may throw on error - if ( - this.transaction.state === TxnState.STARTING_TRANSACTION || - this.transaction.state === TxnState.TRANSACTION_IN_PROGRESS - ) { - await this.abortTransaction(); - } + try { + const promise = fn(this); + if (!isPromiseLike(promise)) { + throw new MongoInvalidArgumentError( + 'Function provided to `withTransaction` must return a Promise' + ); + } - if ( - fnError.hasErrorLabel(MongoErrorLabel.TransientTransactionError) && - now() - startTime < MAX_TIMEOUT - ) { - continue; - } + result = await promise; - throw fnError; - } + if ( + this.transaction.state === TxnState.NO_TRANSACTION || + this.transaction.state === TxnState.TRANSACTION_COMMITTED || + this.transaction.state === TxnState.TRANSACTION_ABORTED + ) { + // Assume callback intentionally ended the transaction + return result; + } + } catch (fnError) { + if (!(fnError instanceof MongoError) || fnError instanceof MongoInvalidArgumentError) { + await this.abortTransaction(); + throw fnError; + } - while (!committed) { - try { - /* - * We will rely on ClientSession.commitTransaction() to - * apply a majority write concern if commitTransaction is - * being retried (see: DRIVERS-601) - */ - await this.commitTransaction(); - committed = true; - } catch (commitError) { - /* - * Note: a maxTimeMS error will have the MaxTimeMSExpired - * code (50) and can be reported as a top-level error or - * inside writeConcernError, ex. - * { ok:0, code: 50, codeName: 'MaxTimeMSExpired' } - * { ok:1, writeConcernError: { code: 50, codeName: 'MaxTimeMSExpired' } } - */ if ( - !isMaxTimeMSExpiredError(commitError) && - commitError.hasErrorLabel(MongoErrorLabel.UnknownTransactionCommitResult) && - now() - startTime < MAX_TIMEOUT + this.transaction.state === TxnState.STARTING_TRANSACTION || + this.transaction.state === TxnState.TRANSACTION_IN_PROGRESS ) { - continue; + await this.abortTransaction(); } if ( - commitError.hasErrorLabel(MongoErrorLabel.TransientTransactionError) && - now() - startTime < MAX_TIMEOUT + fnError.hasErrorLabel(MongoErrorLabel.TransientTransactionError) && + (this.timeoutContext != null || now() - startTime < MAX_TIMEOUT) ) { - break; + continue; } - throw commitError; + throw fnError; + } + + while (!committed) { + try { + /* + * We will rely on ClientSession.commitTransaction() to + * apply a majority write concern if commitTransaction is + * being retried (see: DRIVERS-601) + */ + await this.commitTransaction(); + committed = true; + } catch (commitError) { + /* + * Note: a maxTimeMS error will have the MaxTimeMSExpired + * code (50) and can be reported as a top-level error or + * inside writeConcernError, ex. + * { ok:0, code: 50, codeName: 'MaxTimeMSExpired' } + * { ok:1, writeConcernError: { code: 50, codeName: 'MaxTimeMSExpired' } } + */ + if ( + !isMaxTimeMSExpiredError(commitError) && + commitError.hasErrorLabel(MongoErrorLabel.UnknownTransactionCommitResult) && + (this.timeoutContext != null || now() - startTime < MAX_TIMEOUT) + ) { + continue; + } + + if ( + commitError.hasErrorLabel(MongoErrorLabel.TransientTransactionError) && + (this.timeoutContext != null || now() - startTime < MAX_TIMEOUT) + ) { + break; + } + + throw commitError; + } } } + return result; + } finally { + this.timeoutContext = null; } - - return result; } } diff --git a/src/timeout.ts b/src/timeout.ts index 54566ec8a8..efde00e098 100644 --- a/src/timeout.ts +++ b/src/timeout.ts @@ -74,7 +74,7 @@ export class Timeout extends Promise { this.duration = duration; this.start = Math.trunc(performance.now()); - if (this.duration > 0) { + if (rejection == null && this.duration > 0) { this.id = setTimeout(() => { this.ended = Math.trunc(performance.now()); this.timedOut = true; @@ -84,9 +84,11 @@ export class Timeout extends Promise { // Ensure we do not keep the Node.js event loop running this.id.unref(); } + } else if (rejection != null) { + this.ended = Math.trunc(performance.now()); + this.timedOut = true; + reject(rejection); } - - if (rejection != null) reject(rejection); } /** @@ -260,10 +262,8 @@ export class CSOTTimeoutContext extends TimeoutContext { // null or Timeout this._connectionCheckoutTimeout = this._serverSelectionTimeout; } else { - return Timeout.reject( - new MongoRuntimeError( - 'Unreachable. If you are seeing this error, please file a ticket on the NODE driver project on Jira' - ) + throw new MongoRuntimeError( + 'Unreachable. If you are seeing this error, please file a ticket on the NODE driver project on Jira' ); } } diff --git a/src/transactions.ts b/src/transactions.ts index 7ee8660b97..cbf639da08 100644 --- a/src/transactions.ts +++ b/src/transactions.ts @@ -69,7 +69,10 @@ export interface TransactionOptions extends CommandOperationOptions { writeConcern?: WriteConcern; /** A default read preference for commands in this transaction */ readPreference?: ReadPreferenceLike; - /** Specifies the maximum amount of time to allow a commit action on a transaction to run in milliseconds */ + /** + * Specifies the maximum amount of time to allow a commit action on a transaction to run in milliseconds + * @deprecated This option is deprecated in favor of `timeoutMS` or `defaultTimeoutMS`. + */ maxCommitTimeMS?: number; } From 219b5063db22a02f440c89b752519bde6552fcc3 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 20 Aug 2024 17:29:12 -0400 Subject: [PATCH 07/28] chore: limit server type --- ...ient_side_operations_timeout.prose.test.ts | 202 +++++++++--------- 1 file changed, 104 insertions(+), 98 deletions(-) diff --git a/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts b/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts index 0c250b5585..a6c9d4d804 100644 --- a/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts +++ b/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts @@ -597,112 +597,118 @@ describe('CSOT spec prose tests', function () { 'TODO(DRIVERS-2347): Requires this ticket to be implemented before we can assert on connection CSOT behaviour'; }); - describe( - '9. endSession', - { requires: { topology: ['replicaset', 'sharded', 'load-balanced'] } }, - () => { - /** - * This test MUST only be run against replica sets and sharded clusters with server version 4.4 or higher. It MUST be - * run three times: once with the timeout specified via the MongoClient `timeoutMS` option, once with the timeout - * specified via the ClientSession `defaultTimeoutMS` option, and once more with the timeout specified via the - * `timeoutMS` option for the `endSession` operation. In all cases, the timeout MUST be set to 10 milliseconds. - * - * 1. Using `internalClient`, drop the `db.coll` collection. - * 1. Using `internalClient`, set the following fail point: - * ```js - * { - * configureFailPoint: failCommand, - * mode: { times: 1 }, - * data: { - * failCommands: ["abortTransaction"], - * blockConnection: true, - * blockTimeMS: 15 - * } - * } - * ``` - * 1. Create a new MongoClient (referred to as `client`) and an explicit ClientSession derived from that MongoClient (referred to as `session`). - * 1. Execute the following code: - * ```ts - * coll = client.database("db").collection("coll") - * session.start_transaction() - * coll.insert_one({x: 1}, session=session) - * ``` - * 1. Using `session`, execute `session.end_session` - * - Expect this to fail with a timeout error after no more than 15ms. - */ - const failpoint: FailPoint = { - configureFailPoint: 'failCommand', - mode: { times: 1 }, - data: { - failCommands: ['abortTransaction'], - blockConnection: true, - blockTimeMS: 60 - } - }; - - beforeEach(async function () { - if (!semver.satisfies(this.configuration.version, '>=4.4')) { - this.skipReason = 'Requires server version 4.4+'; - this.skip(); - } - const internalClient = this.configuration.newClient(); - await internalClient - .db('db') - .collection('coll') - .drop() - .catch(() => null); - await internalClient.db('admin').command(failpoint); - await internalClient.close(); - }); + describe('9. endSession', () => { + /** + * This test MUST only be run against replica sets and sharded clusters with server version 4.4 or higher. It MUST be + * run three times: once with the timeout specified via the MongoClient `timeoutMS` option, once with the timeout + * specified via the ClientSession `defaultTimeoutMS` option, and once more with the timeout specified via the + * `timeoutMS` option for the `endSession` operation. In all cases, the timeout MUST be set to 10 milliseconds. + * + * 1. Using `internalClient`, drop the `db.coll` collection. + * 1. Using `internalClient`, set the following fail point: + * ```js + * { + * configureFailPoint: failCommand, + * mode: { times: 1 }, + * data: { + * failCommands: ["abortTransaction"], + * blockConnection: true, + * blockTimeMS: 15 + * } + * } + * ``` + * 1. Create a new MongoClient (referred to as `client`) and an explicit ClientSession derived from that MongoClient (referred to as `session`). + * 1. Execute the following code: + * ```ts + * coll = client.database("db").collection("coll") + * session.start_transaction() + * coll.insert_one({x: 1}, session=session) + * ``` + * 1. Using `session`, execute `session.end_session` + * - Expect this to fail with a timeout error after no more than 15ms. + */ + const failpoint: FailPoint = { + configureFailPoint: 'failCommand', + mode: { times: 1 }, + data: { + failCommands: ['abortTransaction'], + blockConnection: true, + blockTimeMS: 60 + } + }; + + beforeEach(async function () { + if (!semver.satisfies(this.configuration.version, '>=4.4')) { + this.skipReason = 'Requires server version 4.4+'; + this.skip(); + } + + if ( + !['Sharded', 'ReplicaSetWithPrimary', 'ReplicaSetNoPrimary'].includes( + this.configuration.topologyType + ) + ) { + this.skipReason = 'Requires replicaset or sharded clusters'; + this.skip(); + } + + const internalClient = this.configuration.newClient(); + await internalClient + .db('db') + .collection('coll') + .drop() + .catch(() => null); + await internalClient.db('admin').command(failpoint); + await internalClient.close(); + }); - let client: MongoClient; + let client: MongoClient; - afterEach(async function () { - if (semver.satisfies(this.configuration.version, '>=4.4')) { - const internalClient = this.configuration.newClient(); - await internalClient.db('admin').command({ ...failpoint, mode: 'off' }); - await internalClient.close(); - } - await client?.close(); - }); + afterEach(async function () { + if (semver.satisfies(this.configuration.version, '>=4.4')) { + const internalClient = this.configuration.newClient(); + await internalClient.db('admin').command({ ...failpoint, mode: 'off' }); + await internalClient.close(); + } + await client?.close(); + }); - describe('when timeoutMS is provided to the client', () => { - it('throws a timeout error from endSession', async function () { - client = this.configuration.newClient({ timeoutMS: 50, monitorCommands: true }); - const coll = client.db('db').collection('coll'); - const session = client.startSession(); - session.startTransaction(); - await coll.insertOne({ x: 1 }, { session }); - const error = await session.endSession().catch(error => error); - expect(error).to.be.instanceOf(MongoOperationTimeoutError); - }); + describe('when timeoutMS is provided to the client', () => { + it('throws a timeout error from endSession', async function () { + client = this.configuration.newClient({ timeoutMS: 50, monitorCommands: true }); + const coll = client.db('db').collection('coll'); + const session = client.startSession(); + session.startTransaction(); + await coll.insertOne({ x: 1 }, { session }); + const error = await session.endSession().catch(error => error); + expect(error).to.be.instanceOf(MongoOperationTimeoutError); }); + }); - describe('when defaultTimeoutMS is provided to startSession', () => { - it('throws a timeout error from endSession', async function () { - client = this.configuration.newClient(); - const coll = client.db('db').collection('coll'); - const session = client.startSession({ defaultTimeoutMS: 50 }); - session.startTransaction(); - await coll.insertOne({ x: 1 }, { session }); - const error = await session.endSession().catch(error => error); - expect(error).to.be.instanceOf(MongoOperationTimeoutError); - }); + describe('when defaultTimeoutMS is provided to startSession', () => { + it('throws a timeout error from endSession', async function () { + client = this.configuration.newClient(); + const coll = client.db('db').collection('coll'); + const session = client.startSession({ defaultTimeoutMS: 50 }); + session.startTransaction(); + await coll.insertOne({ x: 1 }, { session }); + const error = await session.endSession().catch(error => error); + expect(error).to.be.instanceOf(MongoOperationTimeoutError); }); + }); - describe('when timeoutMS is provided to endSession', () => { - it('throws a timeout error from endSession', async function () { - client = this.configuration.newClient(); - const coll = client.db('db').collection('coll'); - const session = client.startSession(); - session.startTransaction(); - await coll.insertOne({ x: 1 }, { session }); - const error = await session.endSession({ timeoutMS: 50 }).catch(error => error); - expect(error).to.be.instanceOf(MongoOperationTimeoutError); - }); + describe('when timeoutMS is provided to endSession', () => { + it('throws a timeout error from endSession', async function () { + client = this.configuration.newClient(); + const coll = client.db('db').collection('coll'); + const session = client.startSession(); + session.startTransaction(); + await coll.insertOne({ x: 1 }, { session }); + const error = await session.endSession({ timeoutMS: 50 }).catch(error => error); + expect(error).to.be.instanceOf(MongoOperationTimeoutError); }); - } - ); + }); + }); describe( '10. Convenient Transactions', From a0824369989491548c60909687781fed04d7b12e Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 21 Aug 2024 10:04:03 -0400 Subject: [PATCH 08/28] test: fix metadata --- ...ient_side_operations_timeout.prose.test.ts | 185 +++++++++--------- 1 file changed, 89 insertions(+), 96 deletions(-) diff --git a/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts b/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts index a6c9d4d804..84626696a9 100644 --- a/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts +++ b/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts @@ -598,6 +598,9 @@ describe('CSOT spec prose tests', function () { }); describe('9. endSession', () => { + const metadata: MongoDBMetadataUI = { + requires: { mongodb: '>=4.4', topology: ['replicaset', 'sharded'] } + }; /** * This test MUST only be run against replica sets and sharded clusters with server version 4.4 or higher. It MUST be * run three times: once with the timeout specified via the MongoClient `timeoutMS` option, once with the timeout @@ -643,15 +646,6 @@ describe('CSOT spec prose tests', function () { this.skip(); } - if ( - !['Sharded', 'ReplicaSetWithPrimary', 'ReplicaSetNoPrimary'].includes( - this.configuration.topologyType - ) - ) { - this.skipReason = 'Requires replicaset or sharded clusters'; - this.skip(); - } - const internalClient = this.configuration.newClient(); await internalClient .db('db') @@ -674,7 +668,7 @@ describe('CSOT spec prose tests', function () { }); describe('when timeoutMS is provided to the client', () => { - it('throws a timeout error from endSession', async function () { + it('throws a timeout error from endSession', metadata, async function () { client = this.configuration.newClient({ timeoutMS: 50, monitorCommands: true }); const coll = client.db('db').collection('coll'); const session = client.startSession(); @@ -686,7 +680,7 @@ describe('CSOT spec prose tests', function () { }); describe('when defaultTimeoutMS is provided to startSession', () => { - it('throws a timeout error from endSession', async function () { + it('throws a timeout error from endSession', metadata, async function () { client = this.configuration.newClient(); const coll = client.db('db').collection('coll'); const session = client.startSession({ defaultTimeoutMS: 50 }); @@ -698,7 +692,7 @@ describe('CSOT spec prose tests', function () { }); describe('when timeoutMS is provided to endSession', () => { - it('throws a timeout error from endSession', async function () { + it('throws a timeout error from endSession', metadata, async function () { client = this.configuration.newClient(); const coll = client.db('db').collection('coll'); const session = client.startSession(); @@ -710,99 +704,98 @@ describe('CSOT spec prose tests', function () { }); }); - describe( - '10. Convenient Transactions', - { requires: { topology: ['replicaset', 'sharded', 'load-balanced'] } }, - () => { - /** Tests in this section MUST only run against replica sets and sharded clusters with server versions 4.4 or higher. */ + describe('10. Convenient Transactions', () => { + /** Tests in this section MUST only run against replica sets and sharded clusters with server versions 4.4 or higher. */ + const metadata: MongoDBMetadataUI = { + requires: { topology: ['replicaset', 'sharded', 'load-balanced'] } + }; - describe('when an operation fails inside withTransaction callback', () => { - /** - * 1. Using `internalClient`, drop the `db.coll` collection. - * 1. Using `internalClient`, set the following fail point: - * ```js - * { - * configureFailPoint: failCommand, - * mode: { times: 2 }, - * data: { - * failCommands: ["insert", "abortTransaction"], - * blockConnection: true, - * blockTimeMS: 15 - * } - * } - * ``` - * 1. Create a new MongoClient (referred to as `client`) configured with `timeoutMS=10` and an explicit ClientSession derived from that MongoClient (referred to as `session`). - * 1. Using `session`, execute a `withTransaction` operation with the following callback: - * ```js - * function callback() { - * coll = client.database("db").collection("coll") - * coll.insert_one({ _id: 1 }, session=session) - * } - * ``` - * 1. Expect the previous `withTransaction` call to fail with a timeout error. - * 1. Verify that the following events were published during the `withTransaction` call: - * 1. `command_started` and `command_failed` events for an `insert` command. - * 1. `command_started` and `command_failed` events for an `abortTransaction` command. - */ + describe('when an operation fails inside withTransaction callback', () => { + /** + * 1. Using `internalClient`, drop the `db.coll` collection. + * 1. Using `internalClient`, set the following fail point: + * ```js + * { + * configureFailPoint: failCommand, + * mode: { times: 2 }, + * data: { + * failCommands: ["insert", "abortTransaction"], + * blockConnection: true, + * blockTimeMS: 15 + * } + * } + * ``` + * 1. Create a new MongoClient (referred to as `client`) configured with `timeoutMS=10` and an explicit ClientSession derived from that MongoClient (referred to as `session`). + * 1. Using `session`, execute a `withTransaction` operation with the following callback: + * ```js + * function callback() { + * coll = client.database("db").collection("coll") + * coll.insert_one({ _id: 1 }, session=session) + * } + * ``` + * 1. Expect the previous `withTransaction` call to fail with a timeout error. + * 1. Verify that the following events were published during the `withTransaction` call: + * 1. `command_started` and `command_failed` events for an `insert` command. + * 1. `command_started` and `command_failed` events for an `abortTransaction` command. + */ - const failpoint: FailPoint = { - configureFailPoint: 'failCommand', - mode: { times: 2 }, - data: { - failCommands: ['insert', 'abortTransaction'], - blockConnection: true, - blockTimeMS: 60 - } - }; + const failpoint: FailPoint = { + configureFailPoint: 'failCommand', + mode: { times: 2 }, + data: { + failCommands: ['insert', 'abortTransaction'], + blockConnection: true, + blockTimeMS: 60 + } + }; + + beforeEach(async function () { + if (!semver.satisfies(this.configuration.version, '>=4.4')) { + this.skipReason = 'Requires server version 4.4+'; + this.skip(); + } + const internalClient = this.configuration.newClient(); + await internalClient + .db('db') + .collection('coll') + .drop() + .catch(() => null); + await internalClient.db('admin').command(failpoint); + await internalClient.close(); + }); - beforeEach(async function () { - if (!semver.satisfies(this.configuration.version, '>=4.4')) { - this.skipReason = 'Requires server version 4.4+'; - this.skip(); - } + let client: MongoClient; + + afterEach(async function () { + if (semver.satisfies(this.configuration.version, '>=4.4')) { const internalClient = this.configuration.newClient(); - await internalClient - .db('db') - .collection('coll') - .drop() - .catch(() => null); - await internalClient.db('admin').command(failpoint); + await internalClient.db('admin').command({ ...failpoint, mode: 'off' }); await internalClient.close(); - }); - - let client: MongoClient; - - afterEach(async function () { - if (semver.satisfies(this.configuration.version, '>=4.4')) { - const internalClient = this.configuration.newClient(); - await internalClient.db('admin').command({ ...failpoint, mode: 'off' }); - await internalClient.close(); - } - await client?.close(); - }); + } + await client?.close(); + }); - it('timeoutMS is refreshed for abortTransaction', async function () { - const commandsFailed = []; + it('timeoutMS is refreshed for abortTransaction', metadata, async function () { + const commandsFailed = []; - client = this.configuration - .newClient({ timeoutMS: 50, monitorCommands: true }) - .on('commandFailed', e => commandsFailed.push(e)); + client = this.configuration + .newClient({ timeoutMS: 50, monitorCommands: true }) + .on('commandFailed', e => commandsFailed.push(e)); - const coll = client.db('db').collection('coll'); + const coll = client.db('db').collection('coll'); - const error = await client - .withSession(async session => - session.withTransaction(async session => await coll.insertOne({ x: 1 }, { session })) - ) - .catch(error => error); + const error = await client + .withSession(async session => + session.withTransaction(async session => await coll.insertOne({ x: 1 }, { session })) + ) + .catch(error => error); - expect(error).to.be.instanceOf(MongoOperationTimeoutError); - expect(commandsFailed.map(e => e.commandName)).to.deep.equal([ - 'insert', - 'abortTransaction' - ]); - }); + expect(error).to.be.instanceOf(MongoOperationTimeoutError); + expect(commandsFailed.map(e => e.commandName)).to.deep.equal([ + 'insert', + 'abortTransaction' + ]); }); - } - ); + }); + }); }); From 008de841c23d62d820ea117cf2a74e6de0e019af Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 21 Aug 2024 15:24:30 -0400 Subject: [PATCH 09/28] chore: drop collection once --- ...ient_side_operations_timeout.prose.test.ts | 37 +++++++++++-------- 1 file changed, 21 insertions(+), 16 deletions(-) diff --git a/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts b/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts index 84626696a9..2d786f4d5e 100644 --- a/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts +++ b/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts @@ -13,7 +13,7 @@ import { import { type FailPoint } from '../../tools/utils'; // TODO(NODE-5824): Implement CSOT prose tests -describe('CSOT spec prose tests', function () { +describe.only('CSOT spec prose tests', function () { let internalClient: MongoClient; let client: MongoClient; @@ -636,22 +636,27 @@ describe('CSOT spec prose tests', function () { data: { failCommands: ['abortTransaction'], blockConnection: true, - blockTimeMS: 60 + blockTimeMS: 600 } }; - beforeEach(async function () { + before('drop collection', async function () { + const internalClient = this.configuration.newClient(); + await internalClient + .db('endSession_db') + .collection('endSession_coll') + .drop() + .catch(() => null); + await internalClient.close(); + }); + + beforeEach('setup failpoint', async function () { if (!semver.satisfies(this.configuration.version, '>=4.4')) { this.skipReason = 'Requires server version 4.4+'; this.skip(); } const internalClient = this.configuration.newClient(); - await internalClient - .db('db') - .collection('coll') - .drop() - .catch(() => null); await internalClient.db('admin').command(failpoint); await internalClient.close(); }); @@ -669,8 +674,8 @@ describe('CSOT spec prose tests', function () { describe('when timeoutMS is provided to the client', () => { it('throws a timeout error from endSession', metadata, async function () { - client = this.configuration.newClient({ timeoutMS: 50, monitorCommands: true }); - const coll = client.db('db').collection('coll'); + client = this.configuration.newClient({ timeoutMS: 500, monitorCommands: true }); + const coll = client.db('endSession_db').collection('endSession_coll'); const session = client.startSession(); session.startTransaction(); await coll.insertOne({ x: 1 }, { session }); @@ -682,8 +687,8 @@ describe('CSOT spec prose tests', function () { describe('when defaultTimeoutMS is provided to startSession', () => { it('throws a timeout error from endSession', metadata, async function () { client = this.configuration.newClient(); - const coll = client.db('db').collection('coll'); - const session = client.startSession({ defaultTimeoutMS: 50 }); + const coll = client.db('endSession_db').collection('endSession_coll'); + const session = client.startSession({ defaultTimeoutMS: 500 }); session.startTransaction(); await coll.insertOne({ x: 1 }, { session }); const error = await session.endSession().catch(error => error); @@ -694,11 +699,11 @@ describe('CSOT spec prose tests', function () { describe('when timeoutMS is provided to endSession', () => { it('throws a timeout error from endSession', metadata, async function () { client = this.configuration.newClient(); - const coll = client.db('db').collection('coll'); + const coll = client.db('endSession_db').collection('endSession_coll'); const session = client.startSession(); session.startTransaction(); await coll.insertOne({ x: 1 }, { session }); - const error = await session.endSession({ timeoutMS: 50 }).catch(error => error); + const error = await session.endSession({ timeoutMS: 500 }).catch(error => error); expect(error).to.be.instanceOf(MongoOperationTimeoutError); }); }); @@ -745,7 +750,7 @@ describe('CSOT spec prose tests', function () { data: { failCommands: ['insert', 'abortTransaction'], blockConnection: true, - blockTimeMS: 60 + blockTimeMS: 600 } }; @@ -779,7 +784,7 @@ describe('CSOT spec prose tests', function () { const commandsFailed = []; client = this.configuration - .newClient({ timeoutMS: 50, monitorCommands: true }) + .newClient({ timeoutMS: 500, monitorCommands: true }) .on('commandFailed', e => commandsFailed.push(e)); const coll = client.db('db').collection('coll'); From c1d34e570f36c73eec71d8d14a10de64f966ab04 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 21 Aug 2024 16:00:17 -0400 Subject: [PATCH 10/28] test: improve prose test --- ...ient_side_operations_timeout.prose.test.ts | 33 ++++++++++++------- 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts b/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts index 2d786f4d5e..00c762205f 100644 --- a/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts +++ b/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts @@ -13,7 +13,7 @@ import { import { type FailPoint } from '../../tools/utils'; // TODO(NODE-5824): Implement CSOT prose tests -describe.only('CSOT spec prose tests', function () { +describe('CSOT spec prose tests', function () { let internalClient: MongoClient; let client: MongoClient; @@ -712,7 +712,7 @@ describe.only('CSOT spec prose tests', function () { describe('10. Convenient Transactions', () => { /** Tests in this section MUST only run against replica sets and sharded clusters with server versions 4.4 or higher. */ const metadata: MongoDBMetadataUI = { - requires: { topology: ['replicaset', 'sharded', 'load-balanced'] } + requires: { topology: ['replicaset', 'sharded'], mongodb: '>=4.4' } }; describe('when an operation fails inside withTransaction callback', () => { @@ -782,24 +782,33 @@ describe.only('CSOT spec prose tests', function () { it('timeoutMS is refreshed for abortTransaction', metadata, async function () { const commandsFailed = []; + const commandsStarted = []; client = this.configuration .newClient({ timeoutMS: 500, monitorCommands: true }) - .on('commandFailed', e => commandsFailed.push(e)); + .on('commandStarted', e => commandsStarted.push(e.commandName)) + .on('commandFailed', e => commandsFailed.push(e.commandName)); const coll = client.db('db').collection('coll'); - const error = await client - .withSession(async session => - session.withTransaction(async session => await coll.insertOne({ x: 1 }, { session })) - ) + const session = client.startSession(); + + let insertError: Error | null = null; + const withTransactionError = await session + .withTransaction(async session => { + insertError = await coll.insertOne({ x: 1 }, { session }).catch(error => error); + throw insertError; + }) .catch(error => error); - expect(error).to.be.instanceOf(MongoOperationTimeoutError); - expect(commandsFailed.map(e => e.commandName)).to.deep.equal([ - 'insert', - 'abortTransaction' - ]); + try { + expect(insertError).to.be.instanceOf(MongoOperationTimeoutError); + expect(withTransactionError).to.be.instanceOf(MongoOperationTimeoutError); + expect(commandsStarted, 'commands started').to.deep.equal(['insert', 'abortTransaction']); + expect(commandsFailed, 'commands failed').to.deep.equal(['insert', 'abortTransaction']); + } finally { + await session.endSession(); + } }); }); }); From 9cb3fe5e202d6bec73875c712224a106a9c99115 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 23 Aug 2024 14:07:01 -0400 Subject: [PATCH 11/28] fix: always pass timeoutMS value --- test/tools/unified-spec-runner/operations.ts | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/test/tools/unified-spec-runner/operations.ts b/test/tools/unified-spec-runner/operations.ts index cc4829b9dd..d43f541aae 100644 --- a/test/tools/unified-spec-runner/operations.ts +++ b/test/tools/unified-spec-runner/operations.ts @@ -227,16 +227,12 @@ operations.set('close', async ({ entities, operation }) => { operations.set('commitTransaction', async ({ entities, operation }) => { const session = entities.getEntity('session', operation.object); - if (operation.arguments?.timeoutMS != null) - return await session.commitTransaction({ timeoutMS: operation.arguments.timeoutMS }); - return await session.commitTransaction(); + return await session.commitTransaction({ timeoutMS: operation.arguments?.timeoutMS }); }); operations.set('abortTransaction', async ({ entities, operation }) => { const session = entities.getEntity('session', operation.object); - if (operation.arguments?.timeoutMS != null) - return await session.abortTransaction({ timeoutMS: operation.arguments.timeoutMS }); - return await session.abortTransaction(); + return await session.abortTransaction({ timeoutMS: operation.arguments?.timeoutMS }); }); operations.set('createChangeStream', async ({ entities, operation }) => { From 8f961b1571662ca506a59b9ae7dd0d57ac1ecf29 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 23 Aug 2024 14:17:07 -0400 Subject: [PATCH 12/28] fix: make timeout take options object --- src/timeout.ts | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/timeout.ts b/src/timeout.ts index efde00e098..51386689bd 100644 --- a/src/timeout.ts +++ b/src/timeout.ts @@ -56,15 +56,13 @@ export class Timeout extends Promise { /** Create a new timeout that expires in `duration` ms */ private constructor( executor: Executor = () => null, - duration: number, - unref = true, - rejection: Error | null = null + { duration, unref, rejection }: { duration: number; unref?: true; rejection?: Error } ) { - let reject!: Reject; if (duration < 0) { throw new MongoInvalidArgumentError('Cannot create a Timeout with a negative duration'); } + let reject!: Reject; super((_, promiseReject) => { reject = promiseReject; @@ -104,8 +102,8 @@ export class Timeout extends Promise { if (this.timedOut) throw new TimeoutError('Timed out', { duration: this.duration }); } - public static expires(durationMS: number, unref?: boolean): Timeout { - return new Timeout(undefined, durationMS, unref); + public static expires(duration: number, unref?: true): Timeout { + return new Timeout(undefined, { duration, unref }); } static is(timeout: unknown): timeout is Timeout { @@ -121,7 +119,7 @@ export class Timeout extends Promise { } static override reject(rejection?: Error): Timeout { - return new Timeout(undefined, 0, true, rejection); + return new Timeout(undefined, { duration: 0, unref: true, rejection }); } } From a84a4826ad19d9ab318d0347c1b4bd4f627ffd7d Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 23 Aug 2024 15:11:59 -0400 Subject: [PATCH 13/28] fix: add documentation of timeoutMS in withTransaction --- src/sessions.ts | 11 ++++++++++- src/transactions.ts | 2 +- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/sessions.ts b/src/sessions.ts index 4127900f74..1709992d93 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -693,7 +693,16 @@ export class ClientSession */ async withTransaction( fn: WithTransactionCallback, - options?: TransactionOptions + options?: TransactionOptions & { + /** + * Configures a timeoutMS expiry for the entire withTransactionCallback. + * + * @remarks + * - The remaining timeout will not be applied to callback operations that do not use the ClientSession. + * - Overriding timeoutMS for operations executed using the explicit session inside the provided callback will result in a client-side error. + */ + timeoutMS?: number; + } ): Promise { const MAX_TIMEOUT = 120000; diff --git a/src/transactions.ts b/src/transactions.ts index cbf639da08..2fbb0ca42b 100644 --- a/src/transactions.ts +++ b/src/transactions.ts @@ -61,7 +61,7 @@ const COMMITTED_STATES: Set = new Set([ * Configuration options for a transaction. * @public */ -export interface TransactionOptions extends CommandOperationOptions { +export interface TransactionOptions extends Omit { // TODO(NODE-3344): These options use the proper class forms of these settings, it should accept the basic enum values too /** A default read concern for commands in this transaction */ readConcern?: ReadConcernLike; From eab394f580b55377eccfe827c28738521dee13cf Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 23 Aug 2024 15:13:38 -0400 Subject: [PATCH 14/28] fix: add validation for timeoutMS in withTxn operations --- src/sessions.ts | 7 ++- src/timeout.ts | 6 +- src/utils.ts | 8 ++- .../node_csot.test.ts | 61 +++++++++++++++++++ 4 files changed, 77 insertions(+), 5 deletions(-) diff --git a/src/sessions.ts b/src/sessions.ts index 1709992d93..8cce00697e 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -707,8 +707,11 @@ export class ClientSession const MAX_TIMEOUT = 120000; this.timeoutContext = - options != null && 'timeoutMS' in options && typeof options.timeoutMS === 'number' - ? TimeoutContext.create({ timeoutMS: options.timeoutMS, ...this.clientOptions }) + typeof options?.timeoutMS === 'number' || typeof this.timeoutMS === 'number' + ? TimeoutContext.create({ + timeoutMS: options?.timeoutMS ?? this.timeoutMS, + ...this.clientOptions + }) : null; const startTime = this.timeoutContext?.csotEnabled() ? this.timeoutContext.start : now(); diff --git a/src/timeout.ts b/src/timeout.ts index 51386689bd..d805f84194 100644 --- a/src/timeout.ts +++ b/src/timeout.ts @@ -56,8 +56,12 @@ export class Timeout extends Promise { /** Create a new timeout that expires in `duration` ms */ private constructor( executor: Executor = () => null, - { duration, unref, rejection }: { duration: number; unref?: true; rejection?: Error } + options?: { duration: number; unref?: true; rejection?: Error } ) { + const duration = options?.duration ?? 0; + const unref = !!options?.unref; + const rejection = options?.rejection; + if (duration < 0) { throw new MongoInvalidArgumentError('Cannot create a Timeout with a negative duration'); } diff --git a/src/utils.ts b/src/utils.ts index 5648b226ef..49452835e3 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -528,9 +528,13 @@ export function resolveOptions( result.readPreference = readPreference; } - const timeoutMS = options?.timeoutMS; + if (session?.explicit && session?.timeoutContext != null && options?.timeoutMS != null) { + throw new MongoInvalidArgumentError( + 'An operation cannot be given a timeoutMS setting when inside a withTransaction call that has a timeoutMS setting' + ); + } - result.timeoutMS = timeoutMS ?? parent?.timeoutMS; + result.timeoutMS = options?.timeoutMS ?? parent?.timeoutMS; return result; } diff --git a/test/integration/client-side-operations-timeout/node_csot.test.ts b/test/integration/client-side-operations-timeout/node_csot.test.ts index d7d4a4ede5..76973106e3 100644 --- a/test/integration/client-side-operations-timeout/node_csot.test.ts +++ b/test/integration/client-side-operations-timeout/node_csot.test.ts @@ -12,6 +12,7 @@ import { type FindCursor, LEGACY_HELLO_COMMAND, type MongoClient, + MongoInvalidArgumentError, MongoOperationTimeoutError, MongoServerError } from '../../mongodb'; @@ -320,4 +321,64 @@ describe('CSOT driver tests', { requires: { mongodb: '>=4.4' } }, () => { }); }); }); + + describe.only('when using an explicit session', () => { + describe('created for a withTransaction callback', () => { + describe('passing a timeoutMS and a session with a timeoutContext', () => { + let client: MongoClient; + + beforeEach(async function () { + client = this.configuration.newClient({ timeoutMS: 123 }); + }); + + afterEach(async function () { + await client.close(); + }); + + it('throws a validation error from the operation', async () => { + // Drivers MUST raise a validation error if an explicit session with a timeout is used and + // the timeoutMS option is set at the operation level for operations executed as part of a withTransaction callback. + + const coll = client.db('db').collection('coll'); + + const session = client.startSession(); + + let insertError: Error | null = null; + const withTransactionError = await session + .withTransaction(async session => { + insertError = await coll + .insertOne({ x: 1 }, { session, timeoutMS: 1234 }) + .catch(error => error); + throw insertError; + }) + .catch(error => error); + + expect(insertError).to.be.instanceOf(MongoInvalidArgumentError); + expect(withTransactionError).to.be.instanceOf(MongoInvalidArgumentError); + }); + }); + }); + + describe('created manually', () => { + describe('passing a timeoutMS and a session with an inherited timeoutMS', () => { + let client: MongoClient; + + beforeEach(async function () { + client = this.configuration.newClient({ timeoutMS: 123 }); + }); + + afterEach(async function () { + await client.close(); + }); + + it('does not throw a validation error', async () => { + const coll = client.db('db').collection('coll'); + const session = client.startSession(); + session.startTransaction(); + await coll.insertOne({ x: 1 }, { session, timeoutMS: 1234 }); + await session.abortTransaction(); // this uses the inherited timeoutMS, not the insert + }); + }); + }); + }); }); From a55fd6a39fd21f2a5b60a63ae09ede5c2dc0d44b Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 23 Aug 2024 15:22:38 -0400 Subject: [PATCH 15/28] fix: only and metadata --- .../client-side-operations-timeout/node_csot.test.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/integration/client-side-operations-timeout/node_csot.test.ts b/test/integration/client-side-operations-timeout/node_csot.test.ts index 76973106e3..af7becb515 100644 --- a/test/integration/client-side-operations-timeout/node_csot.test.ts +++ b/test/integration/client-side-operations-timeout/node_csot.test.ts @@ -322,7 +322,9 @@ describe('CSOT driver tests', { requires: { mongodb: '>=4.4' } }, () => { }); }); - describe.only('when using an explicit session', () => { + describe('when using an explicit session', () => { + const metadata = { requires: { topology: ['replicaset'] } }; + describe('created for a withTransaction callback', () => { describe('passing a timeoutMS and a session with a timeoutContext', () => { let client: MongoClient; @@ -335,7 +337,7 @@ describe('CSOT driver tests', { requires: { mongodb: '>=4.4' } }, () => { await client.close(); }); - it('throws a validation error from the operation', async () => { + it('throws a validation error from the operation', metadata, async () => { // Drivers MUST raise a validation error if an explicit session with a timeout is used and // the timeoutMS option is set at the operation level for operations executed as part of a withTransaction callback. @@ -371,7 +373,7 @@ describe('CSOT driver tests', { requires: { mongodb: '>=4.4' } }, () => { await client.close(); }); - it('does not throw a validation error', async () => { + it('does not throw a validation error', metadata, async () => { const coll = client.db('db').collection('coll'); const session = client.startSession(); session.startTransaction(); From ec174f51b70d97584d5cdf39c76c384d41bbe88a Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 26 Aug 2024 14:46:40 -0400 Subject: [PATCH 16/28] chore: fix collection drop hang --- ...ient_side_operations_timeout.prose.test.ts | 23 ++++++------------- 1 file changed, 7 insertions(+), 16 deletions(-) diff --git a/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts b/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts index 00c762205f..6fbcbacca9 100644 --- a/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts +++ b/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts @@ -640,23 +640,16 @@ describe('CSOT spec prose tests', function () { } }; - before('drop collection', async function () { + beforeEach(async function () { const internalClient = this.configuration.newClient(); + // End in-progress transactions otherwise "drop" will hang + await internalClient.db('admin').command({ killAllSessions: [] }); await internalClient .db('endSession_db') .collection('endSession_coll') .drop() .catch(() => null); - await internalClient.close(); - }); - - beforeEach('setup failpoint', async function () { - if (!semver.satisfies(this.configuration.version, '>=4.4')) { - this.skipReason = 'Requires server version 4.4+'; - this.skip(); - } - - const internalClient = this.configuration.newClient(); + await internalClient.db('endSession_db').createCollection('endSession_coll'); await internalClient.db('admin').command(failpoint); await internalClient.close(); }); @@ -664,11 +657,9 @@ describe('CSOT spec prose tests', function () { let client: MongoClient; afterEach(async function () { - if (semver.satisfies(this.configuration.version, '>=4.4')) { - const internalClient = this.configuration.newClient(); - await internalClient.db('admin').command({ ...failpoint, mode: 'off' }); - await internalClient.close(); - } + const internalClient = this.configuration.newClient(); + await internalClient.db('admin').command({ ...failpoint, mode: 'off' }); + await internalClient.close(); await client?.close(); }); From 174df9ef0b741e64f941f0198a2a581f5f3cce27 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 26 Aug 2024 15:19:29 -0400 Subject: [PATCH 17/28] chore: remove is --- src/error.ts | 16 +++------------- src/sessions.ts | 11 +++++++---- 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/src/error.ts b/src/error.ts index 4f9b507e95..4435e90f0a 100644 --- a/src/error.ts +++ b/src/error.ts @@ -124,6 +124,9 @@ function isAggregateError(e: unknown): e is Error & { errors: Error[] } { * mongodb-client-encryption has a dependency on this error, it uses the constructor with a string argument */ export class MongoError extends Error { + get [Symbol.toStringTag]() { + return this.name; + } /** @internal */ [kErrorLabels]: Set; /** @@ -765,22 +768,9 @@ export class MongoUnexpectedServerResponseError extends MongoRuntimeError { * @internal */ export class MongoOperationTimeoutError extends MongoRuntimeError { - get [Symbol.toStringTag]() { - return 'MongoOperationTimeoutError'; - } - override get name(): string { return 'MongoOperationTimeoutError'; } - - static is(error: unknown): error is MongoOperationTimeoutError { - return ( - error != null && - typeof error === 'object' && - Symbol.toStringTag in error && - error[Symbol.toStringTag] === 'MongoOperationTimeoutError' - ); - } } /** diff --git a/src/sessions.ts b/src/sessions.ts index 8cce00697e..7290f9a7c4 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -16,7 +16,6 @@ import { MongoErrorLabel, MongoExpiredSessionError, MongoInvalidArgumentError, - MongoOperationTimeoutError, MongoRuntimeError, MongoServerError, MongoTransactionError, @@ -284,7 +283,7 @@ export class ClientSession } } catch (error) { // spec indicates that we should ignore all errors for `endSessions` - if (MongoOperationTimeoutError.is(error)) throw error; + if (error.name === 'MongoOperationTimeoutError') throw error; squashError(error); } finally { if (!this.hasEnded) { @@ -629,16 +628,20 @@ export class ClientSession } catch (firstAbortError) { this.unpin(); - if (options?.throwTimeout && MongoOperationTimeoutError.is(firstAbortError)) + if (firstAbortError.name === 'MongoRuntimeError') throw firstAbortError; + if (options?.throwTimeout && firstAbortError.name === 'MongoOperationTimeoutError') { throw firstAbortError; + } if (firstAbortError instanceof MongoError && isRetryableWriteError(firstAbortError)) { try { await executeOperation(this.client, operation, timeoutContext); return; } catch (secondAbortError) { - if (options?.throwTimeout && MongoOperationTimeoutError.is(secondAbortError)) + if (secondAbortError.name === 'MongoRuntimeError') throw secondAbortError; + if (options?.throwTimeout && secondAbortError.name === 'MongoOperationTimeoutError') { throw secondAbortError; + } // we do not retry the retry } } From 7413eeb817dca1c047629b640a2195b1bf6ecb20 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 26 Aug 2024 15:54:25 -0400 Subject: [PATCH 18/28] chore: limit test to 4.4+ --- .../client-side-operations-timeout/node_csot.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/integration/client-side-operations-timeout/node_csot.test.ts b/test/integration/client-side-operations-timeout/node_csot.test.ts index af7becb515..cd3e41c066 100644 --- a/test/integration/client-side-operations-timeout/node_csot.test.ts +++ b/test/integration/client-side-operations-timeout/node_csot.test.ts @@ -323,7 +323,9 @@ describe('CSOT driver tests', { requires: { mongodb: '>=4.4' } }, () => { }); describe('when using an explicit session', () => { - const metadata = { requires: { topology: ['replicaset'] } }; + const metadata: MongoDBMetadataUI = { + requires: { topology: ['replicaset'], mongodb: '>=4.4' } + }; describe('created for a withTransaction callback', () => { describe('passing a timeoutMS and a session with a timeoutContext', () => { From a3b04c93ab28aa3c94cf9406f269378ad2966c5d Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 26 Aug 2024 16:17:00 -0400 Subject: [PATCH 19/28] chore: assert approximate timeout was respected --- .../client_side_operations_timeout.prose.test.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts b/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts index 6fbcbacca9..3b98a20ab6 100644 --- a/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts +++ b/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts @@ -670,8 +670,11 @@ describe('CSOT spec prose tests', function () { const session = client.startSession(); session.startTransaction(); await coll.insertOne({ x: 1 }, { session }); + const start = performance.now(); const error = await session.endSession().catch(error => error); + const end = performance.now(); expect(error).to.be.instanceOf(MongoOperationTimeoutError); + expect(end - start).to.be.within(480, 520); }); }); @@ -682,8 +685,11 @@ describe('CSOT spec prose tests', function () { const session = client.startSession({ defaultTimeoutMS: 500 }); session.startTransaction(); await coll.insertOne({ x: 1 }, { session }); + const start = performance.now(); const error = await session.endSession().catch(error => error); + const end = performance.now(); expect(error).to.be.instanceOf(MongoOperationTimeoutError); + expect(end - start).to.be.within(480, 520); }); }); @@ -694,8 +700,11 @@ describe('CSOT spec prose tests', function () { const session = client.startSession(); session.startTransaction(); await coll.insertOne({ x: 1 }, { session }); + const start = performance.now(); const error = await session.endSession({ timeoutMS: 500 }).catch(error => error); + const end = performance.now(); expect(error).to.be.instanceOf(MongoOperationTimeoutError); + expect(end - start).to.be.within(480, 520); }); }); }); From 24edc0bd694470b255e2676ea01015aacc9cb1e1 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Mon, 26 Aug 2024 16:39:03 -0400 Subject: [PATCH 20/28] test: improve instanceof assertion logging --- test/tools/unified-spec-runner/match.ts | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/test/tools/unified-spec-runner/match.ts b/test/tools/unified-spec-runner/match.ts index 5c4ea000de..8cdcc765fc 100644 --- a/test/tools/unified-spec-runner/match.ts +++ b/test/tools/unified-spec-runner/match.ts @@ -501,6 +501,13 @@ function compareCommandFailedEvents( } } +function expectInstanceOf any>( + instance: any, + ctor: T +): asserts instance is InstanceType { + expect(instance).to.be.instanceOf(ctor); +} + function compareEvents( actual: CommandEvent[] | CmapEvent[] | SdamEvent[], expected: (ExpectedCommandEvent & ExpectedCmapEvent & ExpectedSdamEvent)[], @@ -515,9 +522,7 @@ function compareEvents( if (expectedEvent.commandStartedEvent) { const path = `${rootPrefix}.commandStartedEvent`; - if (!(actualEvent instanceof CommandStartedEvent)) { - expect.fail(`expected ${path} to be instanceof CommandStartedEvent`); - } + expectInstanceOf(actualEvent, CommandStartedEvent); compareCommandStartedEvents(actualEvent, expectedEvent.commandStartedEvent, entities, path); if (expectedEvent.commandStartedEvent.hasServerConnectionId) { expect(actualEvent).property('serverConnectionId').to.be.a('bigint'); @@ -526,9 +531,7 @@ function compareEvents( } } else if (expectedEvent.commandSucceededEvent) { const path = `${rootPrefix}.commandSucceededEvent`; - if (!(actualEvent instanceof CommandSucceededEvent)) { - expect.fail(`expected ${path} to be instanceof CommandSucceededEvent`); - } + expectInstanceOf(actualEvent, CommandSucceededEvent); compareCommandSucceededEvents( actualEvent, expectedEvent.commandSucceededEvent, @@ -542,9 +545,7 @@ function compareEvents( } } else if (expectedEvent.commandFailedEvent) { const path = `${rootPrefix}.commandFailedEvent`; - if (!(actualEvent instanceof CommandFailedEvent)) { - expect.fail(`expected ${path} to be instanceof CommandFailedEvent`); - } + expectInstanceOf(actualEvent, CommandFailedEvent); compareCommandFailedEvents(actualEvent, expectedEvent.commandFailedEvent, entities, path); if (expectedEvent.commandFailedEvent.hasServerConnectionId) { expect(actualEvent).property('serverConnectionId').to.be.a('bigint'); From 4ba842df7551fcf4c3d0b4b5db900a47e21503b0 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 27 Aug 2024 17:03:40 -0400 Subject: [PATCH 21/28] test: assert time first --- .../client_side_operations_timeout.prose.test.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts b/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts index 3b98a20ab6..477eede123 100644 --- a/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts +++ b/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts @@ -673,8 +673,8 @@ describe('CSOT spec prose tests', function () { const start = performance.now(); const error = await session.endSession().catch(error => error); const end = performance.now(); - expect(error).to.be.instanceOf(MongoOperationTimeoutError); expect(end - start).to.be.within(480, 520); + expect(error).to.be.instanceOf(MongoOperationTimeoutError); }); }); @@ -688,8 +688,8 @@ describe('CSOT spec prose tests', function () { const start = performance.now(); const error = await session.endSession().catch(error => error); const end = performance.now(); - expect(error).to.be.instanceOf(MongoOperationTimeoutError); expect(end - start).to.be.within(480, 520); + expect(error).to.be.instanceOf(MongoOperationTimeoutError); }); }); @@ -703,8 +703,8 @@ describe('CSOT spec prose tests', function () { const start = performance.now(); const error = await session.endSession({ timeoutMS: 500 }).catch(error => error); const end = performance.now(); - expect(error).to.be.instanceOf(MongoOperationTimeoutError); expect(end - start).to.be.within(480, 520); + expect(error).to.be.instanceOf(MongoOperationTimeoutError); }); }); }); From c45b166994e4d135146278d645064a08bc4ca4b4 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 29 Aug 2024 10:42:05 -0400 Subject: [PATCH 22/28] chore: skip test --- ...ient_side_operations_timeout.prose.test.ts | 17 ++-- ...lient_side_operations_timeout.spec.test.ts | 11 ++- .../node_csot.test.ts | 85 +++++++++++++++++++ 3 files changed, 107 insertions(+), 6 deletions(-) diff --git a/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts b/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts index 477eede123..e6eec4b600 100644 --- a/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts +++ b/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts @@ -774,13 +774,23 @@ describe('CSOT spec prose tests', function () { afterEach(async function () { if (semver.satisfies(this.configuration.version, '>=4.4')) { const internalClient = this.configuration.newClient(); - await internalClient.db('admin').command({ ...failpoint, mode: 'off' }); + await internalClient + .db('admin') + .command({ configureFailPoint: 'failCommand', mode: 'off' }); await internalClient.close(); } await client?.close(); }); it('timeoutMS is refreshed for abortTransaction', metadata, async function () { + if ( + this.configuration.topologyType === 'ReplicaSetWithPrimary' && + semver.satisfies(this.configuration.version, '<=4.4') + ) { + this.skipReason = '4.4 replicaset fail point does not blockConnection for requested time'; + this.skip(); + } + const commandsFailed = []; const commandsStarted = []; @@ -793,16 +803,13 @@ describe('CSOT spec prose tests', function () { const session = client.startSession(); - let insertError: Error | null = null; const withTransactionError = await session .withTransaction(async session => { - insertError = await coll.insertOne({ x: 1 }, { session }).catch(error => error); - throw insertError; + await coll.insertOne({ x: 1 }, { session }); }) .catch(error => error); try { - expect(insertError).to.be.instanceOf(MongoOperationTimeoutError); expect(withTransactionError).to.be.instanceOf(MongoOperationTimeoutError); expect(commandsStarted, 'commands started').to.deep.equal(['insert', 'abortTransaction']); expect(commandsFailed, 'commands failed').to.deep.equal(['insert', 'abortTransaction']); diff --git a/test/integration/client-side-operations-timeout/client_side_operations_timeout.spec.test.ts b/test/integration/client-side-operations-timeout/client_side_operations_timeout.spec.test.ts index b6f562a02e..43fd3fe272 100644 --- a/test/integration/client-side-operations-timeout/client_side_operations_timeout.spec.test.ts +++ b/test/integration/client-side-operations-timeout/client_side_operations_timeout.spec.test.ts @@ -1,4 +1,5 @@ import { join } from 'path'; +import * as semver from 'semver'; import { loadSpecTests } from '../../spec'; import { runUnifiedSuite } from '../../tools/unified-spec-runner/runner'; @@ -46,5 +47,13 @@ describe('CSOT spec tests', function () { 'TODO(NODE-6274): update test runner to check errorResponse field of MongoBulkWriteError in isTimeoutError assertion'; } } - runUnifiedSuite(specs); + runUnifiedSuite(specs, (test, configuration) => { + if ( + configuration.topologyType === 'ReplicaSetWithPrimary' && + semver.satisfies(configuration.version, '<=4.4') + ) { + return '4.4 replicaset fail point does not blockConnection for requested time'; + } + return false; + }); }); diff --git a/test/integration/client-side-operations-timeout/node_csot.test.ts b/test/integration/client-side-operations-timeout/node_csot.test.ts index cd3e41c066..cc767c1d80 100644 --- a/test/integration/client-side-operations-timeout/node_csot.test.ts +++ b/test/integration/client-side-operations-timeout/node_csot.test.ts @@ -385,4 +385,89 @@ describe('CSOT driver tests', { requires: { mongodb: '>=4.4' } }, () => { }); }); }); + + describe('Convenient Transactions', () => { + /** Tests in this section MUST only run against replica sets and sharded clusters with server versions 4.4 or higher. */ + const metadata: MongoDBMetadataUI = { + requires: { topology: ['replicaset', 'sharded'], mongodb: '>=5.0' } + }; + + describe('when an operation fails inside withTransaction callback', () => { + const failpoint: FailPoint = { + configureFailPoint: 'failCommand', + mode: { times: 2 }, + data: { + failCommands: ['insert', 'abortTransaction'], + blockConnection: true, + blockTimeMS: 600 + } + }; + + beforeEach(async function () { + if (!semver.satisfies(this.configuration.version, '>=4.4')) { + this.skipReason = 'Requires server version 4.4+'; + this.skip(); + } + const internalClient = this.configuration.newClient(); + await internalClient + .db('db') + .collection('coll') + .drop() + .catch(() => null); + await internalClient.db('admin').command(failpoint); + await internalClient.close(); + }); + + let client: MongoClient; + + afterEach(async function () { + if (semver.satisfies(this.configuration.version, '>=4.4')) { + const internalClient = this.configuration.newClient(); + await internalClient + .db('admin') + .command({ configureFailPoint: 'failCommand', mode: 'off' }); + await internalClient.close(); + } + await client?.close(); + }); + + it( + 'timeoutMS is refreshed for abortTransaction and the timeout error is thrown from the operation', + metadata, + async function () { + const commandsFailed = []; + const commandsStarted = []; + + client = this.configuration + .newClient({ timeoutMS: 500, monitorCommands: true }) + .on('commandStarted', e => commandsStarted.push(e.commandName)) + .on('commandFailed', e => commandsFailed.push(e.commandName)); + + const coll = client.db('db').collection('coll'); + + const session = client.startSession(); + + let insertError: Error | null = null; + const withTransactionError = await session + .withTransaction(async session => { + insertError = await coll.insertOne({ x: 1 }, { session }).catch(error => error); + throw insertError; + }) + .catch(error => error); + + try { + expect(insertError).to.be.instanceOf(MongoOperationTimeoutError); + expect(withTransactionError).to.be.instanceOf(MongoOperationTimeoutError); + expect(commandsStarted, 'commands started').to.deep.equal([ + 'insert', + 'abortTransaction' + ]); + expect(commandsFailed, 'commands failed').to.deep.equal(['insert', 'abortTransaction']); + } finally { + await session.endSession(); + } + } + ); + }); + }); }); From 8dc142f04c88649809690275e1e4b22ad5e9f574 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 29 Aug 2024 11:33:47 -0400 Subject: [PATCH 23/28] chore: include more operations in validation --- src/collection.ts | 12 ++++++++---- src/db.ts | 22 +++++++++++++++------- src/utils.ts | 4 ++++ 3 files changed, 27 insertions(+), 11 deletions(-) diff --git a/src/collection.ts b/src/collection.ts index d7dd7b43ac..3ad150ac76 100644 --- a/src/collection.ts +++ b/src/collection.ts @@ -469,10 +469,14 @@ export class Collection { // Intentionally, we do not inherit options from parent for this operation. return await executeOperation( this.client, - new RenameOperation(this as TODO_NODE_3286, newName, { - ...options, - readPreference: ReadPreference.PRIMARY - }) as TODO_NODE_3286 + new RenameOperation( + this as TODO_NODE_3286, + newName, + resolveOptions(undefined, { + ...options, + readPreference: ReadPreference.PRIMARY + }) + ) as TODO_NODE_3286 ); } diff --git a/src/db.ts b/src/db.ts index 07a0c928cc..cbb0eac13f 100644 --- a/src/db.ts +++ b/src/db.ts @@ -275,12 +275,16 @@ export class Db { // Intentionally, we do not inherit options from parent for this operation. return await executeOperation( this.client, - new RunCommandOperation(this, command, { - ...resolveBSONOptions(options), - timeoutMS: options?.timeoutMS ?? this.timeoutMS, - session: options?.session, - readPreference: options?.readPreference - }) + new RunCommandOperation( + this, + command, + resolveOptions(undefined, { + ...resolveBSONOptions(options), + timeoutMS: options?.timeoutMS ?? this.timeoutMS, + session: options?.session, + readPreference: options?.readPreference + }) + ) ); } @@ -385,7 +389,11 @@ export class Db { new RenameOperation( this.collection(fromCollection) as TODO_NODE_3286, toCollection, - { ...options, new_collection: true, readPreference: ReadPreference.primary } + resolveOptions(undefined, { + ...options, + new_collection: true, + readPreference: ReadPreference.primary + }) ) as TODO_NODE_3286 ); } diff --git a/src/utils.ts b/src/utils.ts index 49452835e3..e8a6052f01 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -501,6 +501,10 @@ export function hasAtomicOperators(doc: Document | Document[]): boolean { /** * Merge inherited properties from parent into options, prioritizing values from options, * then values from parent. + * + * @param parent - An optional owning class of the operation being run. ex. Db/Collection/MongoClient. + * @param options - The options passed to the operation method. + * * @internal */ export function resolveOptions( From 2e40d2bc184eb4df52089ee89a558c18fa4e118f Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 29 Aug 2024 11:37:32 -0400 Subject: [PATCH 24/28] chore: reduce condition for timeoutContext in txn --- src/sessions.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/sessions.ts b/src/sessions.ts index 7290f9a7c4..977c1afaf2 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -512,9 +512,8 @@ export class ClientSession ? this.timeoutMS : null; - const timeoutContext = this.timeoutContext?.csotEnabled() - ? this.timeoutContext - : TimeoutContext.create({ timeoutMS, ...this.clientOptions }); + const timeoutContext = + this.timeoutContext ?? TimeoutContext.create({ timeoutMS, ...this.clientOptions }); try { await executeOperation(this.client, operation, timeoutContext); From 63d95e155b0622d8d517ccc4f8f9a7968b591264 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 29 Aug 2024 11:57:05 -0400 Subject: [PATCH 25/28] chore: take session into create and remove csotEnabled condition --- src/operations/execute_operation.ts | 15 +++++++-------- src/sessions.ts | 9 ++++++++- src/timeout.ts | 6 +++++- .../client_side_operations_timeout.spec.test.ts | 2 ++ 4 files changed, 22 insertions(+), 10 deletions(-) diff --git a/src/operations/execute_operation.ts b/src/operations/execute_operation.ts index 18c70eec0d..4ec6d88459 100644 --- a/src/operations/execute_operation.ts +++ b/src/operations/execute_operation.ts @@ -59,7 +59,7 @@ type ResultTypeFromOperation = TOperation extends AbstractOperation< export async function executeOperation< T extends AbstractOperation, TResult = ResultTypeFromOperation ->(client: MongoClient, operation: T, timeoutContext?: TimeoutContext): Promise { +>(client: MongoClient, operation: T, timeoutContext?: TimeoutContext | null): Promise { if (!(operation instanceof AbstractOperation)) { // TODO(NODE-3483): Extend MongoRuntimeError throw new MongoRuntimeError('This method requires a valid operation instance'); @@ -102,13 +102,12 @@ export async function executeOperation< session.unpin(); } - timeoutContext ??= - session.timeoutContext ?? - TimeoutContext.create({ - serverSelectionTimeoutMS: client.s.options.serverSelectionTimeoutMS, - waitQueueTimeoutMS: client.s.options.waitQueueTimeoutMS, - timeoutMS: operation.options.timeoutMS ?? session.timeoutMS - }); + timeoutContext ??= TimeoutContext.create({ + session, + serverSelectionTimeoutMS: client.s.options.serverSelectionTimeoutMS, + waitQueueTimeoutMS: client.s.options.waitQueueTimeoutMS, + timeoutMS: operation.options.timeoutMS + }); try { return await tryOperation(operation, { diff --git a/src/sessions.ts b/src/sessions.ts index 977c1afaf2..5d923dde49 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -513,7 +513,14 @@ export class ClientSession : null; const timeoutContext = - this.timeoutContext ?? TimeoutContext.create({ timeoutMS, ...this.clientOptions }); + this.timeoutContext ?? + (typeof timeoutMS === 'number' + ? TimeoutContext.create({ + serverSelectionTimeoutMS: this.clientOptions.serverSelectionTimeoutMS, + socketTimeoutMS: this.clientOptions.socketTimeoutMS, + timeoutMS + }) + : null); try { await executeOperation(this.client, operation, timeoutContext); diff --git a/src/timeout.ts b/src/timeout.ts index d805f84194..6e2f92b5de 100644 --- a/src/timeout.ts +++ b/src/timeout.ts @@ -1,6 +1,7 @@ import { clearTimeout, setTimeout } from 'timers'; import { MongoInvalidArgumentError, MongoOperationTimeoutError, MongoRuntimeError } from './error'; +import { type ClientSession } from './sessions'; import { csotMin, noop } from './utils'; /** @internal */ @@ -128,7 +129,9 @@ export class Timeout extends Promise { } /** @internal */ -export type TimeoutContextOptions = LegacyTimeoutContextOptions | CSOTTimeoutContextOptions; +export type TimeoutContextOptions = (LegacyTimeoutContextOptions | CSOTTimeoutContextOptions) & { + session?: ClientSession; +}; /** @internal */ export type LegacyTimeoutContextOptions = { @@ -169,6 +172,7 @@ function isCSOTTimeoutContextOptions(v: unknown): v is CSOTTimeoutContextOptions /** @internal */ export abstract class TimeoutContext { static create(options: TimeoutContextOptions): TimeoutContext { + if (options.session?.timeoutContext != null) return options.session?.timeoutContext; if (isCSOTTimeoutContextOptions(options)) return new CSOTTimeoutContext(options); else if (isLegacyTimeoutContextOptions(options)) return new LegacyTimeoutContext(options); else throw new MongoRuntimeError('Unrecognized options'); diff --git a/test/integration/client-side-operations-timeout/client_side_operations_timeout.spec.test.ts b/test/integration/client-side-operations-timeout/client_side_operations_timeout.spec.test.ts index 43fd3fe272..a178cecc5d 100644 --- a/test/integration/client-side-operations-timeout/client_side_operations_timeout.spec.test.ts +++ b/test/integration/client-side-operations-timeout/client_side_operations_timeout.spec.test.ts @@ -48,7 +48,9 @@ describe('CSOT spec tests', function () { } } runUnifiedSuite(specs, (test, configuration) => { + const sessionCSOTTests = ['timeoutMS applied to withTransaction']; if ( + sessionCSOTTests.includes(test.description) && configuration.topologyType === 'ReplicaSetWithPrimary' && semver.satisfies(configuration.version, '<=4.4') ) { From b2096e93fa27344c3791a38dbaa9cb35b4bdaabf Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 29 Aug 2024 12:08:52 -0400 Subject: [PATCH 26/28] chore: adjust timeouts to match spec change --- ...ient_side_operations_timeout.prose.test.ts | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts b/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts index e6eec4b600..406aa53ed6 100644 --- a/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts +++ b/test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts @@ -636,7 +636,7 @@ describe('CSOT spec prose tests', function () { data: { failCommands: ['abortTransaction'], blockConnection: true, - blockTimeMS: 600 + blockTimeMS: 200 } }; @@ -665,7 +665,7 @@ describe('CSOT spec prose tests', function () { describe('when timeoutMS is provided to the client', () => { it('throws a timeout error from endSession', metadata, async function () { - client = this.configuration.newClient({ timeoutMS: 500, monitorCommands: true }); + client = this.configuration.newClient({ timeoutMS: 150, monitorCommands: true }); const coll = client.db('endSession_db').collection('endSession_coll'); const session = client.startSession(); session.startTransaction(); @@ -673,7 +673,7 @@ describe('CSOT spec prose tests', function () { const start = performance.now(); const error = await session.endSession().catch(error => error); const end = performance.now(); - expect(end - start).to.be.within(480, 520); + expect(end - start).to.be.within(100, 170); expect(error).to.be.instanceOf(MongoOperationTimeoutError); }); }); @@ -682,13 +682,13 @@ describe('CSOT spec prose tests', function () { it('throws a timeout error from endSession', metadata, async function () { client = this.configuration.newClient(); const coll = client.db('endSession_db').collection('endSession_coll'); - const session = client.startSession({ defaultTimeoutMS: 500 }); + const session = client.startSession({ defaultTimeoutMS: 150 }); session.startTransaction(); await coll.insertOne({ x: 1 }, { session }); const start = performance.now(); const error = await session.endSession().catch(error => error); const end = performance.now(); - expect(end - start).to.be.within(480, 520); + expect(end - start).to.be.within(100, 170); expect(error).to.be.instanceOf(MongoOperationTimeoutError); }); }); @@ -701,9 +701,9 @@ describe('CSOT spec prose tests', function () { session.startTransaction(); await coll.insertOne({ x: 1 }, { session }); const start = performance.now(); - const error = await session.endSession({ timeoutMS: 500 }).catch(error => error); + const error = await session.endSession({ timeoutMS: 150 }).catch(error => error); const end = performance.now(); - expect(end - start).to.be.within(480, 520); + expect(end - start).to.be.within(100, 170); expect(error).to.be.instanceOf(MongoOperationTimeoutError); }); }); @@ -726,7 +726,7 @@ describe('CSOT spec prose tests', function () { * data: { * failCommands: ["insert", "abortTransaction"], * blockConnection: true, - * blockTimeMS: 15 + * blockTimeMS: 200 * } * } * ``` @@ -750,7 +750,7 @@ describe('CSOT spec prose tests', function () { data: { failCommands: ['insert', 'abortTransaction'], blockConnection: true, - blockTimeMS: 600 + blockTimeMS: 200 } }; @@ -795,7 +795,7 @@ describe('CSOT spec prose tests', function () { const commandsStarted = []; client = this.configuration - .newClient({ timeoutMS: 500, monitorCommands: true }) + .newClient({ timeoutMS: 150, monitorCommands: true }) .on('commandStarted', e => commandsStarted.push(e.commandName)) .on('commandFailed', e => commandsFailed.push(e.commandName)); From 89304811577a5d1c8d946ca745cacc436882317e Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 3 Sep 2024 15:34:11 -0400 Subject: [PATCH 27/28] fix: typescript --- src/sessions.ts | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/sessions.ts b/src/sessions.ts index 5d923dde49..bbd1785275 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -625,7 +625,14 @@ export class ClientSession ? this.timeoutMS : null; - const timeoutContext = TimeoutContext.create({ timeoutMS, ...this.clientOptions }); + const timeoutContext = + timeoutMS != null + ? TimeoutContext.create({ + timeoutMS, + serverSelectionTimeoutMS: this.clientOptions.serverSelectionTimeoutMS, + socketTimeoutMS: this.clientOptions.socketTimeoutMS + }) + : null; try { await executeOperation(this.client, operation, timeoutContext); @@ -715,11 +722,13 @@ export class ClientSession ): Promise { const MAX_TIMEOUT = 120000; + const timeoutMS = options?.timeoutMS ?? this.timeoutMS ?? null; this.timeoutContext = - typeof options?.timeoutMS === 'number' || typeof this.timeoutMS === 'number' + timeoutMS != null ? TimeoutContext.create({ - timeoutMS: options?.timeoutMS ?? this.timeoutMS, - ...this.clientOptions + timeoutMS, + serverSelectionTimeoutMS: this.clientOptions.serverSelectionTimeoutMS, + socketTimeoutMS: this.clientOptions.socketTimeoutMS }) : null; From d6fd82e28b5250798313eef2a0a285e15dcffd9f Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 6 Sep 2024 13:13:50 -0400 Subject: [PATCH 28/28] chore: name the variable --- src/utils.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/utils.ts b/src/utils.ts index e8a6052f01..bf635b5628 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -532,7 +532,8 @@ export function resolveOptions( result.readPreference = readPreference; } - if (session?.explicit && session?.timeoutContext != null && options?.timeoutMS != null) { + const isConvenientTransaction = session?.explicit && session?.timeoutContext != null; + if (isConvenientTransaction && options?.timeoutMS != null) { throw new MongoInvalidArgumentError( 'An operation cannot be given a timeoutMS setting when inside a withTransaction call that has a timeoutMS setting' );