Skip to content
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

Fix crash in node when mixing sync/async resolvers #3529

Closed

Conversation

asztal
Copy link

@asztal asztal commented Apr 5, 2022

Fixes #3528

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.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 5, 2022

CLA Missing ID CLA Not Signed

@netlify
Copy link

netlify bot commented Apr 5, 2022

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit 5bfb9d6
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/625f19104a83690008203b68
😎 Deploy Preview https://deploy-preview-3529--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

github-actions bot commented Apr 7, 2022

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

@asztal asztal force-pushed the bug/unhandled-rejection-mixed-sync-async branch from b391c2f to 17ce540 Compare April 14, 2022 22:38
@asztal asztal force-pushed the bug/unhandled-rejection-mixed-sync-async branch from 17ce540 to bc7049b Compare April 19, 2022 13:58
@saihaj saihaj requested a review from yaacovCR April 19, 2022 14:10
@saihaj
Copy link
Member

saihaj commented Apr 19, 2022

Hey @asztal can you please sign the CLA

Copy link
Contributor

@yaacovCR yaacovCR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need similar logic when dealing with completion of lists?

@IvanGoncharov IvanGoncharov force-pushed the bug/unhandled-rejection-mixed-sync-async branch from 7f14b8e to 5bfb9d6 Compare April 19, 2022 20:18
@IvanGoncharov
Copy link
Member

@asztal I fixed everything except CLA.
Please sign it and this PR is ready to merge

@asztal
Copy link
Author

asztal commented Apr 19, 2022

Thanks! My contribution counts as a corporate contribution so I need a director to approve it for me first - it should be soon hopefully 🤞

@IvanGoncharov
Copy link
Member

Hi @asztal 👋
Any news on CLA?

@chrskrchr
Copy link
Contributor

@IvanGoncharov et al, once this fix lands in main, could we backport it to the 15.x branch? Happy to submit that PR if there aren't any objections and if @asztal doesn't want to do it themselves.

@chrskrchr
Copy link
Contributor

@asztal - any word on the CLA?

@IvanGoncharov
Copy link
Member

@IvanGoncharov et al, once this fix lands in main, could we backport it to the 15.x branch? Happy to submit that PR if there aren't any objections and if @asztal doesn't want to do it themselves.

Sounds great 👍
This is the exact idea behind the current dev flow.

@asztal
Copy link
Author

asztal commented May 11, 2022

@asztal - any word on the CLA?

The CLA manager at my company is having trouble using the Corporate CLA console -- I'm not at all familiar with it, so I've asked them to check they've signed up using the correct email address, otherwise just raise a support ticket about it like the message says. Sorry for the delay.

Here's the actual error in case anyone here knows anything about it.
image

@chrskrchr
Copy link
Contributor

Submitted a PR to backport this fix to 15.x: #3573

@asztal - feel free to submit a similar PR if you like and I can close mine. Just wanted to get the ball rolling on getting this fix in 15.x which is where my team needs it.

chrskrchr added a commit to chrskrchr/graphql-js that referenced this pull request May 11, 2022
@IvanGoncharov IvanGoncharov added the PR: bug fix 🐞 requires increase of "patch" version number label May 12, 2022
@IvanGoncharov
Copy link
Member

IvanGoncharov commented May 22, 2022

@asztal Any news on CLA on your side?
If you still experiencing technical issues with the CLA console I can connect to people at the Linux foundation that can hopefully solve it.

@chrskrchr
Copy link
Contributor

@asztal - any word on the CLA?

@hobby203
Copy link

hobby203 commented Jun 9, 2022

Hi @chrskrchr

Sorry about the delays, @asztal has since left and this one's fallen through the cracks a little. We're still chasing CLA approval, it's still stuck with the CLA approval but currently trying to find out if a ticket has been submitted

@hobby203
Copy link

Hi all,

CLA has now been signed, so hopefully we can make progress on getting this merged.

Screenshot 2022-06-13 at 15 49 34

@saihaj saihaj requested a review from yaacovCR June 14, 2022 02:31
@saihaj
Copy link
Member

saihaj commented Jun 14, 2022

@hobby203 can you try to rebase this and force push so the bot is happy?

@hobby203
Copy link

Hi @saihaj,

I'm not sure if I can. This pr has been created from a fork under the developer's personal account, I don't think I'm able to do that. I could fork his fork and create a new PR, would that affect CLA approval? If it would I can try get in contact with @asztal and ask him to help out but would prefer not to bother him if possible.

Best,
Ed

@asztal
Copy link
Author

asztal commented Jun 14, 2022

Hopefully I'll have time to rebase the branch tonight. Thanks for getting the CLA sorted @hobby203 🙏

@hobby203
Copy link

Hi all,

Looks as though we're having issues with the CLA still. The EasyCLA tool won't approve it as @asztal doesn't have any commits on this project, but we know this to be the case as the fix is coming from their own fork and not from within the project. Is this something that's happened before? Not sure how to proceed.

@saihaj
Copy link
Member

saihaj commented Jun 17, 2022

@hobby203 do you want to create PR with these changes and see if your CLA passes?

@hobby203
Copy link

@saihaj from inside this repo? or from my own fork, I checked but I can't create a new pr using these exact changes but i can try recreating the fix.

@chrskrchr
Copy link
Contributor

@hobby203 - I'm happy to submit a new PR if you're tired of messing with this. I've got backports of this PR to the 15.x branch (#3573) and 16.x (#3576) branches that are passing from a test and CLA perspective - we were just waiting on merging those until this PR was approved and merged.

Just let me know...

@hobby203
Copy link

@chrskrchr - if everyone is happy with that I think it will be the best course of action. I'm not sure why we're having these problems but we are struggling to make progress on our end with this issue, so if it can be moved forward by taking another approach i'm more than happy to let that happen (:

@chrskrchr
Copy link
Contributor

if everyone is happy with that I think it will be the best course of action.

No problem! I'll go ahead and submit that new PR (giving credit to @asztal for the actual fix) and let the maintainers decide if they're OK with moving forward that way.

@chrskrchr
Copy link
Contributor

A clone of this PR has been submitted as #3651.

@saihaj @IvanGoncharov - can you please look at @hobby203's comment above that gives their blessing for this PR to be superseded by another PR to in order to move past the CLA issues?

@saihaj
Copy link
Member

saihaj commented Jun 19, 2022

Sadly I can't be of much help with CLA bot 🥲 if the original author is fine having the PR from someone else I am fine moving forward with it.

@asztal
Copy link
Author

asztal commented Jun 21, 2022

Sadly I can't be of much help with CLA bot 🥲 if the original author is fine having the PR from someone else I am fine moving forward with it.

I'm fine with that, as long as the bug is fixed I'm happy. Apologies for the delays on my end.

@benjie
Copy link
Member

benjie commented Jul 21, 2022

@asztal The CLA error

"The commit (bc7049b). This user is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve.For further assistance with EasyCLA, please submit a support request ticket."

normally indicates that there's an issue with your authorship details for the given commit. Please ensure that that commit is authored by the exact same email address that you have approved via the CLA. Here's some instructions on how to change the email address on that commit:

https://stackoverflow.com/a/3042512

chrskrchr added a commit to chrskrchr/graphql-js that referenced this pull request Aug 18, 2022
@IvanGoncharov
Copy link
Member

Fixed by #3706

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: bug fix 🐞 requires increase of "patch" version number
Projects
None yet
8 participants