Allow headless JS tasks to retry

[JoshuaJamesOng]

Summary

setTimeout inside a headless JS task does not always works; the function does not get invoked until the user starts an Activity.

This was attempted to be used in the context of widgets. When the widget update or user interaction causes the process and React context to be created, the headless JS task may run before other app-specific JS initialisation logic has completed. If it's not possible to change the behaviour of the pre-requisites to be synchronous, then the headless JS task blocks such asynchronous JS work that it may depend on. A primitive solution is the use of setTimeout in order to wait for the pre-conditions to be met before continuing with the rest of the headless JS task. But as the function passed to setTimeout is not always called, the task will not run to completion.

This PR solves this scenario by allowing the task to be retried again with a delay. If the task returns a promise that resolves to a {'timeout': number} object, AppRegistry.js will not notify that the task has finished as per master, instead it will tell HeadlessJsContext to startTask again (cleaning up any posted Runnables beforehand) via a Handler within the HeadlessJsContext.

Documentation also updated here: facebook/react-native-website#771

AppRegistry.js

If the task provider does not return any data, or if the data it returns does not contain timeout as a number, then it behaves as master; notifies that the task has finished. If the response does contain {timeout: number}, then it will attempt to queue a retry. If that fails, then it will behaves as if the task provider returned no response i.e. behaves as master again. If the retry was successfully queued, then there is nothing to do as we do not want the Service to stop itself.

HeadlessJsTaskSupportModule.java

Similar to notify start/finished, we simply check if the context is running, and if so, pass the request onto HeadlessJsTaskContext. The only difference here is that we return a Promise, so that AppRegistry, as above, knows whether the enqueuing failed and thus needs to perform the usual task clean-up.

HeadlessJsTaskContext.java

Before retrying, we need to clean-up any timeout Runnable's posted for the first attempt. Then we need to copy the task config so that if this retry (second attempt) also fails, then on the third attempt (second retry) we do not run into a consumed exception. This is also why in startTask we copy the config before putting it in the Map, so that the initial attempt does leave the config's in the map as consumed. Then we post a Runnable to call startTask on the main thread's Handler. We use the same taskId because the Service is keeping track of active task IDs in order to calculate whether it needs to stopSelf. This negates the need to inform the Service of a new task id and us having to remove the old one.

Changelog

[Android][added] - Allow headless JS tasks to return a promise that will cause the task to be retried again with the specified delay

Test Plan

A fork containing this fix has been integrated and tested in a project like below:

export const headlessJsTask = () => () =>
    async function updateWidget() {
        const isReady = ...;
        if (!isReady) {
            return new Promise(resolve => resolve({'timeout': 1000})); // 1000ms
        }

        // Do work that relies on pre-requisite
    };

[analysis-bot]

Code analysis results:

• eslint found some issues.

[analysis-bot]

Code analysis results:

• eslint found some issues.

[analysis-bot]

Code analysis results:

• eslint found some issues.

[matthargett]

Can you add a new test to WritableNativeMapTest for the copy() method? I'd ask for tests to cover the other areas, but there doesn't appear to be any existing tests for those classes to build off of. So, bonus point if you can add a test one or two of those currently uncovered classes as well.

[cpojer]

@JoshuaJamesOng did you see @matthargett's comment and could you address it?

[cpojer]

@JoshuaJamesOng ping?

[ejanzer]

@JoshuaJamesOng I think this makes sense, the only part that feels a little weird to me is returning the timeout from the JS task. What if we allow you to configure whether or not retries are enabled (along with the timeout in ms, # of retries, etc.) from the native side (in HeadlessJsTaskConfig) and then we just retry on any JS exception if so? Maybe something like:

export const headlessJsTask = () => () =>
    async function updateWidget() {
        const isReady = ...;
        if (!isReady) {
          throw new Error('Not ready');
        }
       // other stuff if it's ready
    };

and then something like this in getTaskConfig (not sure if this is an up-to-date example, pulled it from the docs):

  new HeadlessJsTaskConfig(
    "SomeTaskName",
    Arguments.fromBundle(extras),
    5000, // timeout
    false, // allowed in foreground
    5, // number of retries
    1000 // retry after ms
  );

What do you think? Would that do what you need?.

[cpojer]

@JoshuaJamesOng hey! Did you see @ejanzer's reply to your PR? Would you be able to make the changes she proposed so we can merge this?

[analysis-bot]

Code analysis results:

• eslint found some issues. Run yarn lint --fix to automatically fix problems.

[JoshuaJamesOng]

Have made the changes suggested. Note however that this does change the behaviour. Previously the author of the could decide when to retry. The new implementation retries on all errors. I wonder if we should make it so that the user still has control over which errors are retried? I'm worried we get stuck in a loop. Thoughts?

[cpojer]

@JoshuaJamesOng I think that's a good idea. Could you add it?

[analysis-bot]

Code analysis results:

• eslint found some issues. Run yarn lint --fix to automatically fix problems.

[ejanzer]

Looks good to me, thanks for doing this!

[ejanzer]

Can you change the file name to HeadlessJsTaskError to match the class? Also, instead of putting it at the top level, can you put it /Utilities, maybe?

[ejanzer]

Do we reuse the taskId just so JS can continue to use the same id? Or to avoid adding entries to the active tasks map? (I think it makes sense to use the same taskId, just curious if there's another reason why it's needed)

[cpojer]

Hey @JoshuaJamesOng! Do you mind addressing @ejanzer's last small comments? Then I think we can land it finally :)

[cpojer]

I took care of the nits in order to take this over the finish line.

[facebook-github-bot]

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[react-native-bot]

This pull request was successfully merged by @JoshuaJamesOng in ac7ec46.

^{When will my fix make it into a release? | Upcoming Releases}