feat(NODE-6258): add signal support to cursor APIs #4364
Conversation
src/cursor/abstract_cursor.ts
Outdated
| @@ -481,6 +495,7 @@ export abstract class AbstractCursor< | |||
| } | |||
|
|
|||
| yield document; | |||
| throwIfAborted(this.signal); | |||
There was a problem hiding this comment.
Should a line like this also be added before the await above?
There was a problem hiding this comment.
Yes, unless I'm mistaken I think generally we must always check the state before and after an await. I'm hoping to enumerate the possible state change (before we enter an await and after we resolve) in the tests in such a way that they will fail if I omit it 🤞🏻
src/utils.ts
Outdated
| export function throwIfAborted(signal?: { aborted?: boolean; reason?: any }): void { | ||
| if (signal?.aborted) { | ||
| throw new MongoAbortedError('Operation was aborted', { cause: signal.reason }); | ||
| } |
There was a problem hiding this comment.
I feel like it might be worth not wrapping the exception here, and just going with signal.reason itself. I'm not 100% sure, it is convenient that with this, you could still see that the exception originates from a MongoDB driver method, but at the same time, I feel like there's a decent expectation that if you abort an operation through an AbortSignal, then a) the reason provided when aborting will be the actual exception your code sees and b) the default value for it will be an AbortError instance. Additionally, that way it would be possible to adopt the standard throwIfAborted function in the future.
There was a problem hiding this comment.
I'm trying to reconcile when a reason, an AbortError instance, and, DOMException named "AbortError" is thrown in Node.js and seeing how that can inform our API:
(edit, omitted reason case is missing from screenshot, see below)
await fetch('http://google.com', { signal: AbortSignal.abort() })
Uncaught:
DOMException [AbortError]: This operation was aborted
at node:internal/deps/undici/undici:13178:13
at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
at async REPL24:1:33
await fs.promises.readFile('/dev/zero', { signal: AbortSignal.abort() })
Uncaught AbortError: The operation was aborted
at checkAborted (node:internal/fs/promises:473:11)
at Object.readFile (node:internal/fs/promises:1236:3)
at REPL25:1:51 {
code: 'ABORT_ERR',
[cause]: DOMException [AbortError]: This operation was aborted
at new DOMException (node:internal/per_context/domexception:53:5)
at AbortSignal.abort (node:internal/abort_controller:205:14)
at REPL25:1:95
at REPL25:2:4
at ContextifyScript.runInThisContext (node:vm:136:12)
at REPLServer.defaultEval (node:repl:598:22)
at bound (node:domain:432:15)
at REPLServer.runBound [as eval] (node:domain:443:12)
at REPLServer.onLine (node:repl:927:10)
at REPLServer.emit (node:events:532:35)
- https://github.com/nodejs/node/blob/main/lib/internal/fs/promises.js#L472-L475
- https://github.com/nodejs/node/blob/main/lib/internal/abort_controller.js#L229-L232
The behavior is interestingly different depending on web-ish/node-ish origins. 🤔
An error thrown inside the driver has control flow implications for an operation, throwing the right (or wrong) value with the right properties and you can make something retry that normally wouldn't have. This encourages me to make sure that the error thrown is one we control so we don't accidentally misinterpret an abort.
Perhaps not very useful to the downstream user but it does feel like a debuggability loss to not have the stack trace to where the abort was detected show up in the stack trace.
I am comfortable with the idea that we would always need a wrapper for throwIfAborted (rather than adopting the standard) if it means consistent error behavior, but I don't think I'm sure what is the best developer experience here.
There was a problem hiding this comment.
I suppose another part of this is that the user's signal may be handed over to an API we depend on (possibly someday, not in this PR, depending on need) and that API may turn the abort into a rejection using its own logic (either wrap, don't wrap, etc.) And unless we try catch and convert we're going to throw whatever decision is made inside the upstream API. 🤔
There was a problem hiding this comment.
Fair point, I wasn't aware that there's precedent for either behavior in standard-ish APIs.
I don't disagree with there being a bit of a loss of debuggability, but my gut feeling is still that it's best to stick to what the standard throwIfAborted() method does. Feel free to just resolve if you feel differently!
The behavior is interestingly different depending on web-ish/node-ish origins. 🤔
That's because Web APIs are designed, but Node.js APIs are slapped together
nbbeeken
force-pushed
the
NODE-6258-abortsignal
branch
from
5de94ee to
6af545c
Compare
Description
What is changing?
Is there new documentation needed for these changes?
Yes, API docs.
What is the motivation for this change?
AbortController/AbortSignal has become the defacto interruption mechanism for async operations. We're starting small by linking an abort signal to the lifetime of a cursor so that .next() / toArray() / for-await usage can be interrupted by external means.
Release Highlight
TODO
Double check the following
npm run check:lintscripttype(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript