From 8f7c72a8f8fad6df703c2ed7f1533af8bb37e378 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Tue, 15 Mar 2022 14:58:33 -0400 Subject: [PATCH 01/14] feat(NODE-3697): reduce serverSession allocation --- src/cmap/connection.ts | 2 +- src/operations/operation.ts | 5 +- src/sessions.ts | 106 +++++++++++------- .../sessions/sessions.spec.prose.test.ts | 50 +++++++++ test/integration/sessions/sessions.test.ts | 33 ++++++ test/tools/cluster_setup.sh | 2 +- test/tools/spec-runner/index.js | 2 + test/unit/sessions.test.js | 97 +++++++++++++++- 8 files changed, 247 insertions(+), 50 deletions(-) create mode 100644 test/integration/sessions/sessions.spec.prose.test.ts diff --git a/src/cmap/connection.ts b/src/cmap/connection.ts index e5665e9116..c15d0fadfb 100644 --- a/src/cmap/connection.ts +++ b/src/cmap/connection.ts @@ -114,7 +114,7 @@ export interface CommandOptions extends BSONSerializeOptions { // Applying a session to a command should happen as part of command construction, // most likely in the CommandOperation#executeCommand method, where we have access to // the details we need to determine if a txnNum should also be applied. - willRetryWrite?: true; + willRetryWrite?: boolean; writeConcern?: WriteConcern; } diff --git a/src/operations/operation.ts b/src/operations/operation.ts index 82265d06f3..e21a87585c 100644 --- a/src/operations/operation.ts +++ b/src/operations/operation.ts @@ -25,7 +25,7 @@ export interface OperationConstructor extends Function { export interface OperationOptions extends BSONSerializeOptions { /** Specify ClientSession for this command */ session?: ClientSession; - willRetryWrites?: boolean; + willRetryWrite?: boolean; /** The preferred read preference (ReadPreference.primary, ReadPreference.primary_preferred, ReadPreference.secondary, ReadPreference.secondary_preferred, ReadPreference.nearest). */ readPreference?: ReadPreferenceLike; @@ -56,8 +56,7 @@ export abstract class AbstractOperation { // BSON serialization options bsonOptions?: BSONSerializeOptions; - // TODO: Each operation defines its own options, there should be better typing here - options: Document; + options: OperationOptions; [kSession]: ClientSession | undefined; diff --git a/src/sessions.ts b/src/sessions.ts index 7b05edee40..015f2b4007 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -42,20 +42,6 @@ import { const minWireVersionForShardedTransactions = 8; -function assertAlive(session: ClientSession, callback?: Callback): boolean { - if (session.serverSession == null) { - const error = new MongoExpiredSessionError(); - if (typeof callback === 'function') { - callback(error); - return false; - } - - throw error; - } - - return true; -} - /** @public */ export interface ClientSessionOptions { /** Whether causal consistency should be enabled on this session */ @@ -89,6 +75,8 @@ const kSnapshotTime = Symbol('snapshotTime'); const kSnapshotEnabled = Symbol('snapshotEnabled'); /** @internal */ const kPinnedConnection = Symbol('pinnedConnection'); +/** @internal Accumulates total number of increments to perform to txnNumber */ +const kTxnNumberIncrement = Symbol('txnNumberIncrement'); /** @public */ export interface EndSessionOptions { @@ -130,6 +118,8 @@ export class ClientSession extends TypedEventEmitter { [kSnapshotEnabled] = false; /** @internal */ [kPinnedConnection]?: Connection; + /** @internal Accumulates total number of increments to perform to txnNumber */ + [kTxnNumberIncrement]: number; /** * Create a client session. @@ -172,7 +162,10 @@ export class ClientSession extends TypedEventEmitter { this.sessionPool = sessionPool; this.hasEnded = false; this.clientOptions = clientOptions; - this[kServerSession] = undefined; + + this.explicit = Boolean(options.explicit); + this[kServerSession] = this.explicit ? this.sessionPool.acquire() : undefined; + this[kTxnNumberIncrement] = 0; this.supports = { causalConsistency: options.snapshot !== true && options.causalConsistency !== false @@ -181,7 +174,6 @@ export class ClientSession extends TypedEventEmitter { this.clusterTime = options.initialClusterTime; this.operationTime = undefined; - this.explicit = !!options.explicit; this.owner = options.owner; this.defaultTransactionOptions = Object.assign({}, options.defaultTransactionOptions); this.transaction = new Transaction(); @@ -189,16 +181,20 @@ export class ClientSession extends TypedEventEmitter { /** The server id associated with this session */ get id(): ServerSessionId | undefined { - return this.serverSession?.id; + const serverSession = this[kServerSession]; + if (serverSession == null) { + return undefined; + } + return serverSession.id; } get serverSession(): ServerSession { - if (this[kServerSession] == null) { - this[kServerSession] = this.sessionPool.acquire(); + let serverSession = this[kServerSession]; + if (serverSession == null) { + serverSession = this.sessionPool.acquire(); + this[kServerSession] = serverSession; } - - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - return this[kServerSession]!; + return serverSession; } /** Whether or not this session is configured for snapshot reads */ @@ -267,9 +263,15 @@ export class ClientSession extends TypedEventEmitter { const completeEndSession = () => { maybeClearPinnedConnection(this, finalOptions); - // release the server session back to the pool - this.sessionPool.release(this.serverSession); - this[kServerSession] = undefined; + const serverSession = this[kServerSession]; + if (serverSession != null) { + // release the server session back to the pool + this.sessionPool.release(serverSession); + // Make sure a new serverSession never makes it on to the ClientSession + Object.defineProperty(this, kServerSession, { + value: ServerSession.clone(serverSession) + }); + } // mark the session as ended, and emit a signal this.hasEnded = true; @@ -279,7 +281,9 @@ export class ClientSession extends TypedEventEmitter { done(); }; - if (this.serverSession && this.inTransaction()) { + if (this.inTransaction()) { + // If we've reached endSession and the transaction is still active + // by default we abort it this.abortTransaction(err => { if (err) return done(err); completeEndSession(); @@ -355,10 +359,7 @@ export class ClientSession extends TypedEventEmitter { /** Increment the transaction number on the internal ServerSession */ incrementTransactionNumber(): void { - if (this.serverSession) { - this.serverSession.txnNumber = - typeof this.serverSession.txnNumber === 'number' ? this.serverSession.txnNumber + 1 : 0; - } + this[kTxnNumberIncrement] += 1; } /** @returns whether this session is currently in a transaction or not */ @@ -376,7 +377,6 @@ export class ClientSession extends TypedEventEmitter { throw new MongoCompatibilityError('Transactions are not allowed with snapshot sessions'); } - assertAlive(this); if (this.inTransaction()) { throw new MongoTransactionError('Transaction already in progress'); } @@ -627,7 +627,7 @@ function attemptTransaction( throw err; } - if (session.transaction.isActive) { + if (session.inTransaction()) { return session.abortTransaction().then(() => maybeRetryOrThrow(err)); } @@ -641,11 +641,6 @@ function endTransaction( commandName: 'abortTransaction' | 'commitTransaction', callback: Callback ) { - if (!assertAlive(session, callback)) { - // checking result in case callback was called - return; - } - // handle any initial problematic cases const txnState = session.transaction.state; @@ -750,7 +745,6 @@ function endTransaction( callback(error, result); } - // Assumption here that commandName is "commitTransaction" or "abortTransaction" if (session.transaction.recoveryToken) { command.recoveryToken = session.transaction.recoveryToken; } @@ -832,6 +826,30 @@ export class ServerSession { return idleTimeMinutes > sessionTimeoutMinutes - 1; } + + /** + * @internal + * Cloning meant to keep a readable reference to the server session data + * after ClientSession has ended + */ + static clone(serverSession: ServerSession): Readonly { + const arrayBuffer = new ArrayBuffer(16); + const idBytes = Buffer.from(arrayBuffer); + idBytes.set(serverSession.id.id.buffer); + + const id = new Binary(idBytes, serverSession.id.id.sub_type); + + // Manual prototype construction to avoid modifying the constructor of this class + return Object.setPrototypeOf( + { + id: { id }, + lastUse: serverSession.lastUse, + txnNumber: serverSession.txnNumber, + isDirty: serverSession.isDirty + }, + ServerSession.prototype + ); + } } /** @@ -944,11 +962,11 @@ export function applySession( command: Document, options: CommandOptions ): MongoDriverError | undefined { - // TODO: merge this with `assertAlive`, did not want to throw a try/catch here if (session.hasEnded) { return new MongoExpiredSessionError(); } + // May acquire serverSession here const serverSession = session.serverSession; if (serverSession == null) { return new MongoRuntimeError('Unable to acquire server session'); @@ -967,14 +985,16 @@ export function applySession( command.lsid = serverSession.id; // first apply non-transaction-specific sessions data - const inTransaction = session.inTransaction() || isTransactionCommand(command); - const isRetryableWrite = options?.willRetryWrite || false; + const inTxnOrTxnCommand = session.inTransaction() || isTransactionCommand(command); + const isRetryableWrite = Boolean(options.willRetryWrite); - if (serverSession.txnNumber && (isRetryableWrite || inTransaction)) { + if (isRetryableWrite || inTxnOrTxnCommand) { + serverSession.txnNumber += session[kTxnNumberIncrement]; + session[kTxnNumberIncrement] = 0; command.txnNumber = Long.fromNumber(serverSession.txnNumber); } - if (!inTransaction) { + if (!inTxnOrTxnCommand) { if (session.transaction.state !== TxnState.NO_TRANSACTION) { session.transaction.transition(TxnState.NO_TRANSACTION); } diff --git a/test/integration/sessions/sessions.spec.prose.test.ts b/test/integration/sessions/sessions.spec.prose.test.ts new file mode 100644 index 0000000000..33bf24d54b --- /dev/null +++ b/test/integration/sessions/sessions.spec.prose.test.ts @@ -0,0 +1,50 @@ +import { expect } from 'chai'; + +import { Collection } from '../../../src/index'; + +describe('ServerSession', () => { + let client; + let testCollection: Collection<{ _id: number; a?: number }>; + beforeEach(async function () { + const configuration = this.configuration; + client = await configuration.newClient({ maxPoolSize: 1, monitorCommands: true }).connect(); + + // reset test collection + testCollection = client.db('test').collection('too.many.sessions'); + await testCollection.drop().catch(() => null); + }); + + afterEach(async () => { + await client?.close(true); + }); + + /** + * TODO(DRIVERS-2218): Refactor tests to align exactly with spec wording. Preliminarily implements: + * Drivers MAY assert that exactly one session is used for all the concurrent operations listed in the test, however this is a race condition if the session isn't released before checkIn (which SHOULD NOT be attempted) + * Drivers SHOULD assert that after repeated runs they are able to achieve the use of exactly one session, this will statistically prove we've reduced the allocation amount + * Drivers MUST assert that the number of allocated sessions never exceeds the number of concurrent operations executing + */ + + it('13. may reuse one server session for many operations', async () => { + const events = []; + client.on('commandStarted', ev => events.push(ev)); + + const operations = [ + testCollection.insertOne({ _id: 1 }), + testCollection.deleteOne({ _id: 2 }), + testCollection.updateOne({ _id: 3 }, { $set: { a: 1 } }), + testCollection.bulkWrite([{ updateOne: { filter: { _id: 4 }, update: { $set: { a: 1 } } } }]), + testCollection.findOneAndDelete({ _id: 5 }), + testCollection.findOneAndUpdate({ _id: 6 }, { $set: { a: 1 } }), + testCollection.findOneAndReplace({ _id: 7 }, { a: 8 }), + testCollection.find().toArray() + ]; + + const allResults = await Promise.all(operations); + + expect(allResults).to.have.lengthOf(operations.length); + expect(events).to.have.lengthOf(operations.length); + + expect(new Set(events.map(ev => ev.command.lsid.id.toString('hex'))).size).to.equal(1); // This is a guarantee in node + }); +}); diff --git a/test/integration/sessions/sessions.test.ts b/test/integration/sessions/sessions.test.ts index 9d365812b2..5ef7ff3c22 100644 --- a/test/integration/sessions/sessions.test.ts +++ b/test/integration/sessions/sessions.test.ts @@ -367,4 +367,37 @@ describe('Sessions Spec', function () { }); }); }); + + describe('Session allocation', () => { + let client; + let testCollection; + + beforeEach(async function () { + client = await this.configuration + .newClient({ maxPoolSize: 1, monitorCommands: true }) + .connect(); + // reset test collection + testCollection = client.db('test').collection('too.many.sessions'); + await testCollection.drop().catch(() => null); + }); + + afterEach(async () => { + await client?.close(); + }); + + it('should only use one session for many operations when maxPoolSize is 1', async () => { + const documents = new Array(50).fill(null).map((_, idx) => ({ _id: idx })); + + const events = []; + client.on('commandStarted', ev => events.push(ev)); + const allResults = await Promise.all( + documents.map(async doc => testCollection.insertOne(doc)) + ); + + expect(allResults).to.have.lengthOf(documents.length); + expect(events).to.have.lengthOf(documents.length); + + expect(new Set(events.map(ev => ev.command.lsid.id.toString('hex'))).size).to.equal(1); + }); + }); }); diff --git a/test/tools/cluster_setup.sh b/test/tools/cluster_setup.sh index 44e8704278..3e665e3867 100755 --- a/test/tools/cluster_setup.sh +++ b/test/tools/cluster_setup.sh @@ -17,7 +17,7 @@ if [[ $1 == "replica_set" ]]; then echo "mongodb://bob:pwd123@localhost:31000,localhost:31001,localhost:31002/?replicaSet=rs" elif [[ $1 == "sharded_cluster" ]]; then mkdir -p $SHARDED_DIR - mlaunch init --dir $SHARDED_DIR --auth --username "bob" --password "pwd123" --replicaset --nodes 3 --arbiter --name rs --port 51000 --enableMajorityReadConcern --setParameter enableTestCommands=1 --sharded 1 --mongos 2 + mlaunch init --dir $SHARDED_DIR --auth --username "bob" --password "pwd123" --replicaset --nodes 3 --name rs --port 51000 --enableMajorityReadConcern --setParameter enableTestCommands=1 --sharded 1 --mongos 2 echo "mongodb://bob:pwd123@localhost:51000,localhost:51001" elif [[ $1 == "server" ]]; then mkdir -p $SINGLE_DIR diff --git a/test/tools/spec-runner/index.js b/test/tools/spec-runner/index.js index c9878a6a4a..2039125492 100644 --- a/test/tools/spec-runner/index.js +++ b/test/tools/spec-runner/index.js @@ -459,6 +459,8 @@ function validateExpectations(commandEvents, spec, savedSessionData) { const rawExpectedEvents = spec.expectations.map(x => x.command_started_event); const expectedEvents = normalizeCommandShapes(rawExpectedEvents); + expect(actualEvents).to.have.lengthOf(expectedEvents.length); + for (const [idx, expectedEvent] of expectedEvents.entries()) { const actualEvent = actualEvents[idx]; diff --git a/test/unit/sessions.test.js b/test/unit/sessions.test.js index 45ed170799..109745b54c 100644 --- a/test/unit/sessions.test.js +++ b/test/unit/sessions.test.js @@ -4,12 +4,19 @@ const mock = require('../tools/mongodb-mock/index'); const { expect } = require('chai'); const { genClusterTime, sessionCleanupHandler } = require('../tools/common'); const { Topology } = require('../../src/sdam/topology'); -const { ServerSessionPool, ServerSession, ClientSession } = require('../../src/sessions'); +const { + ServerSessionPool, + ServerSession, + ClientSession, + applySession +} = require('../../src/sessions'); const { now, isHello } = require('../../src/utils'); +const { getSymbolFrom } = require('../tools/utils'); +const { Long } = require('../../src/bson'); let test = {}; -describe('Sessions - unit/core', function () { +describe('Sessions - unit', function () { describe('ClientSession', function () { let session; let sessionPool; @@ -311,4 +318,90 @@ describe('Sessions - unit/core', function () { done(); }); }); + + describe('ServerSession allocation behavior', () => { + let serverSessionPool; + let topology; + + beforeEach(() => { + topology = {}; // we don't need a real topology, just a truthy value + serverSessionPool = new ServerSessionPool(topology); + }); + + it('should acquire a serverSession in the constructor if the session is explicit', () => { + const session = new ClientSession(topology, serverSessionPool, { explicit: true }); + const serverSessionSymbol = getSymbolFrom(session, 'serverSession'); + expect(session).to.have.property(serverSessionSymbol).that.is.an.instanceOf(ServerSession); + }); + + it('should leave serverSession null if the session is implicit', () => { + // implicit via false (this should not be allowed...) + let session = new ClientSession(topology, serverSessionPool, { explicit: false }); + const serverSessionSymbol = getSymbolFrom(session, 'serverSession'); + expect(session).to.have.property(serverSessionSymbol, undefined); + // implicit via omission + session = new ClientSession(topology, serverSessionPool, {}); + expect(session).to.have.property(serverSessionSymbol, undefined); + }); + + it('should start the txnNumberIncrement at zero', () => { + const session = new ClientSession(topology, serverSessionPool); + const txnNumberIncrementSymbol = getSymbolFrom(session, 'txnNumberIncrement'); + expect(session).to.have.property(txnNumberIncrementSymbol, 0); + }); + + it('incrementTransactionNumber should not allocate serverSession', () => { + const session = new ClientSession(topology, serverSessionPool); + const txnNumberIncrementSymbol = getSymbolFrom(session, 'txnNumberIncrement'); + + session.incrementTransactionNumber(); + expect(session).to.have.property(txnNumberIncrementSymbol, 1); + + const serverSessionSymbol = getSymbolFrom(session, 'serverSession'); + expect(session).to.have.property(serverSessionSymbol, undefined); + }); + it('incrementTransactionNumber should save increments', () => { + const session = new ClientSession(topology, serverSessionPool); + const txnNumberIncrementSymbol = getSymbolFrom(session, 'txnNumberIncrement'); + + session.incrementTransactionNumber(); + session.incrementTransactionNumber(); + session.incrementTransactionNumber(); + + expect(session).to.have.property(txnNumberIncrementSymbol, 3); + }); + + it('applySession should allocate serverSession', () => { + const session = new ClientSession(topology, serverSessionPool); + const serverSessionSymbol = getSymbolFrom(session, 'serverSession'); + + const command = { magic: 1 }; + const result = applySession(session, command, {}); + + expect(result).to.not.exist; + expect(command).to.have.property('lsid'); + expect(session).to.have.property(serverSessionSymbol).that.is.instanceOf(ServerSession); + }); + + it('applySession should apply saved txnNumberIncrements', () => { + const session = new ClientSession(topology, serverSessionPool); + const serverSessionSymbol = getSymbolFrom(session, 'serverSession'); + + session.incrementTransactionNumber(); + session.incrementTransactionNumber(); + session.incrementTransactionNumber(); + + const command = { magic: 1 }; + const result = applySession(session, command, { + // txnNumber will be applied for retryable write command + willRetryWrite: true + }); + + expect(result).to.not.exist; + expect(command).to.have.property('lsid'); + expect(command).to.have.property('txnNumber').instanceOf(Long); + expect(command.txnNumber.equals(Long.fromNumber(3))); + expect(session).to.have.property(serverSessionSymbol).that.is.instanceOf(ServerSession); + }); + }); }); From 6bf7b5337f65a49f6c6c5e9982909419cbbe0c0f Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 16 Mar 2022 14:45:06 -0400 Subject: [PATCH 02/14] wip --- test/integration/sessions/sessions.spec.prose.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/sessions/sessions.spec.prose.test.ts b/test/integration/sessions/sessions.spec.prose.test.ts index 33bf24d54b..4dff4c814f 100644 --- a/test/integration/sessions/sessions.spec.prose.test.ts +++ b/test/integration/sessions/sessions.spec.prose.test.ts @@ -24,7 +24,6 @@ describe('ServerSession', () => { * Drivers SHOULD assert that after repeated runs they are able to achieve the use of exactly one session, this will statistically prove we've reduced the allocation amount * Drivers MUST assert that the number of allocated sessions never exceeds the number of concurrent operations executing */ - it('13. may reuse one server session for many operations', async () => { const events = []; client.on('commandStarted', ev => events.push(ev)); @@ -45,6 +44,7 @@ describe('ServerSession', () => { expect(allResults).to.have.lengthOf(operations.length); expect(events).to.have.lengthOf(operations.length); - expect(new Set(events.map(ev => ev.command.lsid.id.toString('hex'))).size).to.equal(1); // This is a guarantee in node + // This is a guarantee in node, unless you are performing a transaction (which is not being done in this test) + expect(new Set(events.map(ev => ev.command.lsid.id.toString('hex'))).size).to.equal(1); }); }); From 02464c820c9b3c3ae8837c0398427a4c9f4acbd6 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 16 Mar 2022 16:08:19 -0400 Subject: [PATCH 03/14] fix: hasEnded logic and test --- src/sessions.ts | 8 +++++++- test/integration/sessions/sessions.test.ts | 2 +- test/unit/sessions.test.js | 10 ++++++++++ 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/sessions.ts b/src/sessions.ts index 015f2b4007..c238942b45 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -189,6 +189,13 @@ export class ClientSession extends TypedEventEmitter { } get serverSession(): ServerSession { + if (this.hasEnded) { + // @ts-expect-error: If the session has ended we do not want to run the acquire code below + // regardless of the value of kServerSession potentially being nullish. It *should* always be + // a ServerSession at this stage, but if it is not, risking a null access seems worth it + // as opposed to accidentally acquiring a new ServerSession for an ended ClientSession + return this[kServerSession]; + } let serverSession = this[kServerSession]; if (serverSession == null) { serverSession = this.sessionPool.acquire(); @@ -984,7 +991,6 @@ export function applySession( serverSession.lastUse = now(); command.lsid = serverSession.id; - // first apply non-transaction-specific sessions data const inTxnOrTxnCommand = session.inTransaction() || isTransactionCommand(command); const isRetryableWrite = Boolean(options.willRetryWrite); diff --git a/test/integration/sessions/sessions.test.ts b/test/integration/sessions/sessions.test.ts index 5ef7ff3c22..96cc6703fd 100644 --- a/test/integration/sessions/sessions.test.ts +++ b/test/integration/sessions/sessions.test.ts @@ -386,7 +386,7 @@ describe('Sessions Spec', function () { }); it('should only use one session for many operations when maxPoolSize is 1', async () => { - const documents = new Array(50).fill(null).map((_, idx) => ({ _id: idx })); + const documents = Array.from({ length: 50 }).map((_, idx) => ({ _id: idx })); const events = []; client.on('commandStarted', ev => events.push(ev)); diff --git a/test/unit/sessions.test.js b/test/unit/sessions.test.js index 109745b54c..4d31da4fd5 100644 --- a/test/unit/sessions.test.js +++ b/test/unit/sessions.test.js @@ -328,6 +328,16 @@ describe('Sessions - unit', function () { serverSessionPool = new ServerSessionPool(topology); }); + it('serverSession getter should return whatever is defined for serverSession symbol if clientSession is ended', () => { + const session = new ClientSession(topology, serverSessionPool, { explicit: false }); + const serverSessionSymbol = getSymbolFrom(session, 'serverSession'); + expect(session).to.have.property(serverSessionSymbol, undefined); + session.hasEnded = true; + expect(session.serverSession).to.be.undefined; + session[serverSessionSymbol] = 'wacky crazy value'; + expect(session.serverSession).to.be.equal('wacky crazy value'); + }); + it('should acquire a serverSession in the constructor if the session is explicit', () => { const session = new ClientSession(topology, serverSessionPool, { explicit: true }); const serverSessionSymbol = getSymbolFrom(session, 'serverSession'); From a417d0027b14b09961c076edd728d2646f4bd200 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 23 Mar 2022 10:37:12 -0400 Subject: [PATCH 04/14] some comments --- src/sessions.ts | 17 ++++++++++------- .../sessions/sessions.spec.prose.test.ts | 9 +++++---- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/src/sessions.ts b/src/sessions.ts index c238942b45..7a691231be 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -118,7 +118,7 @@ export class ClientSession extends TypedEventEmitter { [kSnapshotEnabled] = false; /** @internal */ [kPinnedConnection]?: Connection; - /** @internal Accumulates total number of increments to perform to txnNumber */ + /** @internal Accumulates total number of increments to add to txnNumber when applying session to command */ [kTxnNumberIncrement]: number; /** @@ -181,11 +181,7 @@ export class ClientSession extends TypedEventEmitter { /** The server id associated with this session */ get id(): ServerSessionId | undefined { - const serverSession = this[kServerSession]; - if (serverSession == null) { - return undefined; - } - return serverSession.id; + return this[kServerSession]?.id; } get serverSession(): ServerSession { @@ -364,7 +360,14 @@ export class ClientSession extends TypedEventEmitter { return this.id.id.buffer.equals(session.id.id.buffer); } - /** Increment the transaction number on the internal ServerSession */ + /** + * Increment the transaction number on the internal ServerSession + * + * @privateRemarks + * This helper increments a value stored on the client session that will be + * added to the serverSession's txnNumber upon applying it to a command. + * This is because the serverSession is lazily acquired after a connection is obtained + */ incrementTransactionNumber(): void { this[kTxnNumberIncrement] += 1; } diff --git a/test/integration/sessions/sessions.spec.prose.test.ts b/test/integration/sessions/sessions.spec.prose.test.ts index 4dff4c814f..a2bfbd958f 100644 --- a/test/integration/sessions/sessions.spec.prose.test.ts +++ b/test/integration/sessions/sessions.spec.prose.test.ts @@ -19,10 +19,11 @@ describe('ServerSession', () => { }); /** - * TODO(DRIVERS-2218): Refactor tests to align exactly with spec wording. Preliminarily implements: - * Drivers MAY assert that exactly one session is used for all the concurrent operations listed in the test, however this is a race condition if the session isn't released before checkIn (which SHOULD NOT be attempted) - * Drivers SHOULD assert that after repeated runs they are able to achieve the use of exactly one session, this will statistically prove we've reduced the allocation amount - * Drivers MUST assert that the number of allocated sessions never exceeds the number of concurrent operations executing + * TODO(NODE-4082): Refactor tests to align exactly with spec wording. + * Assert the following across at least 5 retries of the above test: (We do not need to retry in nodejs) + * Drivers MUST assert that exactly one session is used for all operations at least once across the retries of this test. + * Note that it's possible, although rare, for greater than 1 server session to be used because the session is not released until after the connection is checked in. + * Drivers MUST assert that the number of allocated sessions is strictly less than the number of concurrent operations in every retry of this test. In this instance it would less than (but NOT equal to) 8. */ it('13. may reuse one server session for many operations', async () => { const events = []; From 522408b99ea1f616578cf91e21e348a4105a741e Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Wed, 23 Mar 2022 11:07:30 -0400 Subject: [PATCH 05/14] reorganize unit tests and add hasEnded false test --- test/unit/sessions.test.js | 283 +++++++++++++++++++------------------ 1 file changed, 148 insertions(+), 135 deletions(-) diff --git a/test/unit/sessions.test.js b/test/unit/sessions.test.js index 4d31da4fd5..2800d82d88 100644 --- a/test/unit/sessions.test.js +++ b/test/unit/sessions.test.js @@ -17,10 +17,18 @@ const { Long } = require('../../src/bson'); let test = {}; describe('Sessions - unit', function () { - describe('ClientSession', function () { + describe('class ClientSession', function () { let session; let sessionPool; + let serverSessionPool; + let topology; + + beforeEach(() => { + topology = {}; // we don't need a real topology, just a truthy value + serverSessionPool = new ServerSessionPool(topology); + }); + afterEach(done => { if (sessionPool) { sessionCleanupHandler(session, sessionPool, done)(); @@ -29,43 +37,6 @@ describe('Sessions - unit', function () { } }); - it('should throw errors with invalid parameters', function () { - expect(() => { - new ClientSession(); - }).to.throw(/ClientSession requires a topology/); - - expect(() => { - new ClientSession({}); - }).to.throw(/ClientSession requires a ServerSessionPool/); - - expect(() => { - new ClientSession({}, {}); - }).to.throw(/ClientSession requires a ServerSessionPool/); - }); - - it('should throw an error if snapshot and causalConsistency options are both set to true', function () { - const client = new Topology('localhost:27017', {}); - sessionPool = client.s.sessionPool; - expect( - () => new ClientSession(client, sessionPool, { causalConsistency: true, snapshot: true }) - ).to.throw('Properties "causalConsistency" and "snapshot" are mutually exclusive'); - }); - - it('should default to `null` for `clusterTime`', function () { - const client = new Topology('localhost:27017', {}); - sessionPool = client.s.sessionPool; - session = new ClientSession(client, sessionPool); - expect(session.clusterTime).to.not.exist; - }); - - it('should set the internal clusterTime to `initialClusterTime` if provided', function () { - const clusterTime = genClusterTime(Date.now()); - const client = new Topology('localhost:27017'); - sessionPool = client.s.sessionPool; - session = new ClientSession(client, sessionPool, { initialClusterTime: clusterTime }); - expect(session.clusterTime).to.eql(clusterTime); - }); - describe('startTransaction()', () => { it('should throw an error if the session is snapshot enabled', function () { const client = new Topology('localhost:27017', {}); @@ -188,9 +159,147 @@ describe('Sessions - unit', function () { expect(session).property('clusterTime').to.equal(validInitialTime); }); }); + + describe('new ClientSession()', () => { + it('should throw errors with invalid parameters', function () { + expect(() => { + new ClientSession(); + }).to.throw(/ClientSession requires a topology/); + + expect(() => { + new ClientSession({}); + }).to.throw(/ClientSession requires a ServerSessionPool/); + + expect(() => { + new ClientSession({}, {}); + }).to.throw(/ClientSession requires a ServerSessionPool/); + }); + + it('should throw an error if snapshot and causalConsistency options are both set to true', function () { + const client = new Topology('localhost:27017', {}); + sessionPool = client.s.sessionPool; + expect( + () => new ClientSession(client, sessionPool, { causalConsistency: true, snapshot: true }) + ).to.throw('Properties "causalConsistency" and "snapshot" are mutually exclusive'); + }); + + it('should default to `null` for `clusterTime`', function () { + const client = new Topology('localhost:27017', {}); + sessionPool = client.s.sessionPool; + session = new ClientSession(client, sessionPool); + expect(session.clusterTime).to.not.exist; + }); + + it('should set the internal clusterTime to `initialClusterTime` if provided', function () { + const clusterTime = genClusterTime(Date.now()); + const client = new Topology('localhost:27017'); + sessionPool = client.s.sessionPool; + session = new ClientSession(client, sessionPool, { initialClusterTime: clusterTime }); + expect(session.clusterTime).to.eql(clusterTime); + }); + it('should acquire a serverSession in the constructor if the session is explicit', () => { + const session = new ClientSession(topology, serverSessionPool, { explicit: true }); + const serverSessionSymbol = getSymbolFrom(session, 'serverSession'); + expect(session).to.have.property(serverSessionSymbol).that.is.an.instanceOf(ServerSession); + }); + + it('should leave serverSession null if the session is implicit', () => { + // implicit via false (this should not be allowed...) + let session = new ClientSession(topology, serverSessionPool, { explicit: false }); + const serverSessionSymbol = getSymbolFrom(session, 'serverSession'); + expect(session).to.have.property(serverSessionSymbol, undefined); + // implicit via omission + session = new ClientSession(topology, serverSessionPool, {}); + expect(session).to.have.property(serverSessionSymbol, undefined); + }); + + it('should start the txnNumberIncrement at zero', () => { + const session = new ClientSession(topology, serverSessionPool); + const txnNumberIncrementSymbol = getSymbolFrom(session, 'txnNumberIncrement'); + expect(session).to.have.property(txnNumberIncrementSymbol, 0); + }); + }); + + describe('get serverSession()', () => { + it('should return whatever is defined for serverSession symbol if clientSession.hadEnded is true', () => { + const session = new ClientSession(topology, serverSessionPool, { explicit: false }); + const serverSessionSymbol = getSymbolFrom(session, 'serverSession'); + expect(session).to.have.property(serverSessionSymbol, undefined); + session.hasEnded = true; + expect(session.serverSession).to.be.undefined; + session[serverSessionSymbol] = 'wacky crazy value'; + expect(session.serverSession).to.be.equal('wacky crazy value'); + }); + + it('should acquire a serverSession if clientSession.hadEnded is false', () => { + const session = new ClientSession(topology, serverSessionPool, { explicit: false }); + const serverSessionSymbol = getSymbolFrom(session, 'serverSession'); + expect(session).to.have.property(serverSessionSymbol, undefined); + session.hasEnded = false; + expect(session.serverSession).to.be.instanceOf(ServerSession); + }); + }); + + describe('incrementTransactionNumber()', () => { + it('should not allocate serverSession', () => { + const session = new ClientSession(topology, serverSessionPool); + const txnNumberIncrementSymbol = getSymbolFrom(session, 'txnNumberIncrement'); + + session.incrementTransactionNumber(); + expect(session).to.have.property(txnNumberIncrementSymbol, 1); + + const serverSessionSymbol = getSymbolFrom(session, 'serverSession'); + expect(session).to.have.property(serverSessionSymbol, undefined); + }); + it('should save increments to txnNumberIncrement symbol', () => { + const session = new ClientSession(topology, serverSessionPool); + const txnNumberIncrementSymbol = getSymbolFrom(session, 'txnNumberIncrement'); + + session.incrementTransactionNumber(); + session.incrementTransactionNumber(); + session.incrementTransactionNumber(); + + expect(session).to.have.property(txnNumberIncrementSymbol, 3); + }); + }); + + describe('applySession()', () => { + it('should allocate serverSession', () => { + const session = new ClientSession(topology, serverSessionPool); + const serverSessionSymbol = getSymbolFrom(session, 'serverSession'); + + const command = { magic: 1 }; + const result = applySession(session, command, {}); + + expect(result).to.not.exist; + expect(command).to.have.property('lsid'); + expect(session).to.have.property(serverSessionSymbol).that.is.instanceOf(ServerSession); + }); + + it('should apply saved txnNumberIncrements', () => { + const session = new ClientSession(topology, serverSessionPool); + const serverSessionSymbol = getSymbolFrom(session, 'serverSession'); + + session.incrementTransactionNumber(); + session.incrementTransactionNumber(); + session.incrementTransactionNumber(); + + const command = { magic: 1 }; + const result = applySession(session, command, { + // txnNumber will be applied for retryable write command + willRetryWrite: true + }); + + expect(result).to.not.exist; + expect(command).to.have.property('lsid'); + expect(command).to.have.property('txnNumber').instanceOf(Long); + expect(command.txnNumber.toNumber()).to.equal(3); + expect(session).to.have.property(serverSessionSymbol).that.is.instanceOf(ServerSession); + }); + }); }); - describe('ServerSessionPool', function () { + describe('class ServerSessionPool', function () { afterEach(() => { test.client.close(); return mock.cleanup(); @@ -318,100 +427,4 @@ describe('Sessions - unit', function () { done(); }); }); - - describe('ServerSession allocation behavior', () => { - let serverSessionPool; - let topology; - - beforeEach(() => { - topology = {}; // we don't need a real topology, just a truthy value - serverSessionPool = new ServerSessionPool(topology); - }); - - it('serverSession getter should return whatever is defined for serverSession symbol if clientSession is ended', () => { - const session = new ClientSession(topology, serverSessionPool, { explicit: false }); - const serverSessionSymbol = getSymbolFrom(session, 'serverSession'); - expect(session).to.have.property(serverSessionSymbol, undefined); - session.hasEnded = true; - expect(session.serverSession).to.be.undefined; - session[serverSessionSymbol] = 'wacky crazy value'; - expect(session.serverSession).to.be.equal('wacky crazy value'); - }); - - it('should acquire a serverSession in the constructor if the session is explicit', () => { - const session = new ClientSession(topology, serverSessionPool, { explicit: true }); - const serverSessionSymbol = getSymbolFrom(session, 'serverSession'); - expect(session).to.have.property(serverSessionSymbol).that.is.an.instanceOf(ServerSession); - }); - - it('should leave serverSession null if the session is implicit', () => { - // implicit via false (this should not be allowed...) - let session = new ClientSession(topology, serverSessionPool, { explicit: false }); - const serverSessionSymbol = getSymbolFrom(session, 'serverSession'); - expect(session).to.have.property(serverSessionSymbol, undefined); - // implicit via omission - session = new ClientSession(topology, serverSessionPool, {}); - expect(session).to.have.property(serverSessionSymbol, undefined); - }); - - it('should start the txnNumberIncrement at zero', () => { - const session = new ClientSession(topology, serverSessionPool); - const txnNumberIncrementSymbol = getSymbolFrom(session, 'txnNumberIncrement'); - expect(session).to.have.property(txnNumberIncrementSymbol, 0); - }); - - it('incrementTransactionNumber should not allocate serverSession', () => { - const session = new ClientSession(topology, serverSessionPool); - const txnNumberIncrementSymbol = getSymbolFrom(session, 'txnNumberIncrement'); - - session.incrementTransactionNumber(); - expect(session).to.have.property(txnNumberIncrementSymbol, 1); - - const serverSessionSymbol = getSymbolFrom(session, 'serverSession'); - expect(session).to.have.property(serverSessionSymbol, undefined); - }); - it('incrementTransactionNumber should save increments', () => { - const session = new ClientSession(topology, serverSessionPool); - const txnNumberIncrementSymbol = getSymbolFrom(session, 'txnNumberIncrement'); - - session.incrementTransactionNumber(); - session.incrementTransactionNumber(); - session.incrementTransactionNumber(); - - expect(session).to.have.property(txnNumberIncrementSymbol, 3); - }); - - it('applySession should allocate serverSession', () => { - const session = new ClientSession(topology, serverSessionPool); - const serverSessionSymbol = getSymbolFrom(session, 'serverSession'); - - const command = { magic: 1 }; - const result = applySession(session, command, {}); - - expect(result).to.not.exist; - expect(command).to.have.property('lsid'); - expect(session).to.have.property(serverSessionSymbol).that.is.instanceOf(ServerSession); - }); - - it('applySession should apply saved txnNumberIncrements', () => { - const session = new ClientSession(topology, serverSessionPool); - const serverSessionSymbol = getSymbolFrom(session, 'serverSession'); - - session.incrementTransactionNumber(); - session.incrementTransactionNumber(); - session.incrementTransactionNumber(); - - const command = { magic: 1 }; - const result = applySession(session, command, { - // txnNumber will be applied for retryable write command - willRetryWrite: true - }); - - expect(result).to.not.exist; - expect(command).to.have.property('lsid'); - expect(command).to.have.property('txnNumber').instanceOf(Long); - expect(command.txnNumber.equals(Long.fromNumber(3))); - expect(session).to.have.property(serverSessionSymbol).that.is.instanceOf(ServerSession); - }); - }); }); From ef98aeedc37772ba16a9e7d84c4b51ae568af179 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 24 Mar 2022 16:12:40 -0400 Subject: [PATCH 06/14] test and throw updates --- src/sessions.ts | 22 +++++++++++----------- test/unit/sessions.test.js | 20 +++++++++++--------- 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/src/sessions.ts b/src/sessions.ts index 7a691231be..2b5748e69d 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -75,7 +75,7 @@ const kSnapshotTime = Symbol('snapshotTime'); const kSnapshotEnabled = Symbol('snapshotEnabled'); /** @internal */ const kPinnedConnection = Symbol('pinnedConnection'); -/** @internal Accumulates total number of increments to perform to txnNumber */ +/** @internal Accumulates total number of increments to add to txnNumber when applying session to command */ const kTxnNumberIncrement = Symbol('txnNumberIncrement'); /** @public */ @@ -111,14 +111,14 @@ export class ClientSession extends TypedEventEmitter { defaultTransactionOptions: TransactionOptions; transaction: Transaction; /** @internal */ - [kServerSession]?: ServerSession; + [kServerSession]: ServerSession | null; /** @internal */ [kSnapshotTime]?: Timestamp; /** @internal */ [kSnapshotEnabled] = false; /** @internal */ [kPinnedConnection]?: Connection; - /** @internal Accumulates total number of increments to add to txnNumber when applying session to command */ + /** @internal */ [kTxnNumberIncrement]: number; /** @@ -163,8 +163,8 @@ export class ClientSession extends TypedEventEmitter { this.hasEnded = false; this.clientOptions = clientOptions; - this.explicit = Boolean(options.explicit); - this[kServerSession] = this.explicit ? this.sessionPool.acquire() : undefined; + this.explicit = !!options.explicit; + this[kServerSession] = this.explicit ? this.sessionPool.acquire() : null; this[kTxnNumberIncrement] = 0; this.supports = { @@ -185,12 +185,12 @@ export class ClientSession extends TypedEventEmitter { } get serverSession(): ServerSession { - if (this.hasEnded) { - // @ts-expect-error: If the session has ended we do not want to run the acquire code below + if (this.hasEnded && !this.explicit && this[kServerSession] == null) { + // If the session has ended we do not want to run the acquire code below // regardless of the value of kServerSession potentially being nullish. It *should* always be - // a ServerSession at this stage, but if it is not, risking a null access seems worth it - // as opposed to accidentally acquiring a new ServerSession for an ended ClientSession - return this[kServerSession]; + // a ServerSession at this stage, but if it is not, we throw a MongoRuntimeError indicating + // this is an unexpected scenario, that is not recoverable + throw new MongoRuntimeError('Unexpected null serverSession for an ended implicit session'); } let serverSession = this[kServerSession]; if (serverSession == null) { @@ -995,7 +995,7 @@ export function applySession( command.lsid = serverSession.id; const inTxnOrTxnCommand = session.inTransaction() || isTransactionCommand(command); - const isRetryableWrite = Boolean(options.willRetryWrite); + const isRetryableWrite = !!options.willRetryWrite; if (isRetryableWrite || inTxnOrTxnCommand) { serverSession.txnNumber += session[kTxnNumberIncrement]; diff --git a/test/unit/sessions.test.js b/test/unit/sessions.test.js index 2800d82d88..108af548b5 100644 --- a/test/unit/sessions.test.js +++ b/test/unit/sessions.test.js @@ -13,6 +13,7 @@ const { const { now, isHello } = require('../../src/utils'); const { getSymbolFrom } = require('../tools/utils'); const { Long } = require('../../src/bson'); +const { MongoRuntimeError } = require('../../src/error'); let test = {}; @@ -197,6 +198,7 @@ describe('Sessions - unit', function () { session = new ClientSession(client, sessionPool, { initialClusterTime: clusterTime }); expect(session.clusterTime).to.eql(clusterTime); }); + it('should acquire a serverSession in the constructor if the session is explicit', () => { const session = new ClientSession(topology, serverSessionPool, { explicit: true }); const serverSessionSymbol = getSymbolFrom(session, 'serverSession'); @@ -207,10 +209,10 @@ describe('Sessions - unit', function () { // implicit via false (this should not be allowed...) let session = new ClientSession(topology, serverSessionPool, { explicit: false }); const serverSessionSymbol = getSymbolFrom(session, 'serverSession'); - expect(session).to.have.property(serverSessionSymbol, undefined); + expect(session).to.have.property(serverSessionSymbol, null); // implicit via omission session = new ClientSession(topology, serverSessionPool, {}); - expect(session).to.have.property(serverSessionSymbol, undefined); + expect(session).to.have.property(serverSessionSymbol, null); }); it('should start the txnNumberIncrement at zero', () => { @@ -222,20 +224,19 @@ describe('Sessions - unit', function () { describe('get serverSession()', () => { it('should return whatever is defined for serverSession symbol if clientSession.hadEnded is true', () => { - const session = new ClientSession(topology, serverSessionPool, { explicit: false }); + const session = new ClientSession(topology, serverSessionPool, { explicit: false }); // make an implicit session const serverSessionSymbol = getSymbolFrom(session, 'serverSession'); - expect(session).to.have.property(serverSessionSymbol, undefined); + expect(session).to.have.property(serverSessionSymbol, null); session.hasEnded = true; - expect(session.serverSession).to.be.undefined; - session[serverSessionSymbol] = 'wacky crazy value'; - expect(session.serverSession).to.be.equal('wacky crazy value'); + expect(() => session.serverSession).to.throw(MongoRuntimeError); }); it('should acquire a serverSession if clientSession.hadEnded is false', () => { const session = new ClientSession(topology, serverSessionPool, { explicit: false }); const serverSessionSymbol = getSymbolFrom(session, 'serverSession'); - expect(session).to.have.property(serverSessionSymbol, undefined); + expect(session).to.have.property(serverSessionSymbol, null); session.hasEnded = false; + // performs acquire expect(session.serverSession).to.be.instanceOf(ServerSession); }); }); @@ -249,8 +250,9 @@ describe('Sessions - unit', function () { expect(session).to.have.property(txnNumberIncrementSymbol, 1); const serverSessionSymbol = getSymbolFrom(session, 'serverSession'); - expect(session).to.have.property(serverSessionSymbol, undefined); + expect(session).to.have.property(serverSessionSymbol, null); }); + it('should save increments to txnNumberIncrement symbol', () => { const session = new ClientSession(topology, serverSessionPool); const txnNumberIncrementSymbol = getSymbolFrom(session, 'txnNumberIncrement'); From fefdfa2b1119f3e13800b713df01dbdd1d996b05 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Thu, 24 Mar 2022 16:30:50 -0400 Subject: [PATCH 07/14] merge boolean checks --- src/sessions.ts | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/sessions.ts b/src/sessions.ts index 2b5748e69d..bece421735 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -185,15 +185,11 @@ export class ClientSession extends TypedEventEmitter { } get serverSession(): ServerSession { - if (this.hasEnded && !this.explicit && this[kServerSession] == null) { - // If the session has ended we do not want to run the acquire code below - // regardless of the value of kServerSession potentially being nullish. It *should* always be - // a ServerSession at this stage, but if it is not, we throw a MongoRuntimeError indicating - // this is an unexpected scenario, that is not recoverable - throw new MongoRuntimeError('Unexpected null serverSession for an ended implicit session'); - } let serverSession = this[kServerSession]; if (serverSession == null) { + if (this.hasEnded && !this.explicit) { + throw new MongoRuntimeError('Unexpected null serverSession for an ended implicit session'); + } serverSession = this.sessionPool.acquire(); this[kServerSession] = serverSession; } From cbadd923bb35f912855ed96517de01a3ca4c0071 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 25 Mar 2022 14:05:48 -0400 Subject: [PATCH 08/14] reorganized serverSession getter tests --- test/unit/sessions.test.js | 120 ++++++++++++++++++++++--------------- 1 file changed, 72 insertions(+), 48 deletions(-) diff --git a/test/unit/sessions.test.js b/test/unit/sessions.test.js index 108af548b5..e0b9f50559 100644 --- a/test/unit/sessions.test.js +++ b/test/unit/sessions.test.js @@ -14,22 +14,20 @@ const { now, isHello } = require('../../src/utils'); const { getSymbolFrom } = require('../tools/utils'); const { Long } = require('../../src/bson'); const { MongoRuntimeError } = require('../../src/error'); +const sinon = require('sinon'); -let test = {}; +let test = { + topology: null +}; describe('Sessions - unit', function () { + const topology = {}; + const serverSessionPool = new ServerSessionPool(topology); + describe('class ClientSession', function () { let session; let sessionPool; - let serverSessionPool; - let topology; - - beforeEach(() => { - topology = {}; // we don't need a real topology, just a truthy value - serverSessionPool = new ServerSessionPool(topology); - }); - afterEach(done => { if (sessionPool) { sessionCleanupHandler(session, sessionPool, done)(); @@ -40,9 +38,9 @@ describe('Sessions - unit', function () { describe('startTransaction()', () => { it('should throw an error if the session is snapshot enabled', function () { - const client = new Topology('localhost:27017', {}); - sessionPool = client.s.sessionPool; - session = new ClientSession(client, sessionPool, { snapshot: true }); + const topology = new Topology('localhost:27017', {}); + sessionPool = topology.s.sessionPool; + session = new ClientSession(topology, sessionPool, { snapshot: true }); expect(session.snapshotEnabled).to.equal(true); expect(() => session.startTransaction()).to.throw( 'Transactions are not allowed with snapshot sessions' @@ -52,9 +50,9 @@ describe('Sessions - unit', function () { describe('advanceClusterTime()', () => { beforeEach(() => { - const client = new Topology('localhost:27017', {}); - sessionPool = client.s.sessionPool; - session = new ClientSession(client, sessionPool, {}); + const topology = new Topology('localhost:27017', {}); + sessionPool = topology.s.sessionPool; + session = new ClientSession(topology, sessionPool, {}); }); it('should throw an error if the input cluster time is not an object', function () { @@ -177,25 +175,26 @@ describe('Sessions - unit', function () { }); it('should throw an error if snapshot and causalConsistency options are both set to true', function () { - const client = new Topology('localhost:27017', {}); - sessionPool = client.s.sessionPool; + const topology = new Topology('localhost:27017', {}); + sessionPool = topology.s.sessionPool; expect( - () => new ClientSession(client, sessionPool, { causalConsistency: true, snapshot: true }) + () => + new ClientSession(topology, sessionPool, { causalConsistency: true, snapshot: true }) ).to.throw('Properties "causalConsistency" and "snapshot" are mutually exclusive'); }); it('should default to `null` for `clusterTime`', function () { - const client = new Topology('localhost:27017', {}); - sessionPool = client.s.sessionPool; - session = new ClientSession(client, sessionPool); + const topology = new Topology('localhost:27017', {}); + sessionPool = topology.s.sessionPool; + session = new ClientSession(topology, sessionPool); expect(session.clusterTime).to.not.exist; }); it('should set the internal clusterTime to `initialClusterTime` if provided', function () { const clusterTime = genClusterTime(Date.now()); - const client = new Topology('localhost:27017'); - sessionPool = client.s.sessionPool; - session = new ClientSession(client, sessionPool, { initialClusterTime: clusterTime }); + const topology = new Topology('localhost:27017'); + sessionPool = topology.s.sessionPool; + session = new ClientSession(topology, sessionPool, { initialClusterTime: clusterTime }); expect(session.clusterTime).to.eql(clusterTime); }); @@ -223,21 +222,46 @@ describe('Sessions - unit', function () { }); describe('get serverSession()', () => { - it('should return whatever is defined for serverSession symbol if clientSession.hadEnded is true', () => { - const session = new ClientSession(topology, serverSessionPool, { explicit: false }); // make an implicit session - const serverSessionSymbol = getSymbolFrom(session, 'serverSession'); - expect(session).to.have.property(serverSessionSymbol, null); - session.hasEnded = true; - expect(() => session.serverSession).to.throw(MongoRuntimeError); + let serverSessionSymbol; + before(() => { + serverSessionSymbol = getSymbolFrom( + new ClientSession({}, serverSessionPool, {}), + 'serverSession' + ); }); - it('should acquire a serverSession if clientSession.hadEnded is false', () => { - const session = new ClientSession(topology, serverSessionPool, { explicit: false }); - const serverSessionSymbol = getSymbolFrom(session, 'serverSession'); - expect(session).to.have.property(serverSessionSymbol, null); - session.hasEnded = false; - // performs acquire - expect(session.serverSession).to.be.instanceOf(ServerSession); + describe('from an explicit session', () => { + it('should always have a non-null serverSession after construction', () => { + const session = new ClientSession(topology, serverSessionPool, { explicit: true }); + expect(session).to.have.a.property(serverSessionSymbol).be.an.instanceOf(ServerSession); + expect(session.serverSession).be.an.instanceOf(ServerSession); + }); + + it('should always have non-null serverSession even if it is ended before getter called', () => { + const session = new ClientSession(topology, serverSessionPool, { explicit: true }); + session.hasEnded = true; + expect(session).to.have.a.property(serverSessionSymbol).be.an.instanceOf(ServerSession); + expect(session.serverSession).be.an.instanceOf(ServerSession); + }); + }); + + describe('from an implicit session', () => { + const session = new ClientSession(topology, serverSessionPool, { explicit: false }); // make an implicit session + + it('should throw if the session hasEnded before serverSession was acquired', () => { + expect(session).to.have.property(serverSessionSymbol, null); + session.hasEnded = true; + expect(() => session.serverSession).to.throw(MongoRuntimeError); + }); + + it('should acquire a serverSession if clientSession.hadEnded is false', () => { + expect(session).to.have.property(serverSessionSymbol, null); + session.hasEnded = false; + const acquireSpy = sinon.spy(serverSessionPool, 'acquire'); + expect(session.serverSession).to.be.instanceOf(ServerSession); + expect(acquireSpy.calledOnce).to.be.true; + acquireSpy.restore(); + }); }); }); @@ -303,7 +327,7 @@ describe('Sessions - unit', function () { describe('class ServerSessionPool', function () { afterEach(() => { - test.client.close(); + test.topology.close(); return mock.cleanup(); }); @@ -320,12 +344,12 @@ describe('Sessions - unit', function () { }); }) .then(() => { - test.client = new Topology(test.server.hostAddress()); + test.topology = new Topology(test.server.hostAddress()); return new Promise((resolve, reject) => { - test.client.once('error', reject); - test.client.once('connect', resolve); - test.client.connect(); + test.topology.once('error', reject); + test.topology.once('connect', resolve); + test.topology.connect(); }); }); }); @@ -337,7 +361,7 @@ describe('Sessions - unit', function () { }); it('should create a new session if the pool is empty', function (done) { - const pool = new ServerSessionPool(test.client); + const pool = new ServerSessionPool(test.topology); done = sessionCleanupHandler(null, pool, done); expect(pool.sessions).to.have.length(0); @@ -351,7 +375,7 @@ describe('Sessions - unit', function () { it('should reuse sessions which have not timed out yet on acquire', function (done) { const oldSession = new ServerSession(); - const pool = new ServerSessionPool(test.client); + const pool = new ServerSessionPool(test.topology); done = sessionCleanupHandler(null, pool, done); pool.sessions.push(oldSession); @@ -367,7 +391,7 @@ describe('Sessions - unit', function () { const oldSession = new ServerSession(); oldSession.lastUse = now() - 30 * 60 * 1000; // add 30min - const pool = new ServerSessionPool(test.client); + const pool = new ServerSessionPool(test.topology); done = sessionCleanupHandler(null, pool, done); pool.sessions.push(oldSession); @@ -386,7 +410,7 @@ describe('Sessions - unit', function () { return session; }); - const pool = new ServerSessionPool(test.client); + const pool = new ServerSessionPool(test.topology); done = sessionCleanupHandler(null, pool, done); pool.sessions = pool.sessions.concat(oldSessions); @@ -400,7 +424,7 @@ describe('Sessions - unit', function () { const session = new ServerSession(); session.lastUse = now() - 9.5 * 60 * 1000; // add 9.5min - const pool = new ServerSessionPool(test.client); + const pool = new ServerSessionPool(test.topology); done = sessionCleanupHandler(null, pool, done); pool.release(session); @@ -409,7 +433,7 @@ describe('Sessions - unit', function () { }); it('should maintain a LIFO queue of sessions', function (done) { - const pool = new ServerSessionPool(test.client); + const pool = new ServerSessionPool(test.topology); done = sessionCleanupHandler(null, pool, done); const sessionA = new ServerSession(); From 34a2ee47d98c66a9557aef0184ff6ef0ed646ef5 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 25 Mar 2022 14:37:25 -0400 Subject: [PATCH 09/14] test: scenario where explicit session is separated from its server session --- src/sessions.ts | 3 +++ test/unit/sessions.test.js | 8 ++++++++ 2 files changed, 11 insertions(+) diff --git a/src/sessions.ts b/src/sessions.ts index bece421735..7ecaa5b035 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -187,6 +187,9 @@ export class ClientSession extends TypedEventEmitter { get serverSession(): ServerSession { let serverSession = this[kServerSession]; if (serverSession == null) { + if (this.explicit) { + throw new MongoRuntimeError('Unexpected null serverSession for an explicit session'); + } if (this.hasEnded && !this.explicit) { throw new MongoRuntimeError('Unexpected null serverSession for an ended implicit session'); } diff --git a/test/unit/sessions.test.js b/test/unit/sessions.test.js index e0b9f50559..abc396364f 100644 --- a/test/unit/sessions.test.js +++ b/test/unit/sessions.test.js @@ -243,6 +243,14 @@ describe('Sessions - unit', function () { expect(session).to.have.a.property(serverSessionSymbol).be.an.instanceOf(ServerSession); expect(session.serverSession).be.an.instanceOf(ServerSession); }); + + it('should throw if the serverSession at the symbol property goes missing', () => { + const session = new ClientSession(topology, serverSessionPool, { explicit: true }); + // We really want to make sure a ClientSession is not separated from its serverSession + session[serverSessionSymbol] = null; + expect(session).to.have.a.property(serverSessionSymbol).be.null; + expect(() => session.serverSession).throw(MongoRuntimeError); + }); }); describe('from an implicit session', () => { From b53255c650ddffe2f70b9db0f18b5b286621b51d Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 25 Mar 2022 14:41:00 -0400 Subject: [PATCH 10/14] Boolean Logic++ --- src/sessions.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sessions.ts b/src/sessions.ts index 7ecaa5b035..445049cea9 100644 --- a/src/sessions.ts +++ b/src/sessions.ts @@ -190,7 +190,7 @@ export class ClientSession extends TypedEventEmitter { if (this.explicit) { throw new MongoRuntimeError('Unexpected null serverSession for an explicit session'); } - if (this.hasEnded && !this.explicit) { + if (this.hasEnded) { throw new MongoRuntimeError('Unexpected null serverSession for an ended implicit session'); } serverSession = this.sessionPool.acquire(); From c8b1057d2479e7c6d800bf0ca308199e06f4a93b Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 25 Mar 2022 15:32:45 -0400 Subject: [PATCH 11/14] Apply suggestions from code review Co-authored-by: Daria Pardue --- test/unit/sessions.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/sessions.test.js b/test/unit/sessions.test.js index abc396364f..2ce4c7d2f1 100644 --- a/test/unit/sessions.test.js +++ b/test/unit/sessions.test.js @@ -256,13 +256,13 @@ describe('Sessions - unit', function () { describe('from an implicit session', () => { const session = new ClientSession(topology, serverSessionPool, { explicit: false }); // make an implicit session - it('should throw if the session hasEnded before serverSession was acquired', () => { + it('should throw if the session ended before serverSession was acquired', () => { expect(session).to.have.property(serverSessionSymbol, null); session.hasEnded = true; expect(() => session.serverSession).to.throw(MongoRuntimeError); }); - it('should acquire a serverSession if clientSession.hadEnded is false', () => { + it('should acquire a serverSession if clientSession.hasEnded is false and serverSession is not set', () => { expect(session).to.have.property(serverSessionSymbol, null); session.hasEnded = false; const acquireSpy = sinon.spy(serverSessionPool, 'acquire'); From 5a22be441ce828d2677da85e02903f098deffe54 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 25 Mar 2022 15:37:53 -0400 Subject: [PATCH 12/14] test: add check for acquire not being called again --- test/unit/sessions.test.js | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/test/unit/sessions.test.js b/test/unit/sessions.test.js index 2ce4c7d2f1..5ab80c2463 100644 --- a/test/unit/sessions.test.js +++ b/test/unit/sessions.test.js @@ -270,6 +270,23 @@ describe('Sessions - unit', function () { expect(acquireSpy.calledOnce).to.be.true; acquireSpy.restore(); }); + + it('should return the existing serverSession and not acquire a new one if one is already set', () => { + expect(session).to.have.property(serverSessionSymbol, null); + const acquireSpy = sinon.spy(serverSessionPool, 'acquire'); + expect(session.serverSession).to.be.instanceOf(ServerSession); + expect(acquireSpy.calledOnce).to.be.true; + + // call the getter a bunch more times + expect(session.serverSession).to.be.instanceOf(ServerSession); + expect(session.serverSession).to.be.instanceOf(ServerSession); + expect(session.serverSession).to.be.instanceOf(ServerSession); + + // acquire never called again + expect(acquireSpy.calledOnce).to.be.true; + + acquireSpy.restore(); + }); }); }); From fc37a3e2a2ce8ca0258d6dc98c7d963041179578 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 25 Mar 2022 16:40:58 -0400 Subject: [PATCH 13/14] test: ended implict session doesn't call acquire --- test/unit/sessions.test.js | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/test/unit/sessions.test.js b/test/unit/sessions.test.js index 5ab80c2463..08b18ba56c 100644 --- a/test/unit/sessions.test.js +++ b/test/unit/sessions.test.js @@ -274,14 +274,43 @@ describe('Sessions - unit', function () { it('should return the existing serverSession and not acquire a new one if one is already set', () => { expect(session).to.have.property(serverSessionSymbol, null); const acquireSpy = sinon.spy(serverSessionPool, 'acquire'); + const firstServerSessionGetResult = session.serverSession; + expect(firstServerSessionGetResult).to.be.instanceOf(ServerSession); + expect(acquireSpy.calledOnce).to.be.true; + + // call the getter a bunch more times + expect(session.serverSession).to.be.instanceOf(ServerSession); + expect(session.serverSession).to.be.instanceOf(ServerSession); expect(session.serverSession).to.be.instanceOf(ServerSession); + + expect(session.serverSession.id.id.buffer.toString('hex')).to.equal( + firstServerSessionGetResult.id.id.buffer.toString('hex') + ); + + // acquire never called again + expect(acquireSpy.calledOnce).to.be.true; + + acquireSpy.restore(); + }); + + it('should return the existing serverSession and not acquire a new one if one is already set and session is ended', () => { + expect(session).to.have.property(serverSessionSymbol, null); + const acquireSpy = sinon.spy(serverSessionPool, 'acquire'); + const firstServerSessionGetResult = session.serverSession; + expect(firstServerSessionGetResult).to.be.instanceOf(ServerSession); expect(acquireSpy.calledOnce).to.be.true; + session.hasEnded = true; + // call the getter a bunch more times expect(session.serverSession).to.be.instanceOf(ServerSession); expect(session.serverSession).to.be.instanceOf(ServerSession); expect(session.serverSession).to.be.instanceOf(ServerSession); + expect(session.serverSession.id.id.buffer.toString('hex')).to.equal( + firstServerSessionGetResult.id.id.buffer.toString('hex') + ); + // acquire never called again expect(acquireSpy.calledOnce).to.be.true; From 3570ed615d61a1d59c959b7701d7ef10b1de2f16 Mon Sep 17 00:00:00 2001 From: Neal Beeken Date: Fri, 25 Mar 2022 16:51:06 -0400 Subject: [PATCH 14/14] make sure session id doesn't change --- test/unit/sessions.test.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/unit/sessions.test.js b/test/unit/sessions.test.js index 08b18ba56c..84de683ab2 100644 --- a/test/unit/sessions.test.js +++ b/test/unit/sessions.test.js @@ -254,15 +254,15 @@ describe('Sessions - unit', function () { }); describe('from an implicit session', () => { - const session = new ClientSession(topology, serverSessionPool, { explicit: false }); // make an implicit session - it('should throw if the session ended before serverSession was acquired', () => { + const session = new ClientSession(topology, serverSessionPool, { explicit: false }); // make an implicit session expect(session).to.have.property(serverSessionSymbol, null); session.hasEnded = true; expect(() => session.serverSession).to.throw(MongoRuntimeError); }); it('should acquire a serverSession if clientSession.hasEnded is false and serverSession is not set', () => { + const session = new ClientSession(topology, serverSessionPool, { explicit: false }); // make an implicit session expect(session).to.have.property(serverSessionSymbol, null); session.hasEnded = false; const acquireSpy = sinon.spy(serverSessionPool, 'acquire'); @@ -272,6 +272,7 @@ describe('Sessions - unit', function () { }); it('should return the existing serverSession and not acquire a new one if one is already set', () => { + const session = new ClientSession(topology, serverSessionPool, { explicit: false }); // make an implicit session expect(session).to.have.property(serverSessionSymbol, null); const acquireSpy = sinon.spy(serverSessionPool, 'acquire'); const firstServerSessionGetResult = session.serverSession; @@ -294,6 +295,7 @@ describe('Sessions - unit', function () { }); it('should return the existing serverSession and not acquire a new one if one is already set and session is ended', () => { + const session = new ClientSession(topology, serverSessionPool, { explicit: false }); // make an implicit session expect(session).to.have.property(serverSessionSymbol, null); const acquireSpy = sinon.spy(serverSessionPool, 'acquire'); const firstServerSessionGetResult = session.serverSession;