Skip to content

Commit b9ed1fa

Browse files
nbbeekennbbeeken
committed
feat(NODE-3697): reduce serverSession allocation
1 parent ff26b12 commit b9ed1fa

File tree

4 files changed

+176
-33
lines changed

4 files changed

+176
-33
lines changed

‎src/sessions.ts

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,7 @@ export class ClientSession extends TypedEventEmitter<ClientSessionEvents> {
123123
defaultTransactionOptions: TransactionOptions;
124124
transaction: Transaction;
125125
/** @internal */
126-
[kServerSession]?: ServerSession;
126+
readonly [kServerSession]?: ServerSession;
127127
/** @internal */
128128
[kSnapshotTime]?: Timestamp;
129129
/** @internal */
@@ -172,7 +172,12 @@ export class ClientSession extends TypedEventEmitter<ClientSessionEvents> {
172172
this.sessionPool = sessionPool;
173173
this.hasEnded = false;
174174
this.clientOptions = clientOptions;
175-
this[kServerSession] = undefined;
175+
Object.defineProperty(this, kServerSession, {
176+
value: null,
177+
writable: false,
178+
enumerable: false,
179+
configurable: true
180+
});
176181

177182
this.supports = {
178183
causalConsistency: options.snapshot !== true && options.causalConsistency !== false
@@ -194,7 +199,14 @@ export class ClientSession extends TypedEventEmitter<ClientSessionEvents> {
194199

195200
get serverSession(): ServerSession {
196201
if (this[kServerSession] == null) {
197-
this[kServerSession] = this.sessionPool.acquire();
202+
// ServerSessions must remain the same for a ClientSession's entire lifetime
203+
// enforcing readonly here
204+
Object.defineProperty(this, kServerSession, {
205+
writable: false,
206+
configurable: true,
207+
enumerable: false,
208+
value: this.sessionPool.acquire()
209+
});
198210
}
199211

200212
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
@@ -269,7 +281,12 @@ export class ClientSession extends TypedEventEmitter<ClientSessionEvents> {
269281

270282
// release the server session back to the pool
271283
this.sessionPool.release(this.serverSession);
272-
this[kServerSession] = undefined;
284+
Object.defineProperty(this, kServerSession, {
285+
value: null,
286+
writable: false,
287+
enumerable: false,
288+
configurable: true
289+
});
273290

274291
// mark the session as ended, and emit a signal
275292
this.hasEnded = true;
@@ -355,9 +372,9 @@ export class ClientSession extends TypedEventEmitter<ClientSessionEvents> {
355372

356373
/** Increment the transaction number on the internal ServerSession */
357374
incrementTransactionNumber(): void {
358-
if (this.serverSession) {
359-
this.serverSession.txnNumber =
360-
typeof this.serverSession.txnNumber === 'number' ? this.serverSession.txnNumber + 1 : 0;
375+
const serverSession = this[kServerSession];
376+
if (serverSession) {
377+
serverSession.txnNumber += 1;
361378
}
362379
}
363380

‎test/integration/sessions/sessions.spec.prose.test.ts

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
import { expect } from 'chai';
2+
3+
import { Collection } from '../../../src/index';
4+
5+
describe('ServerSession', () => {
6+
let client;
7+
let testCollection: Collection<{ _id: number; a?: number }>;
8+
beforeEach(async function () {
9+
const configuration = this.configuration;
10+
client = await configuration.newClient({ maxPoolSize: 1, monitorCommands: true }).connect();
11+
12+
// reset test collection
13+
testCollection = client.db('test').collection('too.many.sessions');
14+
await testCollection.drop().catch(() => null);
15+
});
16+
17+
afterEach(async () => {
18+
await client?.close(true);
19+
});
20+
21+
/**
22+
* TODO(DRIVERS-2218): Refactor tests to align exactly with spec wording. Preliminarily implements:
23+
* 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)
24+
* 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
25+
* Drivers MUST assert that the number of allocated sessions never exceeds the number of concurrent operations executing
26+
*/
27+
28+
it('13. may reuse one server session for many operations', async () => {
29+
const events = [];
30+
client.on('commandStarted', ev => events.push(ev));
31+
32+
const operations = [
33+
testCollection.insertOne({ _id: 1 }),
34+
testCollection.deleteOne({ _id: 2 }),
35+
testCollection.updateOne({ _id: 3 }, { $set: { a: 1 } }),
36+
testCollection.bulkWrite([{ updateOne: { filter: { _id: 4 }, update: { $set: { a: 1 } } } }]),
37+
testCollection.findOneAndDelete({ _id: 5 }),
38+
testCollection.findOneAndUpdate({ _id: 6 }, { $set: { a: 1 } }),
39+
testCollection.findOneAndReplace({ _id: 7 }, { a: 8 }),
40+
testCollection.find().toArray()
41+
];
42+
43+
const allResults = await Promise.all(operations);
44+
45+
expect(allResults).to.have.lengthOf(operations.length);
46+
expect(events).to.have.lengthOf(operations.length);
47+
48+
expect(new Set(events.map(ev => ev.command.lsid.id.toString('hex'))).size).to.equal(1); // This is a guarantee in node
49+
});
50+
51+
it('should only use one session for many operations when maxPoolSize is 1', async () => {
52+
const documents = new Array(50).fill(null).map((_, idx) => ({ _id: idx }));
53+
54+
const events = [];
55+
client.on('commandStarted', ev => events.push(ev));
56+
const allResults = await Promise.all(documents.map(async doc => testCollection.insertOne(doc)));
57+
58+
expect(allResults).to.have.lengthOf(documents.length);
59+
expect(events).to.have.lengthOf(documents.length);
60+
61+
expect(new Set(events.map(ev => ev.command.lsid.id.toString('hex'))).size).to.equal(1);
62+
});
63+
});

‎test/tools/cluster_setup.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ if [[ $1 == "replica_set" ]]; then
1717
echo "mongodb://bob:pwd123@localhost:31000,localhost:31001,localhost:31002/?replicaSet=rs"
1818
elif [[ $1 == "sharded_cluster" ]]; then
1919
mkdir -p $SHARDED_DIR
20-
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
20+
mlaunch init --dir $SHARDED_DIR --auth --username "bob" --password "pwd123" --replicaset --nodes 3 --name rs --port 51000 --setParameter enableTestCommands=1 --sharded 1 --mongos 2
2121
echo "mongodb://bob:pwd123@localhost:51000,localhost:51001"
2222
elif [[ $1 == "server" ]]; then
2323
mkdir -p $SINGLE_DIR

‎test/unit/sessions.test.js renamed to ‎test/unit/sessions.test.ts

Lines changed: 88 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,47 @@
1-
'use strict';
2-
3-
const mock = require('../tools/mongodb-mock/index');
4-
const { expect } = require('chai');
5-
const { genClusterTime, sessionCleanupHandler } = require('../tools/common');
6-
const { Topology } = require('../../src/sdam/topology');
7-
const { ServerSessionPool, ServerSession, ClientSession } = require('../../src/sessions');
8-
const { now, isHello } = require('../../src/utils');
9-
10-
let test = {};
11-
12-
describe('Sessions - unit/core', function () {
13-
describe('ClientSession', function () {
14-
let session;
15-
let sessionPool;
1+
import { expect } from 'chai';
2+
3+
import { Long } from '../../src/bson';
4+
import { DEFAULT_OPTIONS } from '../../src/connection_string';
5+
import { Topology } from '../../src/sdam/topology';
6+
import { ClientSession, ServerSession, ServerSessionPool } from '../../src/sessions';
7+
import { HostAddress, isHello, now } from '../../src/utils';
8+
import { genClusterTime, sessionCleanupHandler } from '../tools/common';
9+
import * as mock from '../tools/mongodb-mock/index';
10+
import { getSymbolFrom } from '../tools/utils';
11+
12+
const test = {
13+
client: null,
14+
server: null
15+
};
16+
17+
// TODO(NODE-2149): MongoClient will have topology always defined on it and we can stop constructing topologies in here.
18+
const topologyHost = 'iLoveJavaScript:123';
19+
const topologyOptions = {
20+
hosts: [HostAddress.fromString(topologyHost)],
21+
retryReads: DEFAULT_OPTIONS.get('retryReads'),
22+
retryWrites: DEFAULT_OPTIONS.get('retryWrites'),
23+
serverSelectionTimeoutMS: DEFAULT_OPTIONS.get('serverSelectionTimeoutMS'),
24+
directConnection: DEFAULT_OPTIONS.get('directConnection'),
25+
loadBalanced: DEFAULT_OPTIONS.get('loadBalanced'),
26+
metadata: DEFAULT_OPTIONS.get('metadata'),
27+
monitorCommands: DEFAULT_OPTIONS.get('monitorCommands'),
28+
tls: DEFAULT_OPTIONS.get('tls'),
29+
maxPoolSize: DEFAULT_OPTIONS.get('maxPoolSize'),
30+
minPoolSize: DEFAULT_OPTIONS.get('minPoolSize'),
31+
waitQueueTimeoutMS: DEFAULT_OPTIONS.get('waitQueueTimeoutMS'),
32+
connectionType: DEFAULT_OPTIONS.get('connectionType'),
33+
connectTimeoutMS: DEFAULT_OPTIONS.get('connectTimeoutMS'),
34+
maxIdleTimeMS: DEFAULT_OPTIONS.get('maxIdleTimeMS'),
35+
heartbeatFrequencyMS: DEFAULT_OPTIONS.get('heartbeatFrequencyMS'),
36+
minHeartbeatFrequencyMS: DEFAULT_OPTIONS.get('minHeartbeatFrequencyMS'),
37+
srvMaxHosts: 0,
38+
srvServiceName: 'mongodb'
39+
};
40+
41+
describe('Sessions - unit', function () {
42+
describe('new ClientSession()', function () {
43+
let session: ClientSession;
44+
let sessionPool: ServerSessionPool;
1645

1746
afterEach(done => {
1847
if (sessionPool) {
@@ -24,44 +53,69 @@ describe('Sessions - unit/core', function () {
2453

2554
it('should throw errors with invalid parameters', function () {
2655
expect(() => {
56+
// @ts-expect-error: Client session has 3 required parameters
2757
new ClientSession();
2858
}).to.throw(/ClientSession requires a topology/);
2959

3060
expect(() => {
61+
// @ts-expect-error: Client session has 3 required parameters
3162
new ClientSession({});
3263
}).to.throw(/ClientSession requires a ServerSessionPool/);
3364

3465
expect(() => {
66+
// @ts-expect-error: Client session has 3 required parameters
3567
new ClientSession({}, {});
3668
}).to.throw(/ClientSession requires a ServerSessionPool/);
3769
});
3870

3971
it('should throw an error if snapshot and causalConsistency options are both set to true', function () {
40-
const client = new Topology('localhost:27017', {});
72+
const client = new Topology(topologyHost, topologyOptions);
4173
sessionPool = client.s.sessionPool;
4274
expect(
4375
() => new ClientSession(client, sessionPool, { causalConsistency: true, snapshot: true })
4476
).to.throw('Properties "causalConsistency" and "snapshot" are mutually exclusive');
4577
});
4678

4779
it('should default to `null` for `clusterTime`', function () {
48-
const client = new Topology('localhost:27017', {});
80+
const client = new Topology(topologyHost, topologyOptions);
4981
sessionPool = client.s.sessionPool;
50-
session = new ClientSession(client, sessionPool);
82+
session = new ClientSession(client, sessionPool, {});
5183
expect(session.clusterTime).to.not.exist;
5284
});
5385

5486
it('should set the internal clusterTime to `initialClusterTime` if provided', function () {
5587
const clusterTime = genClusterTime(Date.now());
56-
const client = new Topology('localhost:27017');
88+
const client = new Topology(topologyHost, topologyOptions);
5789
sessionPool = client.s.sessionPool;
5890
session = new ClientSession(client, sessionPool, { initialClusterTime: clusterTime });
5991
expect(session.clusterTime).to.eql(clusterTime);
6092
});
6193

94+
it('should not acquire a serverSession inside incrementTxnNumber', () => {
95+
const client = new Topology(topologyHost, topologyOptions);
96+
const sessionPool = client.s.sessionPool;
97+
const session = new ClientSession(client, sessionPool, {});
98+
99+
const serverSessionSymbol = getSymbolFrom(session, 'serverSession');
100+
expect(session).to.have.own.property(serverSessionSymbol).that.is.null;
101+
session.incrementTransactionNumber();
102+
expect(session).to.have.own.property(serverSessionSymbol).that.is.null;
103+
});
104+
105+
it('should acquire serverSession in getter', () => {
106+
const client = new Topology(topologyHost, topologyOptions);
107+
const sessionPool = client.s.sessionPool;
108+
const session = new ClientSession(client, sessionPool, {});
109+
110+
const serverSessionSymbol = getSymbolFrom(session, 'serverSession');
111+
expect(session).to.have.own.property(serverSessionSymbol).that.is.null;
112+
const serverSession = session.serverSession;
113+
expect(session).to.have.own.property(serverSessionSymbol).that.equals(serverSession);
114+
});
115+
62116
describe('startTransaction()', () => {
63117
it('should throw an error if the session is snapshot enabled', function () {
64-
const client = new Topology('localhost:27017', {});
118+
const client = new Topology(topologyHost, topologyOptions);
65119
sessionPool = client.s.sessionPool;
66120
session = new ClientSession(client, sessionPool, { snapshot: true });
67121
expect(session.snapshotEnabled).to.equal(true);
@@ -73,14 +127,15 @@ describe('Sessions - unit/core', function () {
73127

74128
describe('advanceClusterTime()', () => {
75129
beforeEach(() => {
76-
const client = new Topology('localhost:27017', {});
130+
const client = new Topology(topologyHost, topologyOptions);
77131
sessionPool = client.s.sessionPool;
78132
session = new ClientSession(client, sessionPool, {});
79133
});
80134

81135
it('should throw an error if the input cluster time is not an object', function () {
82136
const invalidInputs = [undefined, null, 3, 'a'];
83137
for (const input of invalidInputs) {
138+
// @ts-expect-error: Testing for a runtime type assertion
84139
expect(() => session.advanceClusterTime(input)).to.throw(
85140
'input cluster time must be an object'
86141
);
@@ -94,8 +149,11 @@ describe('Sessions - unit/core', function () {
94149

95150
delete invalidInputs[0].clusterTime;
96151
invalidInputs[1].clusterTime = null;
152+
// @ts-expect-error: Testing for a runtime type assertion
97153
invalidInputs[2].clusterTime = 5;
154+
// @ts-expect-error: Testing for a runtime type assertion
98155
invalidInputs[3].clusterTime = 'not a timestamp';
156+
// @ts-expect-error: Testing for a runtime type assertion
99157
invalidInputs[4].clusterTime = new Date('1');
100158

101159
for (const input of invalidInputs) {
@@ -120,9 +178,13 @@ describe('Sessions - unit/core', function () {
120178
// invalid non-null types
121179
// keyId must be number or BSON long
122180
// hash must be BSON binary
181+
// @ts-expect-error: Testing for a runtime type assertion
123182
invalidInputs[5].signature.keyId = {};
183+
// @ts-expect-error: Testing for a runtime type assertion
124184
invalidInputs[6].signature.keyId = 'not BSON Long';
185+
// @ts-expect-error: Testing for a runtime type assertion
125186
invalidInputs[7].signature.hash = 123;
187+
// @ts-expect-error: Testing for a runtime type assertion
126188
invalidInputs[8].signature.hash = 'not BSON Binary';
127189

128190
for (const input of invalidInputs) {
@@ -148,7 +210,7 @@ describe('Sessions - unit/core', function () {
148210

149211
// extra test case for valid alternative keyId type in signature
150212
const alsoValidTime = genClusterTime(200);
151-
alsoValidTime.signature.keyId = 10;
213+
alsoValidTime.signature.keyId = Long.fromNumber(10);
152214
session.clusterTime = null;
153215
expect(session).property('clusterTime').to.be.null;
154216
session.advanceClusterTime(alsoValidTime);
@@ -183,7 +245,7 @@ describe('Sessions - unit/core', function () {
183245
});
184246
});
185247

186-
describe('ServerSessionPool', function () {
248+
describe('new ServerSessionPool()', function () {
187249
afterEach(() => {
188250
test.client.close();
189251
return mock.cleanup();
@@ -195,14 +257,14 @@ describe('Sessions - unit/core', function () {
195257
.then(server => {
196258
test.server = server;
197259
test.server.setMessageHandler(request => {
198-
var doc = request.document;
260+
const doc = request.document;
199261
if (isHello(doc)) {
200262
request.reply(Object.assign({}, mock.HELLO, { logicalSessionTimeoutMinutes: 10 }));
201263
}
202264
});
203265
})
204266
.then(() => {
205-
test.client = new Topology(test.server.hostAddress());
267+
test.client = new Topology(test.server.hostAddress(), topologyOptions);
206268

207269
return new Promise((resolve, reject) => {
208270
test.client.once('error', reject);
@@ -214,6 +276,7 @@ describe('Sessions - unit/core', function () {
214276

215277
it('should throw errors with invalid parameters', function () {
216278
expect(() => {
279+
// @ts-expect-error: checking for a runtime type assertion
217280
new ServerSessionPool();
218281
}).to.throw(/ServerSessionPool requires a topology/);
219282
});

0 commit comments

Comments
 (0)