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

app user obliged to force sync ? #686

Closed
PatGendre opened this issue Nov 23, 2021 · 16 comments
Closed

app user obliged to force sync ? #686

PatGendre opened this issue Nov 23, 2021 · 16 comments

Comments

@PatGendre
Copy link
Contributor

hi @shankari , our project manager la Rochelle Romain is also a user of course, he is obliged to 'force sync' regularly on the app because otherwise the trips stay as draft so supposedly cannot make it to the server and get processed by the pipeline.

I couldn't find any error in the server logs and in his recent app log he sent me (the db file is 5MB so I cannot attach it), I don't see any communication error either, except maybe this "checkSelfPermission returned 0" ? (the other permission checks seem to return true, although I did not check systemically).

Where should I look for, otherwise ?
Thanks

@shankari
Copy link
Contributor

@PatGendre this is probably because the background operations are not running properly.

on android, the periodic hourly process is the one that pushes the data to the server. You want to search for

START PERIODIC ACTIVITY
...
END PERIODIC ACTIVITY

in the phone logs. If you don't see them multiple times a day, this is the problem.

The workaround is to incorporate e-mission/e-mission-phone#797 while waiting for a principled fix from @ericafenyo

@PatGendre
Copy link
Contributor Author

Hi @shankari
Thanks a lot, this is the case for Romain : there is no "PERIODIC ACTIVITY" message whatsoever in the app logs !
I've seen your 797 PR a few weeks ago, it seems a good workaround to at least force sync at app launch.

As far as I understand this PR cannot be merged (it has conflicts) but you expect @ericafenyo to write a similar PR which would force sync on trip end, which could fix the issue (as a workaround to some phones blocking the correct background operations). This would be great, we'll pull this fix in our tracemob app when it is available :-)

@shankari
Copy link
Contributor

@PatGendre the conflict is in recording the stat after the force sync; if you just change autoForceSync to the following and ignore the ClientStats changes, it should be mergeable. I am on holiday this week due to the American Thanksgiving but can also resolve the merge conflict next week.

  let autoForceSync = function() {
    if ($ionicPlatform.is("android")) {
      console.log("app has started/resumed, forcing sync on android");
      ControlSyncHelper.forceSync();
    }
  };

But this assumes that the user does launch the app periodically.

A more principled fix is to push the data at the end of the trip so the pushes can happen completely in the background. And @ericafenyo has graciously agreed to contribute that change!

@PatGendre
Copy link
Contributor Author

@shankari thanks again, no hurry, enjoy Thanksgiving !!
We'll see later if @ericafenyo can do the trip end force sync hack.

@ericafenyo
Copy link
Contributor

Yea, I'm currently on vacation. I will start working on it after I am back.

@ericafenyo
Copy link
Contributor

ericafenyo commented Dec 15, 2021

@shankari , @PatGendre
I am about to start the functionality

@ericafenyo
Copy link
Contributor

I will illustrate what I did on our end. I am also open to any ideas for improvement.

After the handleTripEnd() in the TripDiaryStateMachineService.java I sent a broadcast and then triggered the force sync in the receiver.

The broadcast:

// TripDiaryStateMachineService.java#handleTripEnd()


// Force sync
 ctxt.sendBroadcast(new ExplicitIntent(ctxt, getString(R.string.tracker_event_trip_ended)));

The receiver:

// SynchronizeDataReceiver.kt (BroadcastReceiver)

...

  override fun onReceive(context: Context, intent: Intent?) {
    Timber.d("onReceive(context: $context, intent: $intent)")

    externalScope.launch(dispatcher) { syncData() }
  }

  private fun syncData() {
    try {
      dataSyncManager.forceSync(authenticatedUser.getIdToken())
    } catch (exception: Exception) {
      Timber.e(exception, "Error occurred while syncing data with server")
    }
  }

...

The dataSyncManager:

// DataSyncManager.kt (impl)

...

 override fun forceSync(token: String) {
    val bundle = Bundle().also { it.putString(EXTRA_USER_TOKEN_KEY, token) }
    ServerSyncAdapter(context, true).onPerformSync(account, bundle, AUTHORITY, null, null)
  }

Since we have a custom jwt_auth implementation, we injected the token instead of using this

@ericafenyo
Copy link
Contributor

Another thing. For us to properly monitor the data sync, we normally show a notification during the sync process and have a "last sync time" info on the settings page.

photo_2021-12-15 14 42 20

@ericafenyo
Copy link
Contributor

ericafenyo commented Dec 22, 2021

@shankari, @PatGendre
I am not able to run the e-mission phone app.
It looks like I am having something similar to this issue katzer/cordova-plugin-local-notifications#1963

android.support.v4.app.* not found
All the imports related to Notifications are affected

@shankari
Copy link
Contributor

shankari commented Dec 22, 2021

@ericafenyo the CI seems to be working, including the scheduled runs on master.
https://github.com/e-mission/e-mission-phone/actions?query=workflow%3Aosx-build-android

Are you using the current setup requirements, including Java 1.8?
Please check the versions in the CI, match them in your build and everything should work!
https://github.com/e-mission/e-mission-phone/runs/4572751180?check_suite_focus=true

@shankari
Copy link
Contributor

@ericafenyo in terms of the overall design, this seems fine. I have generally been handling the state machine transitions in src/android/location/TripDiaryStateMachineService.java. I can understand your reluctance to modify existing code, and in general, decoupling behavior is a good idea. I do want to make sure, however, that the operation will finish.

The general rule of thumb is that receivers are not supposed to perform intensive tasks, but are supposed to delegate to a service instead. (https://developer.android.com/guide/components/broadcasts).

Because a receiver's onReceive(Context, Intent) method runs on the main thread, it should execute and return quickly. If you need to perform long running work, be careful about spawning threads or starting background services because the system can kill the entire process after onReceive() returns. For more information, see Effect on process state To perform long running work, we recommend:

I think that is what you are doing with

externalScope.launch(dispatcher) { syncData() }

Can you tell me a little more about what externalScope is and how it addresses the limitations of the broadcast receiver?

@shankari
Copy link
Contributor

shankari commented Dec 22, 2021

android.support.v4.app.* not found
All the imports related to Notifications are affected

This certainly looks like having to incorporate androidx support. I believe android10 now requires androidx support.
https://cordova.apache.org/announcements/2021/07/20/cordova-android-10.0.0.html

But that was enabled back in 2020 (e-mission/e-mission-phone@5a832ca) for the phone repo...

@ericafenyo
Copy link
Contributor

@shankari

Are you using the current setup requirements, including Java 1.8?

Yea, I am on 1.8. I will take a closer look at the CI and try again.

Can you tell me a little more about what externalScope is and how it addresses the limitations of the broadcast receiver?

The externalScope.launch(dispatcher) { } moves the execution of the syncData() method to a background thread using Kotlin Coroutines but does not addresses the limitations

I will not use Coroutines in the plugins and can move the syncData() to a Service if it is considered a long running task.

This certainly looks like having to incorporate androidx support. I believe android10 now requires androidx support.
https://cordova.apache.org/announcements/2021/07/20/cordova-android-10.0.0.html

But that was enabled back in 2020 (e-mission/e-mission-phone@5a832ca) for the phone repo...

Okay

@shankari
Copy link
Contributor

@ericafenyo

I will not use Coroutines in the plugins and can move the syncData() to a Service if it is considered a long running task.

I think that it could be a long-running task depending on network connectivity, etc. Have you not run into issues where the task is killed?

You should be able to call syncData() directly in
https://github.com/e-mission/e-mission-data-collection/blob/master/src/android/location/TripDiaryStateMachineService.java#L288

you can invoke the background process from there - the current implementation uses several async Tasks. Not sure if you can invoke a coroutine from a java file, but I would also be open to converting the java -> kotlin if that is easier for you. Just have to be careful to test since we don't currently have unit tests for the native code....

@ericafenyo
Copy link
Contributor

@shankari @PatGendre
I created two PRs to sync data after a trip end

My tests: https://drive.google.com/file/d/13aElKiX4gjr7ioIh6qWQ7RaOsTX0zsH2/view?usp=sharing

@shankari
Copy link
Contributor

@ericafenyo thanks for the manual testing. One of the goals of the additional funding from DOE is to introduce automated testing on the phone as well.

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

No branches or pull requests

3 participants