-
Notifications
You must be signed in to change notification settings - Fork 1.8k
refactor(NODE-6187): refactor to use TimeoutContext abstraction #4131
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
Changes from 12 commits
5c945e5
4149b32
8bcff1f
bcbbc45
6fe0c86
47ad495
bb5c7d0
08651b6
f8ddf6a
9936f3b
ed637ed
10dd2a1
8a770a8
926e61b
8ac2b7e
89cb002
c3b15ec
c9b217e
55a139c
db06d9f
d2c3878
e200944
2461b12
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 |
---|---|---|
|
@@ -27,7 +27,7 @@ import { | |
} from '../sdam/server_selection'; | ||
import type { Topology } from '../sdam/topology'; | ||
import type { ClientSession } from '../sessions'; | ||
import { Timeout } from '../timeout'; | ||
import { TimeoutContext } from '../timeout'; | ||
import { squashError, supportsRetryableWrites } from '../utils'; | ||
import { AbstractOperation, Aspect } from './operation'; | ||
|
||
|
@@ -118,6 +118,14 @@ export async function executeOperation< | |
); | ||
} | ||
|
||
const timeoutContext = TimeoutContext.create({ | ||
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. Just out of curiosity - why are we instantiating a timeout here and not in 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. My intention was that we construct exactly one I am open to updating our Wouldn't constructing the 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.
Yeah, you're right that resolveOptions would lead to instantiation of timeouts we don't need, specifically when we construct dbs and collections. I'm not sure an operation owning a timeout context is the right choice either - I think that cursors / change streams will need to "own" their timeout context and they'll perform multiple operations inside the same context. Maybe we can leave it as-is for now and reconsider once we've implemented CSOT for cursors. |
||
serverSelectionTimeoutMS: client.options.serverSelectionTimeoutMS, | ||
waitQueueTimeoutMS: client.options.waitQueueTimeoutMS, | ||
timeoutMS: operation.options.timeoutMS | ||
}); | ||
|
||
operation.timeoutContext = timeoutContext; | ||
|
||
const readPreference = operation.readPreference ?? ReadPreference.primary; | ||
const inTransaction = !!session?.inTransaction(); | ||
|
||
|
@@ -153,13 +161,10 @@ export async function executeOperation< | |
selector = readPreference; | ||
} | ||
|
||
const timeout = operation.timeoutMS != null ? Timeout.expires(operation.timeoutMS) : undefined; | ||
operation.timeout = timeout; | ||
|
||
const server = await topology.selectServer(selector, { | ||
session, | ||
operationName: operation.commandName, | ||
timeout | ||
timeoutContext: operation.timeoutContext | ||
}); | ||
|
||
if (session == null) { | ||
|
@@ -270,9 +275,9 @@ async function retryOperation< | |
// select a new server, and attempt to retry the operation | ||
const server = await topology.selectServer(selector, { | ||
session, | ||
timeout: operation.timeout, | ||
operationName: operation.commandName, | ||
previousServer | ||
previousServer, | ||
timeoutContext: operation.timeoutContext | ||
}); | ||
|
||
if (isWriteOperation && !supportsRetryableWrites(server)) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ import { type BSONSerializeOptions, type Document, resolveBSONOptions } from '.. | |
import { ReadPreference, type ReadPreferenceLike } from '../read_preference'; | ||
import type { Server } from '../sdam/server'; | ||
import type { ClientSession } from '../sessions'; | ||
import { type Timeout } from '../timeout'; | ||
import { type Timeout, type TimeoutContext } from '../timeout'; | ||
import type { MongoDBNamespace } from '../utils'; | ||
|
||
export const Aspect = { | ||
|
@@ -67,6 +67,9 @@ export abstract class AbstractOperation<TResult = any> { | |
/** @internal */ | ||
timeoutMS?: number; | ||
|
||
/** @internal */ | ||
timeoutContext!: TimeoutContext; | ||
|
||
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. NOTE: This is marked with a non-null assertion because the field will be set during execute_operation and only referenced within that function in the following places at which point, if it is not defined, this would indicate a bug in the driver. I can add an inline comment explaining this
|
||
[kSession]: ClientSession | undefined; | ||
|
||
constructor(options: OperationOptions = {}) { | ||
|
@@ -82,8 +85,6 @@ export abstract class AbstractOperation<TResult = any> { | |
this.options = options; | ||
this.bypassPinningCheck = !!options.bypassPinningCheck; | ||
this.trySecondaryWrite = false; | ||
|
||
this.timeoutMS = options.timeoutMS; | ||
} | ||
|
||
/** Must match the first key of the command object sent to the server. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -273,7 +273,7 @@ export class Server extends TypedEventEmitter<ServerEvents> { | |
public async command( | ||
ns: MongoDBNamespace, | ||
command: Document, | ||
options?: CommandOptions | ||
options: CommandOptions | ||
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. there are no instances in the driver where we don't pass options into this method |
||
): Promise<Document>; | ||
|
||
public async command( | ||
|
Uh oh!
There was an error while loading. Please reload this page.