Refactor SyncthingService, moving killSyncthing() to background

src/main/java/com/nutomic/syncthingandroid/activities/StateDialogActivity.java

@@ -6,7 +6,6 @@ import android.content.Intent;
 import android.os.Bundle;
 import android.support.annotation.Nullable;
 import android.support.v4.app.ActivityCompat;
-import android.util.Log;
 import android.view.LayoutInflater;
 import android.view.View;
 import android.widget.TextView;
@@ -98,15 +97,10 @@ public abstract class StateDialogActivity extends SyncthingActivity {
                 ? R.string.web_gui_creating_key
                 : R.string.api_loading);
 
-        try {
+        mLoadingDialog = new AlertDialog.Builder(this)
-            mLoadingDialog = new AlertDialog.Builder(this)
+                .setCancelable(false)
-                    .setCancelable(false)
+                .setView(dialogLayout)
-                    .setView(dialogLayout)
+                .show();
-                    .show();
-        } catch (RuntimeException e) {
-            // Catch and do nothing, workaround for https://stackoverflow.com/q/46030692/1837158
-            Log.w(TAG, e);
-        }
     }
 
     private void dismissLoadingDialog() {

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

@@ -28,8 +28,8 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.io.LineNumberReader;
-import java.util.Map;
 import java.util.HashMap;
+import java.util.Map;
 import java.util.concurrent.atomic.AtomicReference;
 
 import eu.chainfire.libsuperuser.Shell;
@@ -149,8 +149,7 @@ public class SyncthingRunnable implements Runnable {
                     // Syncthing was shut down (via API or SIGKILL), do nothing.
                     break;
                 case 1:
-                    // Syncthing is already running, kill it and try again.
+                    Log.w(TAG, "Another Syncthing instance is already running, requesting restart via SyncthingService intent");
-                    killSyncthing();
                     //fallthrough
                 case 3:
                     // Restart was requested via Rest API call.
@@ -246,42 +245,48 @@ public class SyncthingRunnable implements Runnable {
         }.start();
     }
 
+    public interface OnSyncthingKilled {
+        void onKilled();
+    }
     /**
      * Look for running libsyncthing.so processes and kill them.
      * Try a SIGINT first, then try again with SIGKILL.
      */
-    public void killSyncthing() {
+    public void killSyncthing(OnSyncthingKilled onKilledListener) {
-        for (int i = 0; i < 2; i++) {
+        new Thread(() -> {
-            Process ps = null;
+            for (int i = 0; i < 2; i++) {
-            DataOutputStream psOut = null;
+                Process ps = null;
-            try {
+                DataOutputStream psOut = null;
-                ps = Runtime.getRuntime().exec((mUseRoot) ? "su" : "sh");
-                psOut = new DataOutputStream(ps.getOutputStream());
-                psOut.writeBytes("ps | grep libsyncthing.so\n");
-                psOut.writeBytes("exit\n");
-                psOut.flush();
-                ps.waitFor();
-                InputStreamReader isr = new InputStreamReader(ps.getInputStream(), "UTF-8");
-                BufferedReader br = new BufferedReader(isr);
-                String id;
-                while ((id = br.readLine()) != null) {
-                    id = id.trim().split("\\s+")[1];
-                    killProcessId(id, i > 0);
-                }
-            } catch (IOException | InterruptedException e) {
-                Log.w(TAG_KILL, "Failed list Syncthing processes", e);
-            } finally {
                 try {
-                    if (psOut != null)
+                    ps = Runtime.getRuntime().exec((mUseRoot) ? "su" : "sh");
-                        psOut.close();
+                    psOut = new DataOutputStream(ps.getOutputStream());
-                } catch (IOException e) {
+                    psOut.writeBytes("ps | grep libsyncthing.so\n");
-                    Log.w(TAG_KILL, "Failed close the psOut stream", e);
+                    psOut.writeBytes("exit\n");
-                }
+                    psOut.flush();
-                if (ps != null) {
+                    ps.waitFor();
-                    ps.destroy();
+                    InputStreamReader isr = new InputStreamReader(ps.getInputStream(), "UTF-8");
+                    BufferedReader br = new BufferedReader(isr);
+                    String id;
+                    while ((id = br.readLine()) != null) {
+                        id = id.trim().split("\\s+")[1];
+                        killProcessId(id, i > 0);
+                    }
+                } catch (IOException | InterruptedException e) {
+                    Log.w(TAG_KILL, "Failed list Syncthing processes", e);
+                } finally {
+                    try {
+                        if (psOut != null)
+                            psOut.close();
+                    } catch (IOException e) {
+                        Log.w(TAG_KILL, "Failed close the psOut stream", e);
+                    }
+                    if (ps != null) {
+                        ps.destroy();
+                    }
                 }
             }
-        }
+            onKilledListener.onKilled();
+        }).start();
     }
 
     /**
@@ -403,11 +408,9 @@ public class SyncthingRunnable implements Runnable {
     }
 
     private Process setupAndLaunch(HashMap<String, String> env) throws IOException {
-        Process process = null;
-
         if (mUseRoot) {
             ProcessBuilder pb = new ProcessBuilder("su");
-            process = pb.start();
+            Process process = pb.start();
             // The su binary prohibits the inheritance of environment variables.
             // Even with --preserve-environment the environment gets messed up.
             // We therefore start a root shell, and set all the environment variables manually.
@@ -421,12 +424,11 @@ public class SyncthingRunnable implements Runnable {
             // If we did not use exec, we would wait infinitely for the process to terminate (ret = process.waitFor(); in run()).
             // With exec the whole process terminates when Syncthing exits.
             suOut.writeBytes("exec " + TextUtils.join(" ", mCommand) + "\n");
-            suOut = null;
+            return process;
         } else {
             ProcessBuilder pb = new ProcessBuilder(mCommand);
             pb.environment().putAll(env);
-            process = pb.start();
+            return pb.start();
         }
-        return process;
     }
 }

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

@@ -18,7 +18,6 @@ import android.preference.PreferenceManager;
 import android.support.annotation.Nullable;
 import android.support.v4.app.NotificationCompat;
 import android.util.Log;
-import android.util.Pair;
 import android.widget.Toast;
 
 import com.android.PRNGFixes;
@@ -39,6 +38,7 @@ import java.util.HashSet;
 import java.util.Iterator;
 import java.util.LinkedList;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 /**
  * Holds the native syncthing instance and provides an API to access it.
@@ -135,17 +135,19 @@ public class SyncthingService extends Service implements
     };
 
     /**
-     * INIT: Service is starting up and initializing.
+     * Indicates the current state of SyncthingService and of Syncthing itself.
-     * STARTING: Syncthing binary is starting (but the API is not yet ready).
-     * ACTIVE: Syncthing binary is up and running.
-     * DISABLED: Syncthing binary is stopped according to user preferences.
      */
     public enum State {
+        /** Service is initializing, Syncthing was not started yet. */
         INIT,
+        /** Syncthing binary is starting. */
         STARTING,
+        /** Syncthing binary is running, API is available. */
         ACTIVE,
+        /** Syncthing is stopped according to user preferences. */
         DISABLED,
-        ERROR
+        /** There is some problem that prevents Syncthing from running. */
+        ERROR,
     }
 
     private State mCurrentState = State.INIT;
@@ -164,7 +166,7 @@ public class SyncthingService extends Service implements
 
     private DeviceStateHolder mDeviceStateHolder;
 
-    private SyncthingRunnable mRunnable;
+    private SyncthingRunnable mSyncthingRunnable;
 
     /**
      * Handles intents, either {@link #ACTION_RESTART}, or intents having
@@ -177,14 +179,12 @@ public class SyncthingService extends Service implements
             return START_STICKY;
 
         if (ACTION_RESTART.equals(intent.getAction()) && mCurrentState == State.ACTIVE) {
-            shutdown();
+            shutdown(State.INIT, () -> new StartupTask().executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR));
-            mCurrentState = State.INIT;
-            updateState();
         } else if (ACTION_RESET.equals(intent.getAction())) {
-            shutdown();
+            shutdown(State.INIT, () -> {
-            new SyncthingRunnable(this, SyncthingRunnable.Command.reset).run();
+                new SyncthingRunnable(this, SyncthingRunnable.Command.reset).run();
-            mCurrentState = State.INIT;
+                new StartupTask().executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
-            updateState();
+            });
         } else {
             mDeviceStateHolder.update(intent);
             updateState();
@@ -196,7 +196,7 @@ public class SyncthingService extends Service implements
      * Checks according to preferences and charging/wifi state, whether syncthing should be enabled
      * or not.
      *
-     * Depending on the result, syncthing is started or stopped, and {@link #onApiChange()} is
+     * Depending on the result, syncthing is started or stopped, and {@link #onApiChange} is
      * called.
      */
     private void updateState() {
@@ -209,30 +209,10 @@ public class SyncthingService extends Service implements
 
             // HACK: Make sure there is no syncthing binary left running from an improper
             // shutdown (eg Play Store update).
-            // NOTE: This will log an exception if syncthing is not actually running.
+            shutdown(State.INIT, () -> {
-            shutdown();
+                Log.i(TAG, "Starting syncthing according to current state and preferences");
-
+                new StartupTask().executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
-            Log.i(TAG, "Starting syncthing according to current state and preferences");
+            });
-            mConfig = null;
-            try {
-                mConfig = new ConfigXml(SyncthingService.this);
-            } catch (ConfigXml.OpenConfigException e) {
-                mCurrentState = State.ERROR;
-                Toast.makeText(this, R.string.config_create_failed, Toast.LENGTH_LONG).show();
-            }
-
-            if (mConfig != null) {
-                mCurrentState = State.STARTING;
-
-                if (mApi != null)
-                    registerOnWebGuiAvailableListener(mApi);
-                if (mEventProcessor != null)
-                    registerOnWebGuiAvailableListener(mEventProcessor);
-                pollWebGui();
-                mRunnable = new SyncthingRunnable(this, SyncthingRunnable.Command.main);
-                new Thread(mRunnable).start();
-                updateNotification();
-            }
         }
         // Stop syncthing.
         else {
@@ -240,11 +220,8 @@ public class SyncthingService extends Service implements
                 return;
 
             Log.i(TAG, "Stopping syncthing according to current state and preferences");
-            mCurrentState = State.DISABLED;
+            shutdown(State.DISABLED, () -> {});
-
-            shutdown();
         }
-        onApiChange();
     }
 
     /**
@@ -321,68 +298,74 @@ public class SyncthingService extends Service implements
             registerReceiver(mNetworkReceiver,
                     new IntentFilter(ConnectivityManager.CONNECTIVITY_ACTION));
         }
-        new StartupTask().executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
+        updateState();
         PreferenceManager.getDefaultSharedPreferences(this)
                 .registerOnSharedPreferenceChangeListener(this);
     }
 
     /**
-     * Sets up the initial configuration, updates the config when coming from an old
+     * Sets up the initial configuration, and updates the config when coming from an old
-     * version, and reads syncthing URL and API key (these are passed internally as
+     * version.
-     * {@code Pair<String, String>}.
      */
-    private class StartupTask extends AsyncTask<Void, Void, Pair<URL, String>> {
+    private class StartupTask extends AsyncTask<Void, Void, Void> {
+
+        public StartupTask() {
+            mCurrentState = State.STARTING;
+        }
 
         @Override
-        protected Pair<URL, String> doInBackground(Void... voids) {
+        protected Void doInBackground(Void... voids) {
             try {
                 mConfig = new ConfigXml(SyncthingService.this);
                 mConfig.updateIfNeeded();
-                return new Pair<>(mConfig.getWebGuiUrl(), mConfig.getApiKey());
             } catch (ConfigXml.OpenConfigException e) {
-                return null;
+                Toast.makeText(SyncthingService.this, R.string.config_create_failed,
+                        Toast.LENGTH_LONG).show();
+                onApiChange(State.ERROR);
+                cancel(true);
             }
+            return null;
         }
 
         @Override
-        protected void onPostExecute(Pair<URL, String> urlAndKey) {
+        protected void onPostExecute(Void aVoid) {
-            if (urlAndKey == null) {
+            mApi = new RestApi(SyncthingService.this, mConfig.getWebGuiUrl(), mConfig.getApiKey(),
-                Toast.makeText(SyncthingService.this, R.string.config_create_failed,
+                    SyncthingService.this::onSyncthingStarted, () -> onApiChange(mCurrentState));
-                        Toast.LENGTH_LONG).show();
-                mCurrentState = State.ERROR;
-                onApiChange();
-                return;
-            }
-
-            mApi = new RestApi(SyncthingService.this, urlAndKey.first, urlAndKey.second, () -> {
-                        mCurrentState = State.ACTIVE;
-                        onApiChange();
-                        new Thread(() -> {
-                            for (Folder r : mApi.getFolders()) {
-                                try {
-                                    mObservers.add(new FolderObserver(mApi, r));
-                                } catch (FolderObserver.FolderNotExistingException e) {
-                                    Log.w(TAG, "Failed to add observer for folder", e);
-                                } catch (StackOverflowError e) {
-                                    Log.w(TAG, "Failed to add folder observer", e);
-                                    Toast.makeText(SyncthingService.this,
-                                            R.string.toast_folder_observer_stack_overflow,
-                                            Toast.LENGTH_LONG)
-                                            .show();
-                                }
-                            }
-                        }).start();
-                    }, SyncthingService.this::onApiChange);
 
             mEventProcessor = new EventProcessor(SyncthingService.this, mApi);
 
-            registerOnWebGuiAvailableListener(mApi);
+            if (mApi != null)
-            registerOnWebGuiAvailableListener(mEventProcessor);
+                registerOnWebGuiAvailableListener(mApi);
+            if (mEventProcessor != null)
+                registerOnWebGuiAvailableListener(mEventProcessor);
             Log.i(TAG, "Web GUI will be available at " + mConfig.getWebGuiUrl());
-            updateState();
+
+            pollWebGui();
+            mSyncthingRunnable = new SyncthingRunnable(SyncthingService.this, SyncthingRunnable.Command.main);
+            new Thread(mSyncthingRunnable).start();
+            updateNotification();
         }
     }
 
+    private void onSyncthingStarted() {
+        onApiChange(State.ACTIVE);
+        new Thread(() -> {
+            for (Folder r : mApi.getFolders()) {
+                try {
+                    mObservers.add(new FolderObserver(mApi, r));
+                } catch (FolderObserver.FolderNotExistingException e) {
+                    Log.w(TAG, "Failed to add observer for folder", e);
+                } catch (StackOverflowError e) {
+                    Log.w(TAG, "Failed to add folder observer", e);
+                    Toast.makeText(SyncthingService.this,
+                            R.string.toast_folder_observer_stack_overflow,
+                            Toast.LENGTH_LONG)
+                            .show();
+                }
+            }
+        }).start();
+    }
+
     @Override
     public IBinder onBind(Intent intent) {
         return mBinder;
@@ -405,7 +388,7 @@ public class SyncthingService extends Service implements
 
             } else {
                 Log.i(TAG, "Shutting down service immediately");
-                shutdown();
+                shutdown(State.DISABLED, () -> {});
             }
         }
 
@@ -417,13 +400,17 @@ public class SyncthingService extends Service implements
             unregisterReceiver(mNetworkReceiver);
     }
 
-    private void shutdown() {
+    /**
+     * Stop Syncthing and all helpers like event processor, api handler and folder observers.
+     *
+     * Sets {@link #mCurrentState} to newState, and calls onKilledListener once Syncthing is killed.
+     */
+    private void shutdown(State newState, SyncthingRunnable.OnSyncthingKilled onKilledListener) {
+        onApiChange(newState);
+
         if (mEventProcessor != null)
             mEventProcessor.shutdown();
 
-        if (mRunnable != null)
-            mRunnable.killSyncthing();
-
         if (mApi != null)
             mApi.shutdown();
 
@@ -433,6 +420,13 @@ public class SyncthingService extends Service implements
 
         Stream.of(mObservers).forEach(FolderObserver::stopWatching);
         mObservers.clear();
+
+        if (mSyncthingRunnable != null) {
+            mSyncthingRunnable.killSyncthing(onKilledListener);
+            mSyncthingRunnable = null;
+        } else {
+            onKilledListener.onKilled();
+        }
     }
 
     /**
@@ -498,17 +492,14 @@ public class SyncthingService extends Service implements
                                     mConfig.getApiKey(), result -> {
             synchronized (stateLock) {
                 if (mStopScheduled) {
-                    mCurrentState = State.DISABLED;
+                    shutdown(State.DISABLED, () -> {});
-                    onApiChange();
-                    shutdown();
                     mStopScheduled = false;
                     stopSelf();
                     return;
                 }
             }
             Log.i(TAG, "Web GUI has come online at " + mConfig.getWebGuiUrl());
-            mCurrentState = State.STARTING;
+            onApiChange(State.STARTING);
-            onApiChange();
             Stream.of(mOnWebGuiAvailableListeners).forEach(OnWebGuiAvailableListener::onWebGuiAvailable);
             mOnWebGuiAvailableListeners.clear();
         });
@@ -519,7 +510,8 @@ public class SyncthingService extends Service implements
      *
      * Must only be called from SyncthingService or {@link RestApi} on the main thread.
      */
-    private void onApiChange() {
+    private void onApiChange(State newState) {
+        mCurrentState = newState;
         for (Iterator<OnApiChangeListener> i = mOnApiChangeListeners.iterator();
              i.hasNext(); ) {
             OnApiChangeListener listener = i.next();
@@ -566,24 +558,21 @@ public class SyncthingService extends Service implements
      * @return True if the import was successful, false otherwise (eg if files aren't found).
      */
     public boolean importConfig() {
-        mCurrentState = State.DISABLED;
-        shutdown();
         File config = new File(EXPORT_PATH, ConfigXml.CONFIG_FILE);
         File privateKey = new File(EXPORT_PATH, PRIVATE_KEY_FILE);
         File publicKey = new File(EXPORT_PATH, PUBLIC_KEY_FILE);
         if (!config.exists() || !privateKey.exists() || !publicKey.exists())
             return false;
-
+        shutdown(State.INIT, () -> {
-        try {
+            try {
-            Files.copy(config, new File(getFilesDir(), ConfigXml.CONFIG_FILE));
+                Files.copy(config, new File(getFilesDir(), ConfigXml.CONFIG_FILE));
-            Files.copy(privateKey, new File(getFilesDir(), PRIVATE_KEY_FILE));
+                Files.copy(privateKey, new File(getFilesDir(), PRIVATE_KEY_FILE));
-            Files.copy(publicKey, new File(getFilesDir(), PUBLIC_KEY_FILE));
+                Files.copy(publicKey, new File(getFilesDir(), PUBLIC_KEY_FILE));
-        } catch (IOException e) {
+            } catch (IOException e) {
-            Log.w(TAG, "Failed to import config", e);
+                Log.w(TAG, "Failed to import config", e);
-        }
+            }
-        mCurrentState = State.INIT;
+            new StartupTask().executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
-        onApiChange();
+        });
-        new StartupTask().executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
         return true;
     }
 }