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

willResolveField end hook not guaranteed to be called before end of execution #5372

Open
glasser opened this issue Jun 25, 2021 · 2 comments
Assignees

Comments

@glasser
Copy link
Member

glasser commented Jun 25, 2021

willResolveField end hooks (which at least are sync) are called from this special whenResultIsFinished on the result of a resolver. But that's in parallel with the processing done on that promise by GraphQL execution. So it's possible that it will be called later than the end of execution.

What this rolls up into is that endTime might end up missing from some nodes at the time the trace is either pre-encoded or converted to stats, so they will be missing an endTime on the trace and will not be counted for TypeStats if converted in the usage reporting plugin.

@glasser glasser added this to the MM-2021-07 milestone Jul 22, 2021
@glasser
Copy link
Member Author

glasser commented Jul 23, 2021

This was caught by our internal inconsistent_reports_records_in_gcs monitor in staging, which we should re-enable once this is fixed.

@hwillson hwillson removed this from the MM-2021-07 milestone Jul 29, 2021
@glasser glasser added 2021-08 and removed 2021-07 labels Aug 3, 2021
@hwillson hwillson modified the milestone: 3.x Maintenance Aug 24, 2021
@glasser glasser self-assigned this Sep 1, 2021
@glasser glasser added 2021-09 and removed 2021-08 labels Sep 1, 2021
@glasser glasser added 2021-10 and removed 2021-09 labels Oct 1, 2021
@hwillson hwillson added 2021-11 and removed 2021-10 labels Nov 1, 2021
@glasser glasser added 2021-12 and removed 2021-11 labels Dec 10, 2021
@glasser glasser removed the 2021-12 label Oct 11, 2022
@glasser
Copy link
Member Author

glasser commented Oct 21, 2022

Ideally we would fix this by adding a less hacky way to instrument execution to graphql-js.

#4667 is sorta similar/related.

glasser added a commit that referenced this issue Mar 5, 2024
Paired with @bbeesley.

Updates whenResultIsFinished to call callback also on Promise of Array
of Promises.
Fixes (partially)
#5372 and
#4667.

The test case we're fixing is `'passes result of Promise of Array of
Promises to the callback'`, the other tests test existing behaviour.

The existing bug prevented us from dynamically setting cache hints in
the reference resolver depending on the result of an asynchronous
operation. We discovered the bug within an integration test. If you
@glasser have an idea how to add an integration test and would like to
add it, that would be amazing!

---------

Co-authored-by: David Glasser <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants