-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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: Android headless JS timeout #33044
fix: Android headless JS timeout #33044
Conversation
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.
This looks good to me, from my experience as a java / android native dev but - importantly - without a lot of experience in the headless JS area of react-native. Nice one @marcesengel
Thank you very much for the review and the kind words @mikehardy! Do you have any insights on how to get this merged? I currently have 3 open PRs going with the oldest being from over 3 weeks ago and no feedback yet 😕 |
No special tricks other than making some noise, metaphorically. You can use git commit history to find last committer (or recent frequent committer) for the file and tag them. I can try to get attention from the release crew. Can you add a link to your other 2 PRs here? |
Thank you very much, will do. The PRs in question are #32889/#33045 (the same changes, the original one has a comment and points to Thanks again for your help, much appreciated 👍 |
@dmitryrykun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Would love to see this get fixed. |
This pull request was successfully merged by @marcesengel in dac56ce. When will my fix make it into a release? | Upcoming Releases |
Summary: Fixes facebook#33043 and thereby invertase/react-native-firebase#3955. The issue arised because when there currently is no available React context, `HeadlessJsTaskService` will create a new one in background and start the task using `onReactContextInitialized` of `ReactInstanceManager.addReactInstanceEventListener`. https://github.com/facebook/react-native/blob/7ef14af81f3a1532ca1a703da666ea2e5a70a265/ReactAndroid/src/main/java/com/facebook/react/HeadlessJsTaskService.java#L94-L113 The `TimingModule` however is initialized asynchronously, meaning the headless JS is started before its initialization. That's an issue because the `TimingModule` is only run when there is JS code executing (meaning if the application is running or there is a headless task running) - this is checked by registering a `HeadlessJsTaskEventListener` on the `HeadlessJsTaskContext` in `TimingModule.initialize()`. https://github.com/facebook/react-native/blob/7ef14af81f3a1532ca1a703da666ea2e5a70a265/ReactAndroid/src/main/java/com/facebook/react/modules/core/TimingModule.java#L69-L75 However this event listener is never invoked because the task was started before `TimingModule.initialize()` is called -> `TimingModule.onHeadlessJsTaskStart(...)` is not called and the timer never resumes. In order to fix this we can just invoke `HeadlessJsTaskEventListener.onHeadlessJsTaskStart(...)` for all currently running tasks when a new listener is added to `HeadlessJsTaskContext`. This call then needs to be `synchronized` as otherwise there's a race condition with `HeadlessJsTaskContext.finishTask(...)` where `onHeadlessJsTaskFinish(...)` could be called before `onHeadlessJsTaskStart(...)`. See the diff for the exact changes. ## Changelog <!-- Help reviewers and the release process by writing your own changelog entry. For an example, see: https://github.com/facebook/react-native/wiki/Changelog --> [Android] [Fix] - Fixed `TimingModule` related functions for headless JS tasks, eg. `setTimeout` Pull Request resolved: facebook#33044 Test Plan: I did a local build with the changes and tested the provided example code from facebook#33043 there. Reviewed By: sshic Differential Revision: D34006573 Pulled By: dmitryrykun fbshipit-source-id: d6a821bbd6476ba278c1d8895edb4a0ba16d889e
Summary
Fixes #33043 and thereby invertase/react-native-firebase#3955.
The issue arised because when there currently is no available React context,
HeadlessJsTaskService
will create a new one in background and start the task usingonReactContextInitialized
ofReactInstanceManager.addReactInstanceEventListener
.react-native/ReactAndroid/src/main/java/com/facebook/react/HeadlessJsTaskService.java
Lines 94 to 113 in 7ef14af
The
TimingModule
however is initialized asynchronously, meaning the headless JS is started before its initialization. That's an issue because theTimingModule
is only run when there is JS code executing (meaning if the application is running or there is a headless task running) - this is checked by registering aHeadlessJsTaskEventListener
on theHeadlessJsTaskContext
inTimingModule.initialize()
.react-native/ReactAndroid/src/main/java/com/facebook/react/modules/core/TimingModule.java
Lines 69 to 75 in 7ef14af
However this event listener is never invoked because the task was started before
TimingModule.initialize()
is called ->TimingModule.onHeadlessJsTaskStart(...)
is not called and the timer never resumes.In order to fix this we can just invoke
HeadlessJsTaskEventListener.onHeadlessJsTaskStart(...)
for all currently running tasks when a new listener is added toHeadlessJsTaskContext
. This call then needs to besynchronized
as otherwise there's a race condition withHeadlessJsTaskContext.finishTask(...)
whereonHeadlessJsTaskFinish(...)
could be called beforeonHeadlessJsTaskStart(...)
. See the diff for the exact changes.Changelog
[Android] [Fix] - Fixed
TimingModule
related functions for headless JS tasks, eg.setTimeout
Test Plan
I did a local build with the changes and tested the provided example code from #33043 there.