-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(NODE-7223): run checkout on connect regardless of credentials #4712
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
2005e21
a4a22d0
9f0e6b2
12f61e5
a7feb2c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -81,8 +81,6 @@ export async function executeOperation< | |
session = client.startSession({ owner, explicit: false }); | ||
} else if (session.hasEnded) { | ||
throw new MongoExpiredSessionError('Use of expired sessions is not permitted'); | ||
} else if (session.snapshotEnabled && !topology.capabilities.supportsSnapshotReads) { | ||
throw new MongoCompatibilityError('Snapshot reads require MongoDB 5.0 or later'); | ||
} else if (session.client !== client) { | ||
throw new MongoInvalidArgumentError('ClientSession must be from the same MongoClient'); | ||
} | ||
|
@@ -206,6 +204,10 @@ async function tryOperation<T extends AbstractOperation, TResult = ResultTypeFro | |
signal: operation.options.signal | ||
}); | ||
|
||
if (session?.snapshotEnabled && !topology.capabilities.supportsSnapshotReads) { | ||
throw new MongoCompatibilityError('Snapshot reads require MongoDB 5.0 or later'); | ||
} | ||
Comment on lines
+207
to
+209
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that server selection may not run above, we have stale "supportsSnapshotReads" information, however, this is always been true when relying upon this value before running server selection which may trigger monitoring checks to get the latest topology info. |
||
|
||
const hasReadAspect = operation.hasAspect(Aspect.READ_OPERATION); | ||
const hasWriteAspect = operation.hasAspect(Aspect.WRITE_OPERATION); | ||
const inTransaction = session?.inTransaction() ?? false; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -464,20 +464,14 @@ export class Topology extends TypedEventEmitter<TopologyEvents> { | |
}; | ||
|
||
try { | ||
const server = await this.selectServer( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this change necessary? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This introduces latency while the driver attempts to discover the topology for a different read preference than the auto-connecting operation is using. Since this selectServer uses MongoClient ReadPreference or default primary and the operation can use secondary or specific tagSets we block in auto-connect for longer than needed or end up throwing |
||
readPreferenceServerSelector(readPreference), | ||
selectServerOptions | ||
); | ||
|
||
const skipPingOnConnect = this.s.options.__skipPingOnConnect === true; | ||
if (!skipPingOnConnect && this.s.credentials) { | ||
if (!skipPingOnConnect) { | ||
const server = await this.selectServer( | ||
readPreferenceServerSelector(readPreference), | ||
selectServerOptions | ||
); | ||
const connection = await server.pool.checkOut({ timeoutContext: timeoutContext }); | ||
server.pool.checkIn(connection); | ||
stateTransition(this, STATE_CONNECTED); | ||
this.emit(Topology.OPEN, this); | ||
this.emit(Topology.CONNECT, this); | ||
|
||
return this; | ||
} | ||
|
||
stateTransition(this, STATE_CONNECTED); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ import { | |
type Collection, | ||
MongoClient, | ||
MongoNotConnectedError, | ||
MongoOperationTimeoutError, | ||
ProfilingLevel, | ||
Topology, | ||
TopologyType | ||
|
@@ -862,12 +863,17 @@ describe('When executing an operation for the first time', () => { | |
sinon.restore(); | ||
}); | ||
|
||
it('client.connect() takes as long as selectServer is delayed for and does not throw a timeout error', async function () { | ||
it('client.connect() takes as long as selectServer is delayed for and throws a timeout error', async function () { | ||
const start = performance.now(); | ||
expect(client.topology).to.not.exist; // make sure not connected. | ||
const res = await client.db().collection('test').insertOne({ a: 1 }, { timeoutMS: 500 }); // auto-connect | ||
const error = await client | ||
.db() | ||
.collection('test') | ||
.insertOne({ a: 1 }, { timeoutMS: 500 }) | ||
.catch(error => error); | ||
Comment on lines
+869
to
+873
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is a bit weird but maybe this is just a test clean up? Auto-connect has not started respecting So this is correct for the change we're making, the bit about not double server selecting, but the outcome is that auto-connecting operations are now subject to the higher cost of discovery (which can also be true for any operation at any time, but arguably less likely). Citing our docs:
I think this change makes sense and I can make it clear in the release notes here. |
||
const end = performance.now(); | ||
expect(res).to.have.property('acknowledged', true); | ||
expect(error).to.be.instanceOf(MongoOperationTimeoutError); | ||
expect(error).to.match(/Timed out during server selection/); | ||
expect(end - start).to.be.within(1000, 1500); // timeoutMS is 1000, did not apply. | ||
}); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,18 +3,19 @@ import * as path from 'path'; | |
import { loadSpecTests } from '../../spec'; | ||
import { runUnifiedSuite } from '../../tools/unified-spec-runner/runner'; | ||
|
||
describe.skip('Server Selection Unified Tests (Spec)', function () { | ||
describe('Server Selection Unified Tests (Spec)', function () { | ||
const tests = loadSpecTests(path.join('server-selection', 'logging')); | ||
runUnifiedSuite(tests, test => { | ||
if ( | ||
[ | ||
'Failed bulkWrite operation: log messages have operationIds', | ||
'Successful bulkWrite operation: log messages have operationIds' | ||
'Failed client bulkWrite operation: log messages have operationIds', | ||
'Successful bulkWrite operation: log messages have operationIds', | ||
'Successful client bulkWrite operation: log messages have operationIds' | ||
Comment on lines
+12
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Old bulkWrite we knew we didn't have operationId support but by nature of copying out Node.js specific versions of these tests did we miss adding support to client.bulkWrite too? I have not looked into what it is or does, so maybe it doesn't fit into our API well. |
||
].includes(test.description) | ||
) { | ||
return 'not applicable: operationId not supported'; | ||
} | ||
return false; | ||
}); | ||
}).skipReason = | ||
'TODO: unskip these tests - NODE-2471 (ping on connect) and NODE-5774 (duplicate server selection for bulkWrite and other wrapper operations'; | ||
}); |
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dunno if this matters but I get redirected when I visit the original, so figured we could skip the redirect?