Await on graphql.execute() before calling executionDidEnd handler (AS-59)#2298
Conversation
benjamn
left a comment
There was a problem hiding this comment.
Makes sense!
It's usually not necessary to await a promise before returning it from an async function, because it will just get rolled into the final promise, but in this case the timing of the finally block is what matters.
abernix
left a comment
There was a problem hiding this comment.
Great find and LGTM.
As we discussed on our call: Were this execute used anywhere else besides the place where it's used, it might have been tempting to suggest wrapping the execution in a return-'d Promise.resolve with a .then(onFulfilled, onRejected) where fulfillment or rejection both called executionDidEnd() to preserve the async dynamics, but given the limited scope, this is great.
Also as discussed, a test which ensures that the duration amounts in the traces mathematically match up would be welcomed either in this PR or as a follow-up (since we could use more of those in general). Your call!
82f7138 to
8b9af1c
Compare
|
Thanks for the reviews! |
* test: ensure "total duration" > "duration of resolvers" Add test for #2298 * Update tracing duration test to be more comprehensive Previously we were only guaranteeing that individual resolvers didn't have a duration longer than the total duration. The updated test guarantees that the duration (from when the first resolver started to when the last resolver ended) should be less than total duration * Add another resolver to tracing test * Prefer `const` over `let` for variables which aren't mutated.
|
Glad you guys found and fixed this so quickly! I was scratching my head for hours on this before I contacted about the issue. Eagerly awaiting the new version for my own testing!! |
|
@dragonfriend0013 This has been released in |
extensionStack's executionDidEnd handler (the callback function returned fromexecutionDidStart) was being called beforegraphql.executefinishes runningThis PR adds an
awaitbefore the return so code infinallyruns aftergraphql.executefinishesTODO: