-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Allow execution to be cancelled #3791
Conversation
✅ Deploy Preview for compassionate-pike-271cb3 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Hi @igrlk, I'm @github-actions bot happy to help you with this PR 👋 Supported commandsPlease post this commands in separate comments and only one per comment:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@igrlk The latest changes of this PR are available on NPM as Also you can depend on latest version built from this PR: |
@igrlk Something went wrong, please check log. |
This comment has been minimized.
This comment has been minimized.
@igrlk Please, see benchmark results here: https://github.com/graphql/graphql-js/actions/runs/3594181152/jobs/6052115992#step:6:1 |
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.
I'm not enough of an execution expert to know if the place where you're checking the signal is the best spot, but I'm really excited to see this! Use cases include:
- Stopping execution when the client closes their request.
- Stopping execution after a request timeout.
- Stopping execution when it evaluates too many fields.
I do have one suggestion! There's an issue I've know about in graphql-js for a while which apparently I never filed as a bug, so I just did that: #3792
If we're going to add this AbortSignal support to graphql-js, I'd suggest doing something slightly differently, where graphql-js always creates a new AbortController which it passes to execution the way to do here. If a signal is passed in then you do passedInSignal.addEventListener('abort', () => abortController.abort(passedInSignal.reason))
. Finally, when execution has finished and there's a complete response to be passed back (the same places you're unsubscribing) it can abort the controller it created. This means that if there's any pieces of execution doing work that it doesn't know contributes to an already-rejected Promise.all
, that work will stop recursing.
Another thought that doesn't go on a particular line: as a potential extension which doesn't need to be part of this PR, the signal could be made available to resolvers on GraphQLResolveInfo
, so that resolvers can themselves abort their work when the operation is aborted.
src/execution/execute.ts
Outdated
function subscribeToAbortSignal(exeContext: ExecutionContext): { | ||
unsubscribeFromAbortSignal: () => void; | ||
} { | ||
const onAbort = () => { |
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.
If you start the function with something like
const { signal } = exeContext;
if (!signal) {
return () => {};
}
does it get a bit easier to read (no more ?.
, can access signal
directly within onAbort without a conditional)?
src/execution/execute.ts
Outdated
@@ -832,6 +867,14 @@ function completeValue( | |||
result: unknown, | |||
asyncPayloadRecord?: AsyncPayloadRecord, | |||
): PromiseOrValue<unknown> { | |||
if (exeContext.signal?.isAborted) { |
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.
My instinct is that this is a global enough thing that if it fires, the overall response should be { data: null, errors: [ { message: "aborted" } ]}
rather than keeping whatever data
and errors
were already produced. (Maybe that's not a change to this code but just a suggestion that at the top level of executeImpl
it checks to see if it's aborted and if so replaces the whole response with a short one.)
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.
@IvanGoncharov @yaacovCR any instincts on this — does it make more sense for "execution stopped due to AbortSignal" to return a partial result with field errors for all resolvers that run after abortion, or just a short error with no data?
src/execution/execute.ts
Outdated
function subscribeToAbortSignal(exeContext: ExecutionContext): { | ||
unsubscribeFromAbortSignal: () => void; | ||
} { | ||
const onAbort = () => { |
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.
On the other hand, I don't think you need any of this. The AbortSignal already has an aborted
boolean property. Since all we're doing is checking whether things are aborted inside completeValue, that can just check aborted
directly, with no subscription needed. (Though if you take my suggestion from the top level of the review to always have an AbortController and hook up the user-provided one to it, then you'll still need the structure of subscribe/unsubscribe — it just won't be updating a boolean.)
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.
Decided to go with the option you suggested and create a new AbortController for each execution
src/execution/execute.ts
Outdated
@@ -261,6 +265,7 @@ export interface ExecutionArgs { | |||
fieldResolver?: Maybe<GraphQLFieldResolver<any, any>>; | |||
typeResolver?: Maybe<GraphQLTypeResolver<any, any>>; | |||
subscribeFieldResolver?: Maybe<GraphQLFieldResolver<any, any>>; | |||
signal?: AbortSignal; |
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.
ExecutionArgs is also the argument to subscribe
/experimentalSubscribeIncrementally
. So this PR allows you to specify signal
to subscription APIs. I would expect that if I passed signal
to a subscription API and then I aborted the signal at some point in the middle of a subscription (say, between source events), that it would also complete the subscription somehow, but I don't think your implementation does this.
Personally I think it would be reasonable for this PR to only work with non-subscription execution (and subscription support could be added later) — so my suggestion is for experimentalSubscribeIncrementally
to throw if signal
is provided, unless you're familiar with subscriptions and are motivated to make it work there too!
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.
Would be happy to try implementing it for Subscriptions but I think it makes sense to reduce this PR scope to minimal.
I added error throwing in case of a client tries to pass an abort signal to subscriptions execution, thanks for the suggestion!
src/execution/execute.ts
Outdated
@@ -261,6 +265,7 @@ export interface ExecutionArgs { | |||
fieldResolver?: Maybe<GraphQLFieldResolver<any, any>>; | |||
typeResolver?: Maybe<GraphQLTypeResolver<any, any>>; | |||
subscribeFieldResolver?: Maybe<GraphQLFieldResolver<any, any>>; | |||
signal?: AbortSignal; |
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.
I think it would be reasonable to add this to the top-level GraphQLArgs
/graphql
function as well.
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.
Not quite sure I understand the idea, can you explain why?
The way I understand it: clients (graphql servers) are calling execute/experimentalExecuteIncrementally
and that's exactly the place where they will be passing an abort signal.
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.
There's a function called graphql
that you can use that wraps a bunch of stuff (parsing, validation, execution) into a single function call. So it might be reasonable for users of that API to be able to set a signal (it's just a matter of adding it to GraphQLArgs and threading it through to execute).
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.
Thanks. I added signal to the GraphQLArgs/graphql call
src/execution/execute.ts
Outdated
@@ -832,6 +867,14 @@ function completeValue( | |||
result: unknown, | |||
asyncPayloadRecord?: AsyncPayloadRecord, | |||
): PromiseOrValue<unknown> { | |||
if (exeContext.signal?.isAborted) { |
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.
Hmm, also: I think completeValue is probably the wrong place to put this, or at least if you do put this here you need to be a bit more careful. Because if result
is an array of Promises, now you are "leaking" the Promises by not calling catch
or putting them in a Promise.all
or whatever, which seems incorrect... it's generally a bad idea to have unhandled Promises and can lead to weird logging and such.
I don't know the best place to put this check (and maybe it should go in multiple places) but putting it at the top of executeField (ie, before doing a resolver call, not midway through after) seems reasonable?
(Or you could leave it here but also be careful to find any Promise
s in result
and put an empty catch
on them or something...)
There's some CI failures. The integration test failures indicate that you probably either need to require some One of my suggestions involves the need to actually In Apollo Server we've been happy with https://www.npmjs.com/package/node-abort-controller but that's Node-specific; https://www.npmjs.com/package/abort-controller is fully portable I think. Although I guess |
Second that this is very exciting. My thoughts around adding an abort mechanism were more centered on #3792 ie not receiving an abortsignal, but rather passing it into resolvers. I assumed that we would end up needing to create a separate AbortController for every nullable field, so that we could abort resolvers as soon as a null bubbled up. I hadn't thought of creating a single AbortController for everything and simply aborting after the result has been returned, ie @glasser's suggestion above. The down side is that we end up having to wait to cancel anything until we get the final result, while the upside is that it's easier to manage and may have a performance boost in the non-error case. Following along here with interest! |
@yaacovCR yeah I think we could do a more complex thing to manage extraneous execution more precisely (perhaps by moving away from Promise.all to something more explicit) but that could be an iterative improvement later. |
50bf3b6
to
14762e7
Compare
@glasser, I've added dummy AbortSignal class in case of Node@14 - tests pass now! |
}), | ||
resolve: () => | ||
new Promise((resolve) => { | ||
setTimeout(() => resolve({}), WAIT_MS_BEFORE_RESOLVING); |
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.
FWIW I don't see any other uses of setTimeout in the test suite. Is it possible to avoid setTimeout and just have some Promises created in the test that are manually resolved or not resolved as part of the test? setTimeout-based tests can get flaky under load...
src/jsutils/AbortController.ts
Outdated
class DummyAbortController implements IAbortController { | ||
private _signal: IAbortSignal = { | ||
aborted: false, | ||
addEventListener: () => null, |
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.
I guess the reason that this is valid is because we don't actually provide the AbortSignal to any code other than the code in graphql-js itself, which doesn't actually call addEventListener at all. If we're going to do that then we might as well go back to something more similar to your original code where we listen on the incoming AbortSignal to update some boolean, and then we also update that boolean when execution is done, rather than have an AbortSignal that doesn't actually support this important method... though I would honestly prefer a system where the AbortSignal is real so it can be provided to resolvers (which requires a more complete polyfill, or the concept that this particular feature only works if you're in a JS environment with a global AbortController). @IvanGoncharov thoughts?
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.
Are there any plans on dropping support for node@14? Would be cool to track it down somewhere so that people know when it's going to happen.
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.
I don't know if graphql-js has any particular plans, but Node v14 does go out of LTS in April. (I'm not sure what non-Node/browser platforms graphql-js has to work on though?)
It might be reasonable to just only do the "cancel stuff when execution is over" if global.AbortController
exists. It's a bit more annoying to do this partial support if we add an AbortSignal to GraphQLRequestInfo since it would be nice if the TS type of the field is non-optional — but we could have that field be optional for now (with a "if you know you're in an AbortController-in-global environment then you can !
it") and non-optional once Node 14 support is dropped.
src/execution/execute.ts
Outdated
@@ -832,6 +867,14 @@ function completeValue( | |||
result: unknown, | |||
asyncPayloadRecord?: AsyncPayloadRecord, | |||
): PromiseOrValue<unknown> { | |||
if (exeContext.signal?.isAborted) { |
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.
@IvanGoncharov @yaacovCR any instincts on this — does it make more sense for "execution stopped due to AbortSignal" to return a partial result with field errors for all resolvers that run after abortion, or just a short error with no data?
@igrlk Sorry for the delay in the review. Also, I want to try removing delays from a test since we don't use them anywhere else, and the performance of the tests is critical for mutation testing. That said, plan to check as much as possible and merge it tomorrow. |
Thank you for checking this, @IvanGoncharov. One thing I think is left besides what you mentioned - @glasser's question here regarding where the check for aborted execution should happen - in |
Update: I managed to remove
Need to think about it more, will continue tomorrow morning. |
cf48da4
to
ce44ff7
Compare
@igrlk Sorry for the long delay. It's ended up being a more complicated task than I initially thought.
|
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.
Thanks a lot for making this huge change @IvanGoncharov! I left some comments - please let me know what you think.
I'm also happy to follow up on this and finalise it (linting and other things left before merging). But I'll only have time by the end of the week.
src/execution/execute.ts
Outdated
@@ -261,6 +262,7 @@ export interface ExecutionArgs { | |||
fieldResolver?: Maybe<GraphQLFieldResolver<any, any>>; | |||
typeResolver?: Maybe<GraphQLTypeResolver<any, any>>; | |||
subscribeFieldResolver?: Maybe<GraphQLFieldResolver<any, any>>; | |||
signal?: AbortSignal | undefined; |
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.
Is AbortSignal defined globally in node@14? I think that I intentionally created an interface for it since it was not defined there, but I might be mistaken, need to double check
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.
It's not, but TypeScript has typings for it.
Typescript knows what it is even on node@14, but it's only during type check in execution you can't call it on node@14 that's why I add typeof AbortController === 'undefined'
check.
errors: [], | ||
}; | ||
} | ||
|
||
class ExecutionController { | ||
/** For performance reason we can't use `signal.isAborted` so we cache it here */ | ||
isAborted: boolean = false; |
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.
For more context, what are the performance implications you mention here?
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.
I will add a screenshot tomorrow.
But you can run npm run benchmark
(locally) on top of my commit and see the difference.
src/execution/execute.ts
Outdated
isAborted: boolean = false; | ||
|
||
private readonly _passedInAbortSignal: AbortSignal | undefined; | ||
private readonly _abortController: AbortController | undefined = |
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.
Same as about AbortSignal
- I believe it was causing an error in node@14 so I had to add the interfaces that match AbortSignal
and AbortController
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.
I will check on node@14. Maybe my assumption about TS detecting this type on node@14 is wrong.
return this._abortController?.signal; | ||
} | ||
|
||
abort(reason?: unknown) { |
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.
Can reason
be typed as a string here?
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.
in normal use, it should be an Error
object, but JS allows it to be a value at all, so we have no control over it 🤷♂️
This comment has been minimized.
This comment has been minimized.
@igrlk The latest changes of this PR are available on NPM as Also you can depend on latest version built from this PR: |
…hen resolving every field
Motivation: technically node14 end of life will happen in two months so we can still support it but it creates problems with graphql#3791 Node14 doesn't support `AbortSignal` and since we want to allow resolvers to be canceled we need to expose this object through our public API (inside `info` argument). Potentialy this and all the other issues (e.g. how to test this feature on node14) can be worked around but complicates solution. So I think, it worth to deprecate node14 two months earlier in order to unblock graphql#3791 especially since we still in alpha stage. Less critical but still significant problem with node14 is that it ships with npm6 which doesn't support default format of the lockfile used by npm9 and this make updating dependencies trickier.
Motivation: technically node14 end of life will happen in two months so we can still support it but it creates problems with graphql#3791 Node14 doesn't support `AbortSignal` and since we want to allow resolvers to be canceled we need to expose this object through our public API (inside `info` argument). Potentialy this and all the other issues (e.g. how to test this feature on node14) can be worked around but complicates solution. So I think, it worth to deprecate node14 two months earlier in order to unblock graphql#3791 especially since we still in alpha stage. Less critical but still significant problem with node14 is that it ships with npm6 which doesn't support default format of the lockfile used by npm9 and this make updating dependencies trickier.
Hey, we ran into issues with timeouts that started this whole chain and want to help get this over the finish line, I've tried to rebase @igrlk code onto newest main and it seems to pass completely fine (https://github.com/ladal1/graphql-js), do we need any more changes done (honestly got lost here in all the discussion) |
Thanks, @ladal1 |
Interesting, the tests were passing on my end when I copied it, thanks for the rebase, I'll take a look into those tests and try to make them pass ASAP |
Thanks @ladal1 , I've invited you to be a collaborator in the fork |
77e6176
to
910a4ae
Compare
This comment has been minimized.
This comment has been minimized.
@ladal1 Please, see benchmark results here: https://github.com/graphql/graphql-js/actions/runs/6381598227/job/17318384919#step:6:1 |
I'm fairly sure the behaviour is fixed, tests pass for me now, seems that either test edit of rebase mess caused the issues |
We've intentionally added
|
Re-comitting from my account to run pr checks, @ladal1 |
@igrlk I thought they weren't intentional as they didn't appear in the copy I had (but I might have had wrong revision), I'll try to take a look as to why they would be there and if it's possible to pass without removing them |
Can't find why it should be there (and especially in all branches of the logic) |
This comment has been minimized.
This comment has been minimized.
@igrlk The latest changes of this PR are available on NPM as Also you can depend on latest version built from this PR: |
@ladal1 thanks for checking. I tried to reproduce cancellation in this repo, but if previously it was working as expected (at least in the initial versions of this PR), now it seems to hang and not finish the execution at all + the execution is not being aborted. Will try to find some time to figure out why, but not sure when |
This adds support for aborting execution from the outside or resolvers, this adds a few tests and tries to make the support as easy as possible. Do we want to support having abort support on subscriptions, I guess it makes sense for server-sent events. I've chosen 2 places to place these interrupts - `executeFieldsSerially` - every time we start a new mutation we check whether the runtime has interrupted - `executeFields` - every time we start executing a new field we check whether the runtime has interrupted - inside of the catch block as well so we return a singular error, all though this doesn't really matter as the consumer would not receive anything - this here should also take care of deferred fields When comparing this to `graphql-tools/execute` I am not sure whether we want to match this behavior, this throws a DomException which would be a whole new exception that gets thrown while normally during execution we wrap everything with GraphQLErrors. Supersedes #3791 Resolves #3764 Co-authored-by: yaacovCR <[email protected]>
Implemented in #4250 |
Hey everyone 👋
This PR addresses #3764
How it works
It extends
ExecutionArgs
withsignal?: AbortSignal
.Then, during the execution, we check whether signal is present and aborted. If that's the case, we throw an error and fail the further execution of resolvers.
Testing
To test it with a real graphql server (and to possibly give an implementation reference to the apollo-server team), I've made a change to Mercurius so that it passes
abortSignal
to thegraphql.experimentExecuteIncrementally
and aborts the signal if request connection is closed.I've also created a minimal reproducible repo that you can clone and test the execution cancelling on your machine 🔥
Feedback
Please let me know if that change makes sense to you.
@glasser that would be awesome if you can play around with apollo-server to see if this way of execution cancelling can be used there :) I deployed this PR to npm and you can install it by
npm install --save graphql@canary-pr-3791
@mcollina, I would love to hear your thoughts too!
Cheers! 🎩