Skip to content

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

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

Closed
wants to merge 1 commit into from

Conversation

chrskrchr
Copy link
Contributor

@chrskrchr chrskrchr commented Jun 17, 2022

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.

@netlify
Copy link

netlify bot commented Jun 17, 2022

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit 5f30234
🔍 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.

@github-actions
Copy link

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

@saihaj saihaj requested a review from a team June 19, 2022 00:09
@chrskrchr
Copy link
Contributor Author

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

@chrskrchr
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@saihaj
Copy link
Member

saihaj commented Jul 6, 2022

Hey @chrskrchr I would love to merge this one but I need to wait for another reviewer from @graphql/graphql-js-reviewers as per #3382

@saihaj
Copy link
Member

saihaj commented Jul 6, 2022

I can provide an alpha release for this

@saihaj

This comment has been minimized.

@github-actions
Copy link

github-actions bot commented Jul 6, 2022

@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

@yaacovCR
Copy link
Contributor

yaacovCR commented Jul 7, 2022

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

@chrskrchr
Copy link
Contributor Author

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?

@yaacovCR
Copy link
Contributor

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.

@benjie
Copy link
Member

benjie commented Jul 14, 2022

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.

@yaacovCR
Copy link
Contributor

Thank you and thanks for the update.

@chrskrchr
Copy link
Contributor Author

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

@benjie
Copy link
Member

benjie commented Jul 21, 2022

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.

@chrskrchr
Copy link
Contributor Author

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 chrskrchr closed this Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants