From 964133983d969c145403a2545ed7759aa2db6461 Mon Sep 17 00:00:00 2001 From: "K. Shankari" Date: Wed, 13 Mar 2019 13:20:52 -0700 Subject: [PATCH] Handle re-entrant calls to the location FSM Broad idea of the updates https://github.com/e-mission/e-mission-docs/issues/325#issuecomment-472294766 and https://github.com/e-mission/e-mission-docs/issues/325#issuecomment-472542739 --- .../TripDiaryStateMachineService.java | 172 +++++++++++++----- 1 file changed, 124 insertions(+), 48 deletions(-) diff --git a/src/android/location/TripDiaryStateMachineService.java b/src/android/location/TripDiaryStateMachineService.java index 8c5a3b3e..c02a1ccb 100644 --- a/src/android/location/TripDiaryStateMachineService.java +++ b/src/android/location/TripDiaryStateMachineService.java @@ -42,8 +42,6 @@ - - import edu.berkeley.eecs.emission.cordova.tracker.location.actions.ActivityRecognitionActions; import edu.berkeley.eecs.emission.cordova.tracker.location.actions.GeofenceActions; import edu.berkeley.eecs.emission.cordova.tracker.location.actions.LocationTrackingActions; @@ -78,6 +76,7 @@ public class TripDiaryStateMachineService extends Service implements private SharedPreferences mPrefs = null; // List of actions being currently processed private List currActions = null; + private int ongoingOperations; public TripDiaryStateMachineService() { super(); @@ -123,10 +122,10 @@ public int onStartCommand(Intent intent, int flags, int startId) { Log.d(this, TAG, "service restarted! need to check idempotency!"); } mTransition = intent.getAction(); + ongoingOperations = ongoingOperations + 1; if (currActions.contains(mTransition)) { Log.i(this, TAG, "Service started again for "+mTransition+ " while processing "+currActions+" early exit from id " + startId); - // stopSelf(startId); return START_REDELIVER_INTENT; } Log.i(this, TAG, "Handling new action "+mTransition+ @@ -179,6 +178,8 @@ public void onConnected(Bundle connectionHint) { } else { NotificationHelper.createNotification(this, Constants.TRACKING_ERROR_ID, locResult.getErrorMessage()); } + // we have generated a notification, and we are not going to handle the action, so we can stop the service now + setNewState(mCurrState); } if (!activityResult.isSuccess()) { if (activityResult.hasResolution()) { @@ -187,9 +188,10 @@ public void onConnected(Bundle connectionHint) { } else { NotificationHelper.createNotification(this, Constants.TRACKING_ERROR_ID, activityResult.getErrorMessage()); } + // we have generated a notification, and we are not going to handle the action, so we can stop the service now + setNewState(mCurrState); } - // Note that it does NOT work to disconnect from here because the actions in the state // might happen asynchronously, and we disconnect too early, then the async callbacks // are never invoked. In particular, anything called from the "main/UI" thread, such as @@ -217,6 +219,12 @@ public static void restartFSMIfStartState(Context ctxt) { } } + private void markOngoingOperationFinished() { + synchronized (this) { + ongoingOperations = ongoingOperations - 1; + } + } + public void setNewState(String newState) { Log.d(this, TAG, "newState after handling action is "+newState); SharedPreferences.Editor prefsEditor = @@ -226,10 +234,15 @@ public void setNewState(String newState) { Log.d(this, TAG, "newState saved in prefManager is "+ PreferenceManager.getDefaultSharedPreferences(this).getString( this.getString(R.string.curr_state_key), "not found")); - currActions.remove(mTransition); - Log.i(this, TAG, "After removing transition "+mTransition+ - ", currActions = "+currActions); + synchronized (this) { + ongoingOperations = ongoingOperations - 1; + if (ongoingOperations == 0) { + Log.i(this, TAG, "About to stop service after handling "+currActions); stopSelf(); + } else { + Log.i(this, TAG, ongoingOperations + " ongoingOperations pending, waiting to stop"); + } + } } @Override @@ -242,13 +255,14 @@ public void onConnectionSuspended(int cause) { } NotificationHelper.createNotification(this, STATE_IN_NUMBERS, "google API client connection suspended"+causeStr); + // let's leave this here in case the connection is restored } @Override public void onConnectionFailed(ConnectionResult cr) { NotificationHelper.createNotification(this, STATE_IN_NUMBERS, "google API client connection failed"+cr.toString()); - + setNewState(this.getString(R.string.state_start)); } private Intent getForegroundServiceIntent() { @@ -264,7 +278,7 @@ private Intent getForegroundServiceIntent() { * as parameters, makes the call, and issues the broadcast in the callback */ private void handleAction(Context ctxt, GoogleApiClient apiClient, String currState, String actionString) { - Log.d(this, TAG, "handleAction("+currState+", "+actionString+") calle"); + Log.d(this, TAG, "handleAction("+currState+", "+actionString+") called"); assert(currState != null); // The current state is stored in the shared preferences, so on reboot, for example, we would // store that we are in ongoing_trip, but no listeners would be registered. We can have @@ -289,6 +303,7 @@ private void handleAction(Context ctxt, GoogleApiClient apiClient, String currSt } else if (currState.equals(ctxt.getString(R.string.state_tracking_stopped))) { handleTrackingStopped(ctxt, apiClient, actionString); } + Log.d(this, TAG, "handleAction("+currState+", "+actionString+") completed, waiting for async operations to complete"); } private void handleStart(final Context ctxt, final GoogleApiClient apiClient, String actionString) { @@ -297,6 +312,7 @@ private void handleStart(final Context ctxt, final GoogleApiClient apiClient, St if (actionString.equals(ctxt.getString(R.string.transition_initialize)) && !mCurrState.equals(ctxt.getString(R.string.state_tracking_stopped))) { createGeofenceInThread(ctxt, apiClient, actionString); + // we will wait for async geofence creation to complete } // One would think that we don't need to deal with anything other than starting from the start @@ -324,11 +340,23 @@ public void handleTripStart(Context ctxt, final GoogleApiClient apiClient, final if (actionString.equals(ctxt.getString(R.string.transition_exited_geofence))) { // Delete geofence + // we cannot add null elements to the token list. + // the LocationTracking start action can now return null + // so we need to handle it similar to the createGeofence in handleTripEnd final List> tokenList = new LinkedList>(); Batch.Builder resultBarrier = new Batch.Builder(apiClient); tokenList.add(resultBarrier.add(new GeofenceActions(ctxt, apiClient).remove())); - tokenList.add(resultBarrier.add(new LocationTrackingActions(ctxt, apiClient).start())); tokenList.add(resultBarrier.add(new ActivityRecognitionActions(ctxt, apiClient).start())); + PendingResult locationTrackingResult = new LocationTrackingActions(ctxt, apiClient).start(); + if (locationTrackingResult != null) { + tokenList.add(resultBarrier.add(locationTrackingResult)); + } else { + // if we can't turn on the location tracking, we may as well not start the activity + // recognition + tokenList.remove(1); + } + final boolean locationTrackingPossible = locationTrackingResult != null; + // TODO: How to pass in the token list? // Also, the callback is currently the same for all of them, but could potentially be // different in the future once we add in failure handling because we may want to do @@ -339,39 +367,62 @@ public void handleTripStart(Context ctxt, final GoogleApiClient apiClient, final resultBarrier.build().setResultCallback(new ResultCallback() { @Override public void onResult(BatchResult batchResult) { - String newState = fCtxt.getString(R.string.state_ongoing_trip); + String newState; + if (locationTrackingPossible) { + newState = fCtxt.getString(R.string.state_ongoing_trip); + } else { + // If we are not going to be able to start location tracking, then we don't + // want to go to ongoing_trip, because then we will never exit + // from it. Instead, we go to state_start so that we will try to get + // out of it at every sync. + newState = fCtxt.getString(R.string.state_start); + } + if (batchResult.getStatus().isSuccess()) { - setNewState(newState); + if (locationTrackingPossible) { startService(getForegroundServiceIntent()); + } if (ConfigManager.getConfig(fCtxt).isSimulateUserInteraction()) { NotificationHelper.createNotification(fCtxt, STATE_IN_NUMBERS, "Success moving to "+newState); } + setNewState(newState); } else { if (batchResult.getStatus().hasResolution()) { NotificationHelper.createResolveNotification(fCtxt, STATE_IN_NUMBERS, "Error " + batchResult.getStatus().getStatusCode()+" while creating geofence", batchResult.getStatus().getResolution()); + // we should set something here to stop the service since our async call + // is complete and since we already have a resolution, we are not going to do anything more + // but the set value depends on the result if the geofence deletion failed but the tracking started, we want + // to go to the ongoing_trip state anyway... + if (locationTrackingPossible && batchResult.take(tokenList.get(2)).isSuccess()) { + // the location tracking started successfully + setNewState(fCtxt.getString(R.string.state_ongoing_trip)); + } else { + setNewState(fCtxt.getString(R.string.state_start)); + } } else { - NotificationHelper.createNotification(fCtxt, STATE_IN_NUMBERS, - "Error " + batchResult.getStatus().getStatusCode()+" while creating geofence"); + // NotificationHelper.createNotification(fCtxt, STATE_IN_NUMBERS, + // "Error " + batchResult.getStatus().getStatusCode()+" while creating geofence"); + // this will perform some additional checks which we should wait for + // let's mark this operation as done since the other one is static + markOngoingOperationFinished(); checkLocationSettingsAndPermissions(TripDiaryStateMachineService.this, mApiClient); } if (ConfigManager.getConfig(fCtxt).isSimulateUserInteraction()) { NotificationHelper.createNotification(fCtxt, STATE_IN_NUMBERS, "Failed moving to "+newState); } - setNewState(mCurrState); - } - Log.d(fCtxt, TAG, "About to disconnect the api client"); - mApiClient.disconnect(); - } - }); + } // all three branches have called setState or are waiting for sth else + } // onResult function end + }); // result callback inner class end } if(actionString.equals(ctxt.getString(R.string.transition_stop_tracking))) { // Delete geofence deleteGeofence(ctxt, apiClient, ctxt.getString(R.string.state_tracking_stopped)); + // when this completes, it should generate transitions and move to the final state } if (actionString.equals(ctxt.getString(R.string.transition_tracking_error))) { @@ -381,7 +432,7 @@ public void onResult(BatchResult batchResult) { */ Log.i(this, TAG, "Got tracking_error moving to start state"); deleteGeofence(ctxt, mApiClient, ctxt.getString(R.string.state_start)); - setNewState(getString(R.string.state_start)); + // when this completes, it should generate transitions and move to the final state } } @@ -418,30 +469,46 @@ public void onResult(BatchResult batchResult) { newState = fCtxt.getString(R.string.state_start); } if (batchResult.getStatus().isSuccess()) { - setNewState(newState); stopService(getForegroundServiceIntent()); if (ConfigManager.getConfig(ctxt).isSimulateUserInteraction()) { NotificationHelper.createNotification(fCtxt, STATE_IN_NUMBERS, "Success moving to "+newState); } + setNewState(newState); } else { if (batchResult.getStatus().hasResolution()) { NotificationHelper.createResolveNotification(fCtxt, STATE_IN_NUMBERS, "Error " + batchResult.getStatus().getStatusCode()+" while creating geofence", batchResult.getStatus().getResolution()); + // we should set something here to stop the service since our async call + // is complete and since we already have a resolution, we are not going to do anything more + // but the set value depends on the result if the geofence creation failed, we + // want to go to the start state, but if the location tracking stop failed, + // we want to stay in the ongoing trip state + if (!batchResult.take(tokenList.get(0)).isSuccess()) { + // the location tracking stop failed + setNewState(fCtxt.getString(R.string.state_ongoing_trip)); + } else if (geofenceCreationPossible && + batchResult.take(tokenList.get(2)).isSuccess()) { + setNewState(fCtxt.getString(R.string.state_waiting_for_trip_start)); + } else { + // geofence creation is not possible or it failed but location tracking + // did successfully stop. Let's go to the start state + setNewState(fCtxt.getString(R.string.state_start)); + } } else { - NotificationHelper.createNotification(fCtxt, STATE_IN_NUMBERS, - "Error " + batchResult.getStatus().getStatusCode()+" while creating geofence"); + // NotificationHelper.createNotification(fCtxt, STATE_IN_NUMBERS, + // "Error " + batchResult.getStatus().getStatusCode()+" while creating geofence"); + // let's mark this operation as done since the other one is static + markOngoingOperationFinished(); checkLocationSettingsAndPermissions(TripDiaryStateMachineService.this, mApiClient); + // will wait for async call to complete } if (ConfigManager.getConfig(ctxt).isSimulateUserInteraction()) { NotificationHelper.createNotification(fCtxt, STATE_IN_NUMBERS, "Failed moving to "+newState); } - setNewState(mCurrState); } - Log.d(fCtxt, TAG, "About to disconnect the api client"); - mApiClient.disconnect(); } }); } @@ -450,6 +517,7 @@ public void onResult(BatchResult batchResult) { if (actionString.equals(ctxt.getString(R.string.transition_stop_tracking))) { stopAll(ctxt, apiClient, ctxt.getString(R.string.state_tracking_stopped)); + // will wait for stopAll to set the state } if (actionString.equals(ctxt.getString(R.string.transition_tracking_error))) { @@ -460,7 +528,7 @@ public void onResult(BatchResult batchResult) { Log.i(this, TAG, "Got tracking_error moving to start state"); // should I stop everything? maybe to be consistent with the start state stopAll(this, mApiClient, ctxt.getString(R.string.state_start)); - setNewState(getString(R.string.state_start)); + // ditto } } @@ -517,13 +585,16 @@ public void onResult(Status status) { "Success moving to " + newState); } } else { - setNewState(mCurrState); if (status.hasResolution()) { NotificationHelper.createResolveNotification(fCtxt, STATE_IN_NUMBERS, "Error " + status.getStatusCode()+" while creating geofence", status.getResolution()); + // we have a resolution so we will exit the service now + setNewState(mCurrState); } else { - NotificationHelper.createNotification(fCtxt, STATE_IN_NUMBERS, - "Error " + status.getStatusCode()+" while creating geofence"); + // NotificationHelper.createNotification(fCtxt, STATE_IN_NUMBERS, + // "Error " + status.getStatusCode()+" while creating geofence"); + // let's mark this operation as done since the other one is static + markOngoingOperationFinished(); checkLocationSettingsAndPermissions(TripDiaryStateMachineService.this, mApiClient); } if (ConfigManager.getConfig(fCtxt).isSimulateUserInteraction()) { @@ -531,14 +602,14 @@ public void onResult(Status status) { "Failed moving to " + newState); } } - Log.d(fCtxt, TAG, "About to disconnect the api client"); - fApiClient.disconnect(); } }); } else { - // Geofence was not created properly. let's generate a tracking error so that the user is notified + // Geofence was not created properly. let's make an async call that will generate its + // own state change + // let's mark this operation as done since the other one is static + markOngoingOperationFinished(); checkLocationSettingsAndPermissions(fCtxt, fApiClient); - setNewState(mCurrState); } } }).start(); @@ -564,22 +635,21 @@ public void onResult(BatchResult batchResult) { NotificationHelper.createResolveNotification(fCtxt, STATE_IN_NUMBERS, "Error " + batchResult.getStatus().getStatusCode() + " while creating geofence", batchResult.getStatus().getResolution()); + setNewState(mCurrState); } else { - NotificationHelper.createNotification(fCtxt, STATE_IN_NUMBERS, - "Error " + batchResult.getStatus().getStatusCode() + " while creating geofence"); + // NotificationHelper.createNotification(fCtxt, STATE_IN_NUMBERS, + // "Error " + batchResult.getStatus().getStatusCode() + " while creating geofence"); + // let's mark this operation as done since the other one is static + markOngoingOperationFinished(); checkLocationSettingsAndPermissions(TripDiaryStateMachineService.this, mApiClient); } if (ConfigManager.getConfig(fCtxt).isSimulateUserInteraction()) { NotificationHelper.createNotification(fCtxt, STATE_IN_NUMBERS, "Failed moving to " + newState); } - setNewState(mCurrState); } - Log.d(fCtxt, TAG, "About to disconnect the api client"); - mApiClient.disconnect(); } }); - } private void stopAll(Context ctxt, GoogleApiClient apiClient, final String targetState) { @@ -596,21 +666,25 @@ private void stopAll(Context ctxt, GoogleApiClient apiClient, final String targe public void onResult(BatchResult batchResult) { String newState = targetState; if (batchResult.getStatus().isSuccess()) { - setNewState(newState); stopService(getForegroundServiceIntent()); if (ConfigManager.getConfig(fCtxt).isSimulateUserInteraction()) { NotificationHelper.createNotification(fCtxt, STATE_IN_NUMBERS, "Success moving to "+newState); } + setNewState(newState); } else { if (ConfigManager.getConfig(fCtxt).isSimulateUserInteraction()) { NotificationHelper.createNotification(fCtxt, STATE_IN_NUMBERS, "Failed moving to "+newState); } - setNewState(mCurrState); + + if (!batchResult.take(tokenList.get(1)).isSuccess()) { + // the location tracking stop failed + setNewState(fCtxt.getString(R.string.state_ongoing_trip)); + } else { + setNewState(newState); + } } - Log.d(fCtxt, TAG, "About to disconnect the api client"); - mApiClient.disconnect(); } }); } @@ -622,8 +696,10 @@ public static void checkLocationSettingsAndPermissions(final Context ctxt, final if (checkLocationPermissions(ctxt, apiClient, request)) { Log.d(ctxt, TAG, "checkPermissions returned true, checking settings"); checkLocationSettings(ctxt, apiClient, request); + // final state will be set in this async call } else { Log.d(ctxt, TAG, "checkPermissions returned false, no point checking settings"); + ctxt.sendBroadcast(new ExplicitIntent(ctxt, R.string.transition_tracking_error)); } } @@ -677,7 +753,6 @@ public void onResult(LocationSettingsResult result) { case LocationSettingsStatusCodes.RESOLUTION_REQUIRED: // Location settings are not satisfied. But could be fixed by showing the user // a dialog. - ctxt.sendBroadcast(new ExplicitIntent(ctxt, R.string.transition_tracking_error)); if (status.hasResolution()) { NotificationHelper.createResolveNotification(ctxt, Constants.TRACKING_ERROR_ID, "Error " + status.getStatusCode() + " in location settings", @@ -686,18 +761,19 @@ public void onResult(LocationSettingsResult result) { NotificationHelper.createNotification(ctxt, Constants.TRACKING_ERROR_ID, "Error " + status.getStatusCode() + " in location settings"); } + ctxt.sendBroadcast(new ExplicitIntent(ctxt, R.string.transition_tracking_error)); break; case LocationSettingsStatusCodes.SETTINGS_CHANGE_UNAVAILABLE: // Location settings are not satisfied. However, we have no way to fix the // settings so we won't show the dialog. - ctxt.sendBroadcast(new ExplicitIntent(ctxt, R.string.transition_tracking_error)); NotificationHelper.createNotification(ctxt, Constants.TRACKING_ERROR_ID, "Error " + status.getStatusCode() + " in location settings"); + ctxt.sendBroadcast(new ExplicitIntent(ctxt, R.string.transition_tracking_error)); break; default: - ctxt.sendBroadcast(new ExplicitIntent(ctxt, R.string.transition_tracking_error)); NotificationHelper.createNotification(ctxt, Constants.TRACKING_ERROR_ID, "Unknown error while reading location, please check your settings"); + ctxt.sendBroadcast(new ExplicitIntent(ctxt, R.string.transition_tracking_error)); } } });