Skip to content

Commit 8b370a7

Browse files
nbbeekennbbeeken
authored
fix: move session support check to operation layer (#2739)
Monitor checks run on a timer and network errors cause a reset of the topology that would be corrected in the next cycle of the monitor, we were checking a property that gets updated by this async work. By moving the check to the operations layer we will allow users to obtain a session regardless of support and then emit an error if there is not support when the session is used in an operation. NODE-3100
1 parent 2d76492 commit 8b370a7

File tree

6 files changed

+78
-96
lines changed

6 files changed

+78
-96
lines changed

‎lib/mongo_client.js‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -457,10 +457,6 @@ MongoClient.prototype.startSession = function(options) {
457457
throw new MongoError('Must connect to a server before calling this method');
458458
}
459459

460-
if (!this.topology.hasSessionSupport()) {
461-
throw new MongoError('Current topology does not support sessions');
462-
}
463-
464460
return this.topology.startSession(options, this.s.options);
465461
};
466462

‎lib/operations/execute_operation.js‎

Lines changed: 47 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
'use strict';
22

3+
const maybePromise = require('../utils').maybePromise;
34
const MongoError = require('../core/error').MongoError;
45
const Aspect = require('./operation').Aspect;
56
const OperationBase = require('./operation').OperationBase;
@@ -21,7 +22,7 @@ const isUnifiedTopology = require('../core/utils').isUnifiedTopology;
2122
* @param {Operation} operation The operation to execute
2223
* @param {function} callback The command result callback
2324
*/
24-
function executeOperation(topology, operation, callback) {
25+
function executeOperation(topology, operation, cb) {
2526
if (topology == null) {
2627
throw new TypeError('This method requires a valid topology instance');
2728
}
@@ -30,64 +31,57 @@ function executeOperation(topology, operation, callback) {
3031
throw new TypeError('This method requires a valid operation instance');
3132
}
3233

33-
if (isUnifiedTopology(topology) && topology.shouldCheckForSessionSupport()) {
34-
return selectServerForSessionSupport(topology, operation, callback);
35-
}
36-
37-
const Promise = topology.s.promiseLibrary;
38-
39-
// The driver sessions spec mandates that we implicitly create sessions for operations
40-
// that are not explicitly provided with a session.
41-
let session, owner;
42-
if (topology.hasSessionSupport()) {
43-
if (operation.session == null) {
44-
owner = Symbol();
45-
session = topology.startSession({ owner });
46-
operation.session = session;
47-
} else if (operation.session.hasEnded) {
48-
throw new MongoError('Use of expired sessions is not permitted');
34+
return maybePromise(topology, cb, callback => {
35+
if (isUnifiedTopology(topology) && topology.shouldCheckForSessionSupport()) {
36+
// Recursive call to executeOperation after a server selection
37+
return selectServerForSessionSupport(topology, operation, callback);
4938
}
50-
}
51-
52-
let result;
53-
if (typeof callback !== 'function') {
54-
result = new Promise((resolve, reject) => {
55-
callback = (err, res) => {
56-
if (err) return reject(err);
57-
resolve(res);
58-
};
59-
});
60-
}
6139

62-
function executeCallback(err, result) {
63-
if (session && session.owner === owner) {
64-
session.endSession();
65-
if (operation.session === session) {
66-
operation.clearSession();
40+
// The driver sessions spec mandates that we implicitly create sessions for operations
41+
// that are not explicitly provided with a session.
42+
let session, owner;
43+
if (topology.hasSessionSupport()) {
44+
if (operation.session == null) {
45+
owner = Symbol();
46+
session = topology.startSession({ owner });
47+
operation.session = session;
48+
} else if (operation.session.hasEnded) {
49+
return callback(new MongoError('Use of expired sessions is not permitted'));
6750
}
51+
} else if (operation.session) {
52+
// If the user passed an explicit session and we are still, after server selection,
53+
// trying to run against a topology that doesn't support sessions we error out.
54+
return callback(new MongoError('Current topology does not support sessions'));
6855
}
6956

70-
callback(err, result);
71-
}
72-
73-
try {
74-
if (operation.hasAspect(Aspect.EXECUTE_WITH_SELECTION)) {
75-
executeWithServerSelection(topology, operation, executeCallback);
76-
} else {
77-
operation.execute(executeCallback);
78-
}
79-
} catch (e) {
80-
if (session && session.owner === owner) {
81-
session.endSession();
82-
if (operation.session === session) {
83-
operation.clearSession();
57+
function executeCallback(err, result) {
58+
if (session && session.owner === owner) {
59+
session.endSession();
60+
if (operation.session === session) {
61+
operation.clearSession();
62+
}
8463
}
64+
65+
callback(err, result);
8566
}
8667

87-
throw e;
88-
}
68+
try {
69+
if (operation.hasAspect(Aspect.EXECUTE_WITH_SELECTION)) {
70+
executeWithServerSelection(topology, operation, executeCallback);
71+
} else {
72+
operation.execute(executeCallback);
73+
}
74+
} catch (error) {
75+
if (session && session.owner === owner) {
76+
session.endSession();
77+
if (operation.session === session) {
78+
operation.clearSession();
79+
}
80+
}
8981

90-
return result;
82+
callback(error);
83+
}
84+
});
9185
}
9286

9387
function supportsRetryableReads(server) {
@@ -139,7 +133,6 @@ function executeWithServerSelection(topology, operation, callback) {
139133
callback(err, null);
140134
return;
141135
}
142-
143136
const shouldRetryReads =
144137
topology.s.options.retryReads !== false &&
145138
operation.session &&
@@ -156,31 +149,16 @@ function executeWithServerSelection(topology, operation, callback) {
156149
});
157150
}
158151

159-
// TODO: This is only supported for unified topology, it should go away once
160-
// we remove support for legacy topology types.
152+
// The Unified Topology runs serverSelection before executing every operation
153+
// Session support is determined by the result of a monitoring check triggered by this selection
161154
function selectServerForSessionSupport(topology, operation, callback) {
162-
const Promise = topology.s.promiseLibrary;
163-
164-
let result;
165-
if (typeof callback !== 'function') {
166-
result = new Promise((resolve, reject) => {
167-
callback = (err, result) => {
168-
if (err) return reject(err);
169-
resolve(result);
170-
};
171-
});
172-
}
173-
174155
topology.selectServer(ReadPreference.primaryPreferred, err => {
175156
if (err) {
176-
callback(err);
177-
return;
157+
return callback(err);
178158
}
179159

180160
executeOperation(topology, operation, callback);
181161
});
182-
183-
return result;
184162
}
185163

186164
module.exports = executeOperation;

‎test/functional/find.test.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1067,7 +1067,7 @@ describe('Find', function() {
10671067
{ $set: { name: 'test2' } },
10681068
{},
10691069
function(err, updated_doc) {
1070-
test.equal(null, updated_doc);
1070+
expect(updated_doc).to.not.exist;
10711071
test.ok(err != null);
10721072
client.close(done);
10731073
}
@@ -1305,7 +1305,7 @@ describe('Find', function() {
13051305
{ a: 10, b: 10, failIndex: 2 },
13061306
{ w: 1, upsert: true },
13071307
function(err, result) {
1308-
test.equal(null, result);
1308+
expect(result).to.not.exist;
13091309
test.ok(err.errmsg.match('duplicate key'));
13101310
client.close(done);
13111311
}

‎test/functional/insert.test.js‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1097,8 +1097,8 @@ describe('Insert', function() {
10971097
var db = client.db(configuration.db);
10981098
var collection = db.collection('Should_fail_on_insert_due_to_key_starting_with');
10991099
collection.insert(doc, configuration.writeConcernMax(), function(err, result) {
1100-
test.ok(err != null);
1101-
test.equal(null, result);
1100+
expect(err).to.exist;
1101+
expect(result).to.not.exist;
11021102

11031103
client.close(done);
11041104
});
@@ -1348,7 +1348,7 @@ describe('Insert', function() {
13481348

13491349
// Update two fields
13501350
collection.insert({ _id: 1 }, configuration.writeConcernMax(), function(err, r) {
1351-
test.equal(r, null);
1351+
expect(r).to.not.exist;
13521352
test.ok(err != null);
13531353
test.ok(err.result);
13541354

@@ -2560,7 +2560,7 @@ describe('Insert', function() {
25602560
[{ a: 1 }, { a: 2 }, { a: 1 }, { a: 3 }, { a: 1 }],
25612561
{ ordered: true },
25622562
function(err, r) {
2563-
test.equal(r, null);
2563+
expect(r).to.not.exist;
25642564
test.ok(err != null);
25652565
test.ok(err.result);
25662566

@@ -2601,7 +2601,7 @@ describe('Insert', function() {
26012601
[{ a: 1 }, { a: 2 }, { a: 1 }, { a: 3 }, { a: 1 }],
26022602
{ ordered: true },
26032603
function(err, r) {
2604-
test.equal(r, null);
2604+
expect(r).to.not.exist;
26052605
test.ok(err != null);
26062606
test.ok(err.result);
26072607

‎test/functional/operation_example.test.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2657,7 +2657,7 @@ describe('Operation Examples', function() {
26572657
],
26582658
{ w: 1, keepGoing: true },
26592659
function(err, result) {
2660-
test.equal(result, null);
2660+
expect(result).to.not.exist;
26612661
test.ok(err);
26622662
test.ok(err.result);
26632663

@@ -3115,7 +3115,7 @@ describe('Operation Examples', function() {
31153115

31163116
// Attemp to rename the first collection to the second one, this will fail
31173117
collection1.rename('test_rename_collection2', function(err, collection) {
3118-
test.equal(null, collection);
3118+
expect(collection).to.not.exist;
31193119
test.ok(err instanceof Error);
31203120
test.ok(err.message.length > 0);
31213121

‎test/unit/sessions/client.test.js‎

Lines changed: 22 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,9 @@ describe('Sessions', function() {
1414
});
1515
});
1616

17-
it('should throw an exception if sessions are not supported', {
17+
it('should not throw a synchronous exception if sessions are not supported', {
1818
metadata: { requires: { topology: 'single' } },
19-
test: function(done) {
19+
test() {
2020
test.server.setMessageHandler(request => {
2121
var doc = request.document;
2222
if (doc.ismaster) {
@@ -27,13 +27,11 @@ describe('Sessions', function() {
2727
});
2828

2929
const client = this.configuration.newClient(`mongodb://${test.server.uri()}/test`);
30-
client.connect(function(err, client) {
31-
expect(err).to.not.exist;
32-
expect(() => {
33-
client.startSession();
34-
}).to.throw(/Current topology does not support sessions/);
35-
36-
client.close(done);
30+
return client.connect().then(() => {
31+
expect(() => client.startSession()).to.not.throw(
32+
'Current topology does not support sessions'
33+
);
34+
return client.close();
3735
});
3836
}
3937
});
@@ -42,6 +40,7 @@ describe('Sessions', function() {
4240
metadata: { requires: { topology: 'single' } },
4341
test() {
4442
const replicaSetMock = new ReplSetFixture();
43+
let client;
4544
return replicaSetMock
4645
.setup({ doNotInitHandlers: true })
4746
.then(() => {
@@ -92,14 +91,23 @@ describe('Sessions', function() {
9291
return replicaSetMock.uri();
9392
})
9493
.then(uri => {
95-
const client = this.configuration.newClient(uri);
94+
client = this.configuration.newClient(uri);
9695
return client.connect();
9796
})
9897
.then(client => {
99-
expect(client.topology.s.description.logicalSessionTimeoutMinutes).to.not.exist;
100-
expect(() => {
101-
client.startSession();
102-
}).to.throw(/Current topology does not support sessions/);
98+
const session = client.startSession();
99+
return client
100+
.db()
101+
.collection('t')
102+
.insertOne({ a: 1 }, { session });
103+
})
104+
.then(() => {
105+
expect.fail('Expected an error to be thrown about not supporting sessions');
106+
})
107+
.catch(error => {
108+
expect(error.message).to.equal('Current topology does not support sessions');
109+
})
110+
.then(() => {
103111
return client.close();
104112
});
105113
}

0 commit comments

Comments
 (0)