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

CDP Runtime.evaluate does not drain async tasks #46966

Closed
EmielM opened this issue Oct 9, 2024 · 8 comments
Closed

CDP Runtime.evaluate does not drain async tasks #46966

EmielM opened this issue Oct 9, 2024 · 8 comments
Assignees
Labels
Bug Debugger Issues related to React Native DevTools or legacy JavaScript/Hermes debugging Never gets stale Prevent those issues and PRs from getting stale Resolution: Fixed A PR that fixes this issue has been merged.

Comments

@EmielM
Copy link
Contributor

EmielM commented Oct 9, 2024

Bug Description

I'm trying to run some automated testing scripts through the Hermes CDP interface using Runtime.evaluate.

First off, the awaitPromises flag does not seem to be supported at all. There are some work-arounds to this, so that's fine.

A bigger issue seems to be that after Runtime.evaluate, planned Promise tasks are not executed. What I see is usually after a second or 5-10, the queue is triggered and drained again. (I also tried to workaround this by running HermesInternal.drainJobs after every command, but this is unexposed without hermes-internal-test-methods flag see; I didn't manage to enable that flag in an otherwise vanilla RN project either.)

Tried to create a simple case to replicate, but the overall setup is pretty involving.

@EmielM EmielM added the Bug label Oct 9, 2024
@dannysu
Copy link
Contributor

dannysu commented Oct 10, 2024

hi @EmielM,

The current Hermes CDP implementation is scoped to enable the set of features needed by the new RN debugger coming in 0.76. We specifically only meant for it to support the exact version of that RN DevTools.

Unfortunately awaitPromises isn't supported yet. We do want to close the gap but don't have timeline for that.

There are some work-arounds to this

In terms of the workarounds you talked about, could you please elaborate what that is?

planned Promise tasks are not executed

What RN version are you using?

The draining of microtask queue may be the responsibility of the code on the RN side and not something Hermes the engine controls. What are these promise tasks you're talking about? How are you creating them and what's the code you're using that's noticing them resolving after 5-10 seconds.

Having code speaks most clearly. Thanks.

@EmielM
Copy link
Contributor Author

EmielM commented Oct 10, 2024

Apologies, I c/should have been a bit more detailed, indeed. Thanks for your patience.

I attach a chrome inspector via CDP, then evaluate (new Promise((resolve) => resolve('hey'))).then((x) => console.log(x));.

Here's a screenshot that illustrates the (timing) issue:
hermes-lagging-promise-queue

My hypothesis is that the task queue is simply not drained in the Hermes CDP client, and after a while some other message goes over the RN bridge or so and that triggers the pending queue to flush.

Logging the backend CDP interaction:

> {"id":552,"method":"Runtime.evaluate","params":{"expression":"(new Promise((resolve) =\u003e resolve('hey'))).then((x) =\u003e console.log(x
));","objectGroup":"console","includeCommandLineAPI":true,"silent":false,"returnByValue":false,"generatePreview":true,"userGesture":true,"awaitPromise":false,"replMode":true,"allowUnsafeEvalBlockedByCSP":false,"contextId":1}}
< {"id":552,"result":{"result":{"className":"Object","description":"Object","objectId":"453","preview":{"description":"Object","overflow":false,"properties":[{"name":"_h","type":"number","value":"0"},{"name":"_i","type":"number","value":"0"},{"name":"_j","subtype":"null","type":"object","value":"null"},{"name":"_k","subtype":"null","type":"object","value":"null"}],"type":"object"},"type":"object"}}}                                                                    

One little update: I did find out that HermesInternal.useEngineQueue() is false in my setup.

Will attempt to recreate in latest RN/Hermes, but that will take some time in my setup unfortunately.

@tmikov
Copy link

tmikov commented Oct 10, 2024

One little update: I did find out that HermesInternal.useEngineQueue() is false in my set

@EmielM Right. That is only enabled with the New Architecture. But either way, Hermes doesn't control the event loop and the draining of the queue. I think we should transfer this issue to React Native.

@dannysu
Copy link
Contributor

dannysu commented Oct 10, 2024

@EmielM please note that we also don't support just any Chrome DevTools, so if you're just opening up Chrome browser and using it then things are not 100% compatible. We only support the specific fork of DevTools we're calling React DevTools.

The promise queue draining behavior looks to be RN related, so I'll transfer this issue over.

@dannysu dannysu transferred this issue from facebook/hermes Oct 10, 2024
@react-native-bot react-native-bot added Needs: Author Feedback Needs: Repro This issue could be improved with a clear list of steps to reproduce the issue. labels Oct 10, 2024
@react-native-bot
Copy link
Collaborator

⚠️ Missing Reproducible Example
ℹ️ We could not detect a reproducible example in your issue report. Please provide either:
  • If your bug is UI related: a Snack
  • If your bug is build/update related: use our Reproducer Template. A reproducer needs to be in a GitHub repository under your username.

@react-native-bot
Copy link
Collaborator

⚠️ Missing Reproducible Example
ℹ️ We could not detect a reproducible example in your issue report. Please provide either:

@EmielM
Copy link
Contributor Author

EmielM commented Oct 10, 2024

@tmikov Alright (although that's a bit surprising to me as the CDPAgent implementation seems to live in the Hermes codebase)

@dannysu Fair, but this doesn't work (as expected) in the RN inspector either: as I understand it's a pretty straight forward fork from the devtools project, and also sends Runtime.evaluate to RN/hermes. Screenshot:
image

@react-native-bot Yes!

@github-actions github-actions bot added Needs: Attention Issues where the author has responded to feedback. and removed Needs: Author Feedback labels Oct 10, 2024
@cortinico cortinico added Debugger Issues related to React Native DevTools or legacy JavaScript/Hermes debugging and removed Needs: Attention Issues where the author has responded to feedback. labels Oct 11, 2024
@hoxyq
Copy link
Contributor

hoxyq commented Oct 11, 2024

Thanks for reporting this!

A bigger issue seems to be that after Runtime.evaluate, planned Promise tasks are not executed.

We are aware of this issue and planning to fix this on React Native side. I will share more details once we confirm the timeline for it.

@cortinico cortinico added Never gets stale Prevent those issues and PRs from getting stale and removed Needs: Repro This issue could be improved with a clear list of steps to reproduce the issue. labels Oct 11, 2024
hoxyq added a commit to hoxyq/react-native that referenced this issue Oct 18, 2024
…untimeScheduler

Summary:
# Changelog:
[General] [Fixed] - Microtasks are now correctly executed after the code evaluation in Console panel of DevTools.

Fixes facebook#46966.

`runtimeExecutor` which is propagated here, is actually being used by Hermes:
https://www.internalfb.com/code/fbsource/[cba75f2b515a]/xplat/js/react-native-github/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp?lines=112-113

The issue was that any expression that should be evaluated as part of `Runtime.evaluate` was not going through `RuntimeScheduler`, because specified `runtimeExecutor` was not going through it as well, and it was defined prior to `RuntimeScheduler`. Because of this, `RuntimeScheduler` was not draining out the microtasks queue and basically any scheduled Microtasks were not executed, see T200616136.

With this fix, we create an executor that goes through `RuntimeScheduler`, which is using another executor that makes sure that all scheduled callbacks are only executed after `Inspector` was setup.

It is extremely messy and in the future we should untangle these circular dependencies and try to simplify the approach.

Differential Revision: D64552372
hoxyq added a commit to hoxyq/react-native that referenced this issue Oct 22, 2024
…untimeScheduler (facebook#47119)

Summary:

# Changelog:
[General] [Fixed] - Microtasks are now correctly executed after the code evaluation in Console panel of DevTools.

Fixes facebook#46966.

`runtimeExecutor` which is propagated here, is actually being used by Hermes:
https://www.internalfb.com/code/fbsource/[cba75f2b515a]/xplat/js/react-native-github/packages/react-native/ReactCommon/react/runtime/ReactInstance.cpp?lines=112-113

The issue was that any expression that should be evaluated as part of `Runtime.evaluate` was not going through `RuntimeScheduler`, because specified `runtimeExecutor` was not going through it as well, and it was defined prior to `RuntimeScheduler`. Because of this, `RuntimeScheduler` was not draining out the microtasks queue and basically any scheduled Microtasks were not executed, see T200616136.

With this fix, we create an executor that goes through `RuntimeScheduler`, which is using another executor that makes sure that all scheduled callbacks are only executed after `Inspector` was setup.

It is extremely messy and in the future we should untangle these circular dependencies and try to simplify the approach.

Reviewed By: rubennorte

Differential Revision: D64552372
@cortinico cortinico added the Resolution: Fixed A PR that fixes this issue has been merged. label Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Debugger Issues related to React Native DevTools or legacy JavaScript/Hermes debugging Never gets stale Prevent those issues and PRs from getting stale Resolution: Fixed A PR that fixes this issue has been merged.
Projects
None yet
6 participants