-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Await onApiCallEnd
/onApiCallBegin
hooks
#30795
base: main
Are you sure you want to change the base?
Conversation
@@ -188,7 +189,7 @@ export abstract class ChannelOwner<T extends channels.Channel = channels.Channel | |||
logApiCall(logger, `=> ${apiName} started`, isInternal); | |||
const apiZone: ApiZone = { apiName, frames, isInternal, reported: false, csi, callCookie, wallTime }; | |||
const result = await zones.run('apiZone', apiZone, async () => await func(apiZone)); | |||
csi?.onApiCallEnd(callCookie); | |||
await csi?.onApiCallEnd(callCookie); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar issue happens here when this is not awaited - the test runs to completion, closes the browser and code in onApiCallEnd
can get rejected with "Test ended." error because the browser was already closes by the time it gets executed
This comment has been minimized.
This comment has been minimized.
How did you discover this? We only have one instrumentation instance, so things should run in order. Starting with an issue would always be great as well! |
We are hooking into this not-so-stable API in our |
Test results for "tests 1"6 fatal errors, not part of any test 1 flaky5 interrupted10147 passed, 244 skipped, 16897 did not run Merge workflow run. |
Looks like the bots are unhappy with your approach. |
Looks like this was addressed in #31054? |
It improves the situation by differentiating between sync and async handlers/listeners. It still doesn't allow decorating API calls in a way that would wait for completion. Is this something you'd consider? I'm not sure if it's even worth creating a feature request for this... since this whole API is just not quite public 😢 There is a chance that this might work for us since we merely inject |
I thought you tried it and the world broke: #30795 (comment). What would you like us to consider?
We routinely rely on the serialized nature of the posted tasks. So I would not hesitate, just make sure you have a good test coverage. |
onApiCallBegin
doesn't completely serve its purpose today.onApiCallBegin
hook here so whatever gets installed after it gets executed in the next microtask (since this hook is synchronous). That means that the next hooks get called after the API call has already been dispatched since the engine exits from the non-awaited instrumentation wrapper call and proceeds tothis._connection.sendMessageToServer