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

fix: impossible to configure task if previous task has call stop(). (Android + Headless) #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ACHP
Copy link

@ACHP ACHP commented Sep 3, 2024

I noticed that in my app:
IF

  • I configure() a background-task
  • I run this task when my app is killed (using react-native headless)
  • I re-open my app and reconfigure a new task

THEN

  • The second background task is not properly configured and not run properly

Reproducing the bug

I created a simple react-native app with minimal code to reproduce the bug
https://github.com/ACHP/rn-background-fetch-bug-repro

Run the app using react-native (checkout + yarn + yarn start + yarn android)

  • Then click on the blue "Register background fetch" button. (This will register a background task)
  • Then you can kill your app and run the background task with adb shell cmd jobscheduler run -f com.rnbgfetchbugrepro 999
    • You should see a logger in the metro console indicating the task started, stopped, and finished.
  • Then re-open the app and re-click the button
  • Re-kill your app and re-run the headless task with adb shell cmd jobscheduler run -f com.rnbgfetchbugrepro 999
    • You should now see an error in the logcat console [BGTask] failed to load config for taskId: react-native-background-fetch

Video of this process

rnbgfetch-repro.mp4

What is happening

From my research, I noticed that the second time we register the background task, it is not properly saved into the shared-preferences (We can see it in the video).
This happens because on this line, the mConfig hashmap still contains the taskId when we relaunch the app the second time.

And the hashmap still contains the taskId because when we call stop(), it clean the shared-preferences, but not the mConfig hashmap.

if (taskId == null) {
synchronized (mConfig) {
for (BackgroundFetchConfig config : mConfig.values()) {
BGTask task = BGTask.getTask(config.getTaskId());
if (task != null) {
task.finish();
BGTask.removeTask(config.getTaskId());
}
BGTask.cancel(mContext, config.getTaskId(), config.getJobId());
config.destroy(mContext);
}
BGTask.clear();
}
} else {
BGTask task = BGTask.getTask(taskId);
if (task != null) {
task.finish();
BGTask.removeTask(task.getTaskId());
}
BackgroundFetchConfig config = getConfig(taskId);
if (config != null) {
config.destroy(mContext);
BGTask.cancel(mContext, config.getTaskId(), config.getJobId());
}
}

That is why we see a Re-configured existing task, instead of a fresh "configure".
Then when we try to run the background task, it loads the config from the shared-preference, but it can't find nothing. So it fails with [BGTask] failed to load config for taskId: react-native-background-fetch

Suggestion

We should probably clear the mConfig object when we stop the backgroundTask, so that there is no "desynchronisation" between the sharedPreference

This commit ensures tasks are properly removed from the configuration map when they are cancelled. Previously, some tasks were still lingering in the map, leading to potential inconsistencies. Now, the tasks are consistently
@christocracy
Copy link
Member

Have you actually tested this?

@ACHP
Copy link
Author

ACHP commented Sep 3, 2024

TBH I tested by hand only 2 scenarios:

  • When the task is stopped() from background => We should be able to call configure() again and expect it to work
  • When the task is stopped() from headless => We should be able to call configure() again and expect it to work

Here is a video where we see these 2 scenarios seems to works

bg-fetch.mp4

I imported the module via android studio and the implementation(group: 'com.transistorsoft', name:'tsbackgroundfetch', version: '+') by implementation project(":tsbackgroundfetch") in order to test.

I didn't test using the schedule instead of the configure()

@ACHP
Copy link
Author

ACHP commented Sep 30, 2024

@christocracy Have you had a chance to review this PR? It's not urgent since we're currently using a patch, but if there are any major issues with my fix or if I overlooked something that makes the pull request unnecessary, I'd appreciate your feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants