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

HeadlessJsTaskService is expected to run on UI thread #15915

Closed
oriharel opened this issue Sep 12, 2017 · 21 comments
Closed

HeadlessJsTaskService is expected to run on UI thread #15915

oriharel opened this issue Sep 12, 2017 · 21 comments
Labels
Resolution: Locked This issue was locked by the bot.

Comments

@oriharel
Copy link
Contributor

oriharel commented Sep 12, 2017

Is this a bug report?

Yes

Have you read the Contributing Guidelines?

Yes

Environment

  1. react-native -v: react-native-cli: 2.0.1 react-native: 0.48.1
  2. node -v: v7.5.0
  3. npm -v: 5.3.0
  • Target Platform: Android
  • Development Operating System: macOS
  • Build tools:

Steps to Reproduce

  1. Received a push notification
  2. Starting a Service that extends HeadlessJsTaskService.java
  3. App throws exception com.facebook.react.bridge.AssertionException: Expected to run on UI thread!

Expected Behavior

I expect no UI thread requirement for a background service (same as Android native SDK)

Actual Behavior

App throws an exception

@chirag04
Copy link
Contributor

starting a headless task is expected to run on UI thread:

/**
* Start a task. This method handles starting a new React instance if required.
*
* Has to be called on the UI thread.
*
* @param taskConfig describes what task to start and the parameters to pass to it
*/
protected void startTask(final HeadlessJsTaskConfig taskConfig) {

@oriharel
Copy link
Contributor Author

@chirag04 thanks for the note. However, as I understand, one of the common uses of this is performing tasks triggered by a push notification (or a job schedule). These are clearly not required to run in a UI thread as these are background activities.

@cmdshepard
Copy link

cmdshepard commented Sep 12, 2017

@oriharel according to the documentation, HeadlessJS is meant for tasks that are needed to be ran when your app in the background. In which case there are no operations on the main thread.

By default, your app will crash if you try to run a task while the app is in the foreground. This is to prevent developers from shooting themselves in the foot by doing a lot of work in a task and slowing the UI. You can pass a fourth boolean argument to control this behaviour.

@oriharel
Copy link
Contributor Author

Exactly. This is why I don't understand the "startTask must be triggered from the UI thread" requirement. Unless there is some limitation forcing the React context to be spawned from the UI thread.

@D1no
Copy link

D1no commented Sep 13, 2017

That means you can not trigger a headless task from another headless task (threads of threads).

@oriharel
Copy link
Contributor Author

In the documentation, it is clearly stated -

Now, whenever you start your service, e.g. as a periodic task or in response to some system event / broadcast, JS will spin up, run your task, then spin down.

Which means, even if a BroadcastReceiver is triggered, say from a push notification, the Headless task should be able to perform.

But this is not the case, because there is an underline requirement that the onReactContextInitialized event handler will be called from the main thread (AKA UI thread). A requirement that most of the time not fulfilled (for example, when a user swiped out the app from the recent apps menu).

So in fact - I DO consider this a bug and updated the form.

@haimyyy
Copy link

haimyyy commented Sep 13, 2017

+1

2 similar comments
@ycarmel
Copy link

ycarmel commented Sep 13, 2017

+1

@blankg
Copy link

blankg commented Sep 13, 2017

+1

@foghina
Copy link
Contributor

foghina commented Sep 13, 2017

I think there's a bit of confusion here. The "UI" thread is simply the "main" thread of an Android application. It's the thread that Services naturally run on. For various reasons, the React context is always initialized on this thread, from where it spins off two others: JS and modules (IIRC). This is why startTask needs to be run on the UI/main thread, because it might need to initialize the context.

@foghina
Copy link
Contributor

foghina commented Sep 13, 2017

If you're on a background thread it should be fairly easy to post something to the main one and call startTask there. Is there a reason why that wouldn't work?

@oriharel
Copy link
Contributor Author

You're right, everything I said about the UI thread actually reffers to the main thread. The Service "onStartCommand" does indeed happen on that thread. However, inside startTask there is an asynchronic call to create the React context. The callback that is triggered asynchronically is no longer on that thread. And that is already a private area so we can't override this in our inheriting class.

@oriharel
Copy link
Contributor Author

An updated pull request here.

facebook-github-bot pushed a commit that referenced this issue Sep 18, 2017
Summary:
<!--
Thank you for sending the PR! We appreciate you spending the time to work on these changes.

Help us understand your motivation by explaining why you decided to make this change.

You can learn more about contributing to React Native here: http://facebook.github.io/react-native/docs/contributing.html

Happy contributing!

-->

Headless tasks are required to run in the main thread, however due to the nature of the React context creation flow, the handler may be returned outside of the main thread, causing the HeadlessJsTaskContext to throw an exception.

Swipe out the app. send push notification from a server that starts a HeadlessJsTaskService
Closes #15940

Differential Revision: D5852448

Pulled By: foghina

fbshipit-source-id: 54c58a1eb7434dd5de5c39c28f6e068ed15ead9d
@narodejesus
Copy link

+1

@oriharel
Copy link
Contributor Author

Issue is now fixed and merged to React Native.

@sparsh
Copy link

sparsh commented Sep 28, 2017

I have used latest version i.e 0.48.4 of react-native, still, I don't see your changes in File HeadlessJsTaskService.java.
screen shot 2017-09-28 at 6 32 41 am

@oriharel
Copy link
Contributor Author

I don't think it's released yet

@artemukolov
Copy link

Hi. I tried to fix this file manually, but i got another error.
screen shot 2017-10-26 at 15 39 49

My code

BackgroundJob.register({
        jobKey: exactJobKey,
        job: () => process()
    });

    BackgroundJob.schedule({
        jobKey: exactJobKey,
        period: 900000,
        override: true,
        allowExecutionInForeground: true,
        persist: true
    });

@oriharel
Copy link
Contributor Author

Check your AppRegistry in js code. You need to register a js file before you call it from Java

@oriharel
Copy link
Contributor Author

Also check your HeadlessJsTaskService implementation in Java. Make sure you return new HeadlessJsTaskConfig with a name "exactJobKey".

@artemukolov
Copy link

Thanks. Looks like this issue is fixed in 0.49.5.

@facebook facebook locked as resolved and limited conversation to collaborators Sep 20, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Sep 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests