Fix runtime left behind on app update (#31)

* Fix runtime left behind on app update

* Always kill runtime synchronously and run it asynchronously

* More log output

app/src/main/java/com/nutomic/syncthingandroid/fragments/DrawerFragment.java

@@ -169,6 +169,7 @@ public class DrawerFragment extends Fragment implements SyncthingService.OnServi
                 mActivity.closeDrawer();
                 break;
             case R.id.drawerActionExit:
+                Log.i(TAG, "Exiting app on user request");
                 mActivity.stopService(new Intent(mActivity, SyncthingService.class));
                 mActivity.finish();
                 mActivity.closeDrawer();

app/src/main/java/com/nutomic/syncthingandroid/service/SyncthingRunnable.java

@@ -133,10 +133,14 @@ public class SyncthingRunnable implements Runnable {
                 ? pm.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, TAG)
                 : null;
         try {
-            if (wakeLock != null)
+            if (wakeLock != null) {
                 wakeLock.acquire();
-            increaseInotifyWatches();
+            }
 
+            /**
+             * Setup and run a new syncthing instance
+             */
+            increaseInotifyWatches();
             HashMap<String, String> targetEnv = buildEnvironment();
             process = setupAndLaunch(targetEnv);
 
@@ -216,11 +220,13 @@ public class SyncthingRunnable implements Runnable {
         } catch (IOException | InterruptedException e) {
             Log.e(TAG, "Failed to execute syncthing binary or read output", e);
         } finally {
-            if (wakeLock != null)
+            if (wakeLock != null) {
                 wakeLock.release();
-            if (process != null)
+            }
+            if (process != null) {
                 process.destroy();
             }
+        }
 
         // Restart syncthing if it exited unexpectedly while running on a separate thread.
         if (!returnStdOut && restartSyncthingNative) {
@@ -346,7 +352,7 @@ public class SyncthingRunnable implements Runnable {
         int exitCode;
         List<String> syncthingPIDs = getSyncthingPIDs();
         if (syncthingPIDs.isEmpty()) {
-            Log.d(TAG, "killSyncthing: Found no more running instances of " + Constants.FILENAME_SYNCTHING_BINARY);
+            Log.d(TAG, "killSyncthing: Found no running instances of " + Constants.FILENAME_SYNCTHING_BINARY);
             return;
         }
         for (String syncthingPID : syncthingPIDs) {
@@ -358,6 +364,15 @@ public class SyncthingRunnable implements Runnable {
                     " exit code " + Integer.toString(exitCode));
             }
         }
+
+        /**
+         * Wait for the syncthing instance to end.
+         */
+        Log.v(TAG, "Waiting for all syncthing instances to end ...");
+        while (!getSyncthingPIDs().isEmpty()) {
+            SystemClock.sleep(50);
+        }
+        Log.v(TAG, "killSyncthing: Complete.");
     }
 
     /**

app/src/main/java/com/nutomic/syncthingandroid/service/SyncthingService.java

@@ -236,18 +236,19 @@ public class SyncthingService extends Service {
         }
 
         if (ACTION_RESTART.equals(intent.getAction()) && mCurrentState == State.ACTIVE) {
-            shutdown(State.INIT, () -> launchStartupTask());
+            shutdown(State.INIT, () -> launchStartupTask(SyncthingRunnable.Command.main));
         } else if (ACTION_STOP.equals(intent.getAction()) && mCurrentState == State.ACTIVE) {
             shutdown(State.DISABLED, () -> {});
         } else if (ACTION_RESET_DATABASE.equals(intent.getAction())) {
+            Log.i(TAG, "Invoking reset of database");
             shutdown(State.INIT, () -> {
                 new SyncthingRunnable(this, SyncthingRunnable.Command.resetdatabase).run();
-                launchStartupTask();
+                launchStartupTask(SyncthingRunnable.Command.main);
             });
         } else if (ACTION_RESET_DELTAS.equals(intent.getAction())) {
+            Log.i(TAG, "Invoking reset of delta indexes");
             shutdown(State.INIT, () -> {
-                new SyncthingRunnable(this, SyncthingRunnable.Command.resetdeltas).run();
+                launchStartupTask(SyncthingRunnable.Command.resetdeltas);
-                launchStartupTask();
             });
         } else if (ACTION_REFRESH_NETWORK_INFO.equals(intent.getAction())) {
             mRunConditionMonitor.updateShouldRunDecision();
@@ -282,11 +283,7 @@ public class SyncthingService extends Service {
                 switch (mCurrentState) {
                     case DISABLED:
                     case INIT:
-                        // HACK: Make sure there is no syncthing binary left running from an improper
+                        launchStartupTask(SyncthingRunnable.Command.main);
-                        // shutdown (eg Play Store update).
-                        shutdown(State.INIT, () -> {
-                            launchStartupTask();
-                        });
                         break;
                     case STARTING:
                     case ACTIVE:
@@ -309,10 +306,10 @@ public class SyncthingService extends Service {
     /**
      * Prepares to launch the syncthing binary.
      */
-    private void launchStartupTask () {
+    private void launchStartupTask (SyncthingRunnable.Command srCommand) {
         Log.v(TAG, "Starting syncthing");
         synchronized(mStateLock) {
-            if (mCurrentState != State.INIT) {
+            if (mCurrentState != State.DISABLED && mCurrentState != State.INIT) {
                 Log.e(TAG, "launchStartupTask: Wrong state " + mCurrentState + " detected. Cancelling.");
                 return;
             }
@@ -324,7 +321,7 @@ public class SyncthingService extends Service {
             return;
         }
         onServiceStateChange(State.STARTING);
-        mStartupTask = new StartupTask(this);
+        mStartupTask = new StartupTask(this, srCommand);
         mStartupTask.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
     }
 
@@ -334,9 +331,11 @@ public class SyncthingService extends Service {
      */
      private static class StartupTask extends AsyncTask<Void, Void, Void> {
          private WeakReference<SyncthingService> refSyncthingService;
+         private SyncthingRunnable.Command srCommand;
 
-         StartupTask(SyncthingService context) {
+         StartupTask(SyncthingService context, SyncthingRunnable.Command srCommand) {
              refSyncthingService = new WeakReference<>(context);
+             this.srCommand = srCommand;
          }
 
          @Override
@@ -364,7 +363,7 @@ public class SyncthingService extends Service {
              // Get a reference to the service if it is still there.
              SyncthingService syncthingService = refSyncthingService.get();
              if (syncthingService != null) {
-                 syncthingService.onStartupTaskCompleteListener();
+                 syncthingService.onStartupTaskCompleteListener(srCommand);
              }
          }
      }
@@ -372,19 +371,27 @@ public class SyncthingService extends Service {
      /**
       * Callback on {@link StartupTask#onPostExecute}.
       */
-     private void onStartupTaskCompleteListener() {
+     private void onStartupTaskCompleteListener(SyncthingRunnable.Command srCommand) {
          if (mApi == null) {
              mApi = new RestApi(this, mConfig.getWebGuiUrl(), mConfig.getApiKey(),
                                  this::onApiAvailable, () -> onServiceStateChange(mCurrentState));
              Log.i(TAG, "Web GUI will be available at " + mConfig.getWebGuiUrl());
          }
 
-         // Start the syncthing binary.
+         // Check mSyncthingRunnable lifecycle and create singleton.
          if (mSyncthingRunnable != null || mSyncthingRunnableThread != null) {
              Log.e(TAG, "onStartupTaskCompleteListener: Syncthing binary lifecycle violated");
              return;
          }
-         mSyncthingRunnable = new SyncthingRunnable(this, SyncthingRunnable.Command.main);
+         mSyncthingRunnable = new SyncthingRunnable(this, srCommand);
+
+         /**
+          * Check if an old syncthing instance is still running.
+          * This happens after an in-place app upgrade. If so, end it.
+          */
+         mSyncthingRunnable.killSyncthing();
+
+         // Start the syncthing binary in a separate thread.
          mSyncthingRunnableThread = new Thread(mSyncthingRunnable);
          mSyncthingRunnableThread.start();
 
@@ -643,7 +650,7 @@ public class SyncthingService extends Service {
             } catch (IOException e) {
                 Log.w(TAG, "Failed to import config", e);
             }
-            launchStartupTask();
+            launchStartupTask(SyncthingRunnable.Command.main);
         });
         return true;
     }