refactor(NODE-6187): refactor to use TimeoutContext abstraction #4131
Conversation
W-A-James
force-pushed
the
NODE-6090
branch
3 times, most recently
from
e3b7a63 to
61564cd
Compare
src/sdam/server.ts
Outdated
| @@ -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.
there are no instances in the driver where we don't pass options into this method
src/operations/operation.ts
Outdated
| /** @internal */ | ||
| timeoutContext!: TimeoutContext; | ||
|
|
There was a problem hiding this comment.
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
Topology.selectServerServer.commandConnectionPool.checkOutConnection.command
src/timeout.ts
Outdated
| export type TimeoutContextOptions = { | ||
| timeoutMS?: number; | ||
| serverSelectionTimeoutMS: number; | ||
| waitQueueTimeoutMS: number; | ||
| socketTimeoutMS?: number; | ||
| }; |
There was a problem hiding this comment.
There are going to be some places, I think, where we could use a timeout context but we won't have connection checkout or server selection (I'm thinking RTT pinger, moniters, connect / handshakes, and auth). This might not be relevant here, but once we have timeouts applied to the connection layer any place that uses connection.command() should pass one in.
Could we make serverSelectionTimeoutMS and waitQueueTimeoutMS optional?
There was a problem hiding this comment.
If we were to make serverSelectionTimeoutMS optional, would we want to return a null/0 timeout when requested for the serverSelectionTimeout?
src/operations/execute_operation.ts
Outdated
| @@ -118,6 +118,14 @@ export async function executeOperation< | |||
| ); | |||
| } | |||
|
|
|||
| const timeoutContext = TimeoutContext.create({ | |||
There was a problem hiding this comment.
Just out of curiosity - why are we instantiating a timeout here and not in resolveOptions?
There was a problem hiding this comment.
My intention was that we construct exactly one TimeoutContext per operation with a lifetime equal to that of the operation. The first convergent place we have access to both the client and operation options would be in executeOperation, but the absolute first place is in each of our operation helper methods (insertOne, updateOne, etc).
I am open to updating our AbstractOperation constructor to accept the client options we'd need to construct the TimeoutContext and updating these methods since that would be cleaner and make clear that each operation "owns" its TimeoutContext.
Wouldn't constructing the TimeoutContext in resolveOptions lead to multiple TimeoutContext objects being constructed that likely wouldn't be used.
There was a problem hiding this comment.
I am open to updating our AbstractOperation constructor to accept the client options we'd need to construct the TimeoutContext and updating these methods since that would be cleaner and make clear that each operation "owns" its TimeoutContext.
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.
Yes, but more specifically it would be that it could take up to |
On closer inspection, the only place where this isn't dead code is when change streams are selecting the server on encountering an error (see Addressing this would also let us make |
Maybe address it later? We don't actually provide a timeout to |
Description
What is changing?
New
TimeoutContextabstractionTimeoutContextabstraction that centralizes creation ofTimeoutinstances and implementation of legacy or CSOT timeout behaviourTimeoutContexttoserver.commandinvocations for allCommandOperationclassesTimeoutContextIs there new documentation needed for these changes?
What is the motivation for this change?
Release Highlight
Fill in title or leave empty for no highlight
Double check the following
npm run check:lintscripttype(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript