graphql-js icon indicating copy to clipboard operation
graphql-js copied to clipboard

Fix crash in node when mixing sync/async resolvers (backport of #3529)

Open chrskrchr opened this issue 3 years ago • 16 comments

Fixes #3528

This PR is a clone of #3529, which is currently held up by CLA approvals. Full credit goes to @asztal for identifying and fixing the underlying issue. @hobby203, the current steward of the original PR, has given their blessing for the PR to be re-created.

@asztal's notes from the original PR:

Ensures that if executeFields encounters a mix of promises and thrown errors, the promises will be awaited before throwing the error.

This does technically change executeFields to return a rejected promise in this scenario as opposed to throwing the not-null error synchronously, but I figured if any of the resolvers are asynchronous then returning a promise will be expected anyway, and this was better than crashing the process.

Note: I had to add an unhandledRejection event listener myself, as it seems mocha doesn't do it.

This PR has been backported to the 15.x branch as #3573 and to the 16.x branch as #3576.

chrskrchr avatar Jun 17 '22 15:06 chrskrchr

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
Latest commit 5f302345e008c68f39c5bfbdc7392135cb20f367
Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/62ac983ab95f58000864102e
Deploy Preview https://deploy-preview-3651--compassionate-pike-271cb3.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

netlify[bot] avatar Jun 17 '22 15:06 netlify[bot]

Hi @chrskrchr, 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

github-actions[bot] avatar Jun 19 '22 00:06 github-actions[bot]

@saihaj - any idea on the expected turnaround time to get a review from the reviewers group?

chrskrchr avatar Jun 23 '22 15:06 chrskrchr

@saihaj @n1ru4l - looks like we may have the approvals necessary to merge this, but I myself don't have right to merge. Are either of you able to merge it?

chrskrchr avatar Jul 03 '22 13:07 chrskrchr

@n1ru4l - apologies for continuing to pester you about this, but does your 👍 on my message mean that you'll take care of merging this? And if so, do you have an ETA on when the PR will be merged and cut into a new release? (and similar questions for the #3573 and #3576 backports of this fix to older versions)

chrskrchr avatar Jul 06 '22 15:07 chrskrchr

Hey @chrskrchr I would love to merge this one but I need to wait for another reviewer from @graphql/graphql-js-reviewers as per https://github.com/graphql/graphql-js/issues/3382

saihaj avatar Jul 06 '22 16:07 saihaj

I can provide an alpha release for this

saihaj avatar Jul 06 '22 16:07 saihaj

@github-actions publish-pr-on-npm

saihaj avatar Jul 06 '22 16:07 saihaj

@github-actions publish-pr-on-npm

@saihaj The latest changes of this PR are available on NPM as graphql@17.0.0-alpha.1.canary.pr.3651.57364d3f9da445b2bba520d3b886e07dc2af10e2 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-3651

github-actions[bot] avatar Jul 06 '22 16:07 github-actions[bot]

I think we need someone from the foundation to verify that the cla workaround here is viable

yaacovCR avatar Jul 07 '22 15:07 yaacovCR

I think we need someone from the foundation to verify that the cla workaround here is viable

@yaacovCR - is there anything I can do to help with that verification?

chrskrchr avatar Jul 13 '22 14:07 chrskrchr

I believe the person who can help us in terms of CLA issues would be @brianwarner but @graphql/tsc might know better.

I suppose we can summarize the issue as follows: the original code contributor for (x seemingly technical reason) cannot sign the CLA but has posted that agreement to a new PR with their changes to be submitted under an author that can sign the CLA.

yaacovCR avatar Jul 13 '22 16:07 yaacovCR

I've raised this with the TSC on Discord too, thanks for bringing it to our attention.

By the way, Brian is no longer working at the Linux Foundation so he's no longer the right person to ping.

benjie avatar Jul 14 '22 10:07 benjie

Thank you and thanks for the update.

yaacovCR avatar Jul 14 '22 10:07 yaacovCR

@benjie - curious if there are any updates on the convo you raised with the TSC?

chrskrchr avatar Jul 21 '22 12:07 chrskrchr

Sadly I've not heard anything. In my understanding though taking this approach isn't legally sound; I think your options are either to do a clean re-implementation of it (where you reimplement the fix without referencing the previous code) or to solve the underlying CLA issues. Let me know if I can do anything to help on the CLA front.

benjie avatar Jul 21 '22 14:07 benjie

I think your options are either to do a clean re-implementation of it (where you reimplement the fix without referencing the previous code)

OK - thanks, @benjie. I'll submit a new PR that makes no reference to the original code or PR.

chrskrchr avatar Aug 18 '22 16:08 chrskrchr