Allow execution to be cancelled

[igrlk]

Hey everyone 👋

This PR addresses https://github.com/graphql/graphql-js/issues/3764

How it works

It extends ExecutionArgs with signal?: 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 the graphql.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! 🎩

[netlify[bot]]

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Name	Link
🔨 Latest commit	e781bbcea685d22c8d84af996addfdded6c8bb27
🔍 Latest deploy log	https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/651bbac97de5e80008e89d05
😎 Deploy Preview	https://deploy-preview-3791--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[github-actions[bot]]

Hi @igrlk, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

• @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
• @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

[igrlk]

@github-actions publish-pr-on-npm

[igrlk]

@github-actions run-benchmark

[github-actions[bot]]

@github-actions publish-pr-on-npm

@igrlk The latest changes of this PR are available on NPM as graphql@17.0.0-alpha.2.canary.pr.3791.264f22163eb937ff87a420be9f7d45965f2cbf07 Note: no gurantees provided so please use your own discretion.

Also you can depend on latest version built from this PR: npm install --save graphql@canary-pr-3791

[github-actions[bot]]

@github-actions run-benchmark

@igrlk Something went wrong, please check log.

[igrlk]

@github-actions run-benchmark

[github-actions[bot]]

@github-actions run-benchmark

@igrlk Please, see benchmark results here: https://github.com/graphql/graphql-js/actions/runs/3594181152/jobs/6052115992#step:6:1

[glasser]

There's some CI failures. The integration test failures indicate that you probably either need to require some @types/node in packages that depend on graphql, or you can get the TS AbortSignal from some other package.

One of my suggestions involves the need to actually new AbortController() at runtime too, which raises the question of what implementation to use from being a TS issue to a runtime issue. I believe we need graphql-js (even execution, right?) to work in browsers and other non-Node environments, though I don't know how old the browsers we need to support are (and I don't know if we need to support environments other than browsers and Node). But in any case we still support Node v14 and the AbortController and AbortSignal classes aren't available as globals (without an experimental CLI flag) until v15, so you need some sort of polyfill anyway... so might as well use the polyfill for TS as well.

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 graphql-js has zero dependencies (is that an absolute requirement @IvanGoncharov ?) In that case the choices (assuming you like my suggestion that graphql-js should use its own AbortController to resolve #3792) are to only fix the #3792 bug in Node 16 and newer, or to copy a polyfill of AbortController into graphql-js.

[yaacovCR]

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!

[glasser]

@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.

[igrlk]

There's some CI failures

@glasser, I've added dummy AbortSignal class in case of Node@14 - tests pass now!

[IvanGoncharov]

@igrlk Sorry for the delay in the review. I like the idea of reusing the abort functionality of a fetch. Before merging, I want to do a quick check to ensure that API matches the W3C standards.

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. If delays can't be easily removed, I'm ok to merge it as is, and I just want to try before merging.

That said, plan to check as much as possible and merge it tomorrow.

[igrlk]

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 completeValue or no and how we should handle it. Do you have an opinion on that?

[IvanGoncharov]

Update: I managed to remove setTimeout from tests but stuck on few other things including

@glasser's question https://github.com/graphql/graphql-js/pull/3791#discussion_r1038543598 regarding where the check for aborted execution should happen - in completeValue or no and how we should handle it. Do you have an opinion on that?@glasser's question https://github.com/graphql/graphql-js/pull/3791#discussion_r1038543598 regarding where the check for aborted execution should happen - in completeValue or no and how we should handle it. Do you have an opinion on that?@glasser's question https://github.com/graphql/graphql-js/pull/3791#discussion_r1038543598 regarding where the check for aborted execution should happen - in completeValue or no and how we should handle it. Do you have an opinion on that?

Need to think about it more, will continue tomorrow morning.

[IvanGoncharov]

@igrlk Sorry for the long delay. It's ended up being a more complicated task than I initially thought. I made some changes:

1. Pass signal into resolvers
2. move the place where we interrupt execution before we call resolver per @glasser suggestion
3. Fix interruption of steam/defer
4. Implement it on subscriptions (need to fix some edge cases) It failing some lints + need more testing, mainly steam/defer and subscription. But all existing tests are passing, including the new test added by this PR. Plan to finish it tomorrow but want to get early feedback on the changes.

[igrlk]

@github-actions publish-pr-on-npm

[github-actions[bot]]

@github-actions publish-pr-on-npm

@igrlk The latest changes of this PR are available on NPM as graphql@17.0.0-alpha.2.canary.pr.3791.e6d3ec58026d75b71b7b84c3da5f376ec7eeca94 Note: no gurantees provided so please use your own discretion.

Also you can depend on latest version built from this PR: npm install --save graphql@canary-pr-3791

[igrlk]

AbortController and AbortSignal were not defined and failing the tests so I added the interfaces for them

[igrlk]

@github-actions publish-pr-on-npm

[igrlk]

@github-actions run-benchmark

[github-actions[bot]]

@github-actions publish-pr-on-npm

@igrlk The latest changes of this PR are available on NPM as graphql@17.0.0-alpha.2.canary.pr.3791.22288c73e61ad3ca68687546f2058561e41fcc93 Note: no gurantees provided so please use your own discretion.

Also you can depend on latest version built from this PR: npm install --save graphql@canary-pr-3791

[igrlk]

@IvanGoncharov I tried to fix all the PR errors but there is one I'm not sure how to fix: C8 on Node 14 reports about missing coverage but there is no real missing coverage, c8 ignore also doesn't help. Do you have an idea of what it can be?

Besides that, I tested the latest version of the branch using this repo that just does simple scenario - 3 nested resolvers. The change works as expected (Although we now see error message in the console when execution stops, is it expected?)

[github-actions[bot]]

@github-actions run-benchmark

@igrlk Please, see benchmark results here: https://github.com/graphql/graphql-js/actions/runs/4216489486/jobs/7318349660#step:6:1

[ladal1]

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)

[igrlk]

Thanks, @ladal1 I've rebased the branch, fixed linters, but there are some failing tests. I don't have time to check it this week. Would you happen to have time to look into that?

[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

[igrlk]

Thanks @ladal1 , I've invited you to be a collaborator in the fork

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: igrlk / name: Igor Luchenkov (f8b148f840fec29067ce34413948846c24232b85, e449cae02b64209005bf5903c8c312d2ea4c4096, f47dee791741d6d0e9d4f4b2b8bfb9d5429ffca7, e4b51d377d08a01b34e82d28fa14ea3487c06527, 8f84774296425752ccfbdd68f34c75356af3104c, b4751f45ccc0675b84fda43d466afa531743f651, b6b38aefe55b7ef775a097d0bd5d7d5025a284f1, 88cd4246297b1aea0aed9c1ff314c2e754182fe4, 11a5d321bd45f9b58694e3fcdb0bd17ab25ab471, 8ca80ddfcdd6b93de0b903bc54a97a8d5a5ffe6f, b15b818684ab58748ba0c1097bb3b5f5151168fd, fa044f583ec2e40bcbaf16eb3782ed5757d14eea, 6fa5336c3c23984c7e7bc6eae5ea59283cdfca34, 01c64ace878b4d4b813187b85f3bae4653c97f89, 1f851b3fb7c5e5d9749b938b06ce937289ca79fa)
• :white_check_mark: login: IvanGoncharov / name: Ivan Goncharov (2baa1e3ae5230c6f7e32bc9cc2c267b80d39006e, 256f780ba7b8dfd4b789891d238bb979a4a60e51)
• :white_check_mark: login: ladal1 (e781bbcea685d22c8d84af996addfdded6c8bb27)

[ladal1]

@github-actions run-benchmark