cloudant / sync-android

A JSON-based document datastore for Android applications
Apache License 2.0
267 stars 90 forks source link

PeriodicReplicationService doesn't replicate after a forced stop of the APP #568

Closed jjrodrig closed 6 years ago

jjrodrig commented 6 years ago

PROBLEM After the Android Application is stopped by the user using "force stop" the replication service is not started again on application restart. The problem is solved restarting the device.

The user can decide to stop the application when Android detects that the app is behaving slowly and proposes to wait or stop the application.

When the app is restarted by the user, the PeriodicReplicationService checks a flag stored in shared preferences. This flag is set to true, then the replication service is not restarted as the alarm is not scheduled.

POSSIBLE SOLUTION

Make a dual check


tomblench commented 6 years ago

@jjrodrig I tried to reproduce this with a test app and didn't get the behaviour you reported.

In my sample app I used a class extending PeriodicReplicationService and one extending PeriodicReplicationReceiver. I killed it with adb shell am force-stop com.cloudant.todo and then re-opened it and the replications resumed.

I did this on the emulator but the results should be the same on real hardware.

If you have a minimal app that you can share, as well as information about versions of Android OS, API, etc, then I will try to reproduce using that.

jjrodrig commented 6 years ago

Thanks @tomblench

We've a few users where we have noticed this issue. The only way we managed to reproduce the problem is by forcing the application stop. The effect is that the flag keeps set to true but the PendingIntent is not schedulled.

We have tested it in real HW, I'll try to reproduce the problem in emulator and I'll update the issue with the details.

jjrodrig commented 6 years ago

@tomblench I've been testing with the todo example and in an emulator.

Are you binding from the activity to the service in you sample?

I can provide you my todo sample in case you need it.

Thanks

tomblench commented 6 years ago

@jjrodrig please provide your example, it's easier for me to reproduce that way.

jjrodrig commented 6 years ago

In the TodoActivity, the methods onStart/onStop has the binding logic commented. In this situation the problem is reproduced.

todo-sync.zip

tomblench commented 6 years ago

OK I reproduced the issue with your app. One of the differences with my app and yours is the code which starts the periodic replication. It looks like your code starts it but doesn't bind it.

I changed startPeriodicReplication() to the following:

    private void startPeriodicReplication() {
        bindService(new Intent(this, CustomPeriodicReplicationService.class), mConnection, Context.BIND_AUTO_CREATE);
    }

And after un-installing the app, and re-deploying it, the replication schedule now appears to survive being force killed.

Can you try this out and report back to me? I think our sample code for these replication policies is a bit confusing and could be more clear.

tomblench commented 6 years ago

I just re-read your comment, I didn't realise the bind/unbind were commented out. If they are un-commented then the code should work correctly.

Note also that there should be no need to send the command to start the replication, but there is no harm in doing it.

jjrodrig commented 6 years ago

Thanks @tomblench

In our case we have decided not to bind the main activity to the service because we don't need any communication between them. As I understand it is required by the PeriodicReplicationService to be bound to an activity in order to be rescheduled after a forced stop. We have not interpreted this requirement from the current doc. We understood it as an optional requirement.

In any case, don't you find possitive that the alarm is rescheduled even if the service is not bound to any activity?

tomblench commented 6 years ago

Yes, I would agree with your statement that the binding is required or the service won't be rescheduled after a forced stop. We will look into fixing our documentation and samples to make this clearer.

The problem that you discovered is that if the service is not bound, the shared preferences flag can get out of sync with the state of the service (the flag indicates the service is running, when it has been stopped, so startPeriodicReplication logs "Attempted to start an already running alarm manager" and doesn't start).

I think a better long-term solution would be to get rid of the flag and just check directly whether the service is running, which removes any chance of these sorts of error conditions. But I'm not keen to introduce a larger change which might need more testing if you have a solution which works for now.

jjrodrig commented 6 years ago

In this case, we have a workaround that is working so it is not a major issue for us to keep it in this way. A more explicit doc for the service start process and the effect of not to bind it to the activity will be very helpful. This library is being very useful in our mobility projects. If you find we can contribute somehow, just contact me. Thanks

tomblench commented 6 years ago

OK that's great news. We are normally happy to take PRs if they fix bugs or introduce useful features.