#283 fix: Improvement of AppiumDriverLocalService for the multithreading.

src/main/java/io/appium/java_client/service/local/AppiumDriverLocalService.java

@@ -44,7 +44,7 @@ public final class AppiumDriverLocalService extends DriverService {
     private final String ipAddress;
     private final long startupTimeout;
     private final TimeUnit timeUnit;
-    private final ReentrantLock lock = new ReentrantLock();
+    private final ReentrantLock lock = new ReentrantLock(true); //uses "fair" thread ordering policy
     private final ListOutputStream stream = new ListOutputStream().add(System.out);
 
 
@@ -82,19 +82,21 @@ public URL getUrl() {
     @Override
     public boolean isRunning() {
         lock.lock();
-        if (process == null) {
-            return false;
-        }
+        try {
+            if (process == null) {
+                return false;
+            }
 
-        if (!process.isRunning()) {
-            return false;
-        }
+            if (!process.isRunning()) {
+                return false;
+            }
 
-        try {
-            ping(500, TimeUnit.MILLISECONDS);
-            return true;
-        } catch (UrlChecker.TimeoutException e) {
-            return false;
+            try {
+                ping(500, TimeUnit.MILLISECONDS);
+                return true;
+            } catch (UrlChecker.TimeoutException e) {
+                return false;
+            }
         } finally {
             lock.unlock();
         }
@@ -118,27 +120,30 @@ private void ping(long time, TimeUnit timeUnit) throws UrlChecker.TimeoutExcepti
      */
     public void start() throws AppiumServerHasNotBeenStartedLocallyException {
         lock.lock();
-        if (isRunning())
-            return;
-
         try {
-            process = new CommandLine(this.nodeJSExec.getCanonicalPath(), nodeJSArgs.toArray(new String[] {}));
-            process.setEnvironmentVariables(nodeJSEnvironment);
-            process.copyOutputTo(stream);
-            process.executeAsync();
-            ping(startupTimeout, timeUnit);
-        } catch (Throwable e) {
-            destroyProcess();
-            String msgTxt = "The local appium server has not been started. " +
-                    "The given Node.js executable: " + this.nodeJSExec.getAbsolutePath() + " Arguments: " + nodeJSArgs.toString() + " " + "\n";
-            if (process != null) {
-                String processStream = process.getStdOut();
-                if (!StringUtils.isBlank(processStream))
-                    msgTxt = msgTxt + "Process output: " + processStream + "\n";
+            if (isRunning()) {
+                return;
             }
 
-            throw new AppiumServerHasNotBeenStartedLocallyException(msgTxt,
-                    e);
+            try {
+                process = new CommandLine(this.nodeJSExec.getCanonicalPath(), nodeJSArgs.toArray(new String[]{}));
+                process.setEnvironmentVariables(nodeJSEnvironment);
+                process.copyOutputTo(stream);
+                process.executeAsync();
+                ping(startupTimeout, timeUnit);
+            } catch (Throwable e) {
+                destroyProcess();
+                String msgTxt = "The local appium server has not been started. " +
+                        "The given Node.js executable: " + this.nodeJSExec.getAbsolutePath() + " Arguments: " + nodeJSArgs.toString() + " " + "\n";
+                if (process != null) {
+                    String processStream = process.getStdOut();
+                    if (!StringUtils.isBlank(processStream))
+                        msgTxt = msgTxt + "Process output: " + processStream + "\n";
+                }
+
+                throw new AppiumServerHasNotBeenStartedLocallyException(msgTxt,
+                        e);
+            }
         } finally {
             lock.unlock();
         }
@@ -153,14 +158,22 @@ public void start() throws AppiumServerHasNotBeenStartedLocallyException {
     @Override
     public void stop() {
         lock.lock();
-        destroyProcess();
-        lock.unlock();
+        try {
+            if (process != null) {
+                destroyProcess();
+            }
+            process = null;
+        }
+        finally {
+            lock.unlock();
+        }
     }
 
 
     private void destroyProcess(){
-        if (process != null)
+        if (process.isRunning()) {
             process.destroy();
+        }
     }
 
     /**

src/test/java/io/appium/java_client/localserver/ServerBuilderTest.java

@@ -179,4 +179,35 @@ public void checkAbilityToChangeOutputStreamAfterTheServiceIsStarted() throws Ex
                 file.delete();
         }
     }
+
+    @Test
+    public void checkAbilityToShutDownService() {
+        AppiumDriverLocalService service = AppiumDriverLocalService.buildDefaultService();
+        service.start();
+        service.stop();
+        assertTrue(!service.isRunning());
+    }
+
+    @Test
+    public void checkAbilityToStartAndShutDownFewServices() throws Exception{
+        AppiumDriverLocalService service1 = new AppiumServiceBuilder().usingAnyFreePort().build();
+        AppiumDriverLocalService service2 = new AppiumServiceBuilder().usingAnyFreePort().build();
+        AppiumDriverLocalService service3 = new AppiumServiceBuilder().usingAnyFreePort().build();
+        AppiumDriverLocalService service4 = new AppiumServiceBuilder().usingAnyFreePort().build();
+        service1.start();
+        service2.start();
+        service3.start();
+        service4.start();
+        service1.stop();
+        Thread.sleep(1000);
+        service2.stop();
+        Thread.sleep(1000);
+        service3.stop();
+        Thread.sleep(1000);
+        service4.stop();
+        assertTrue(!service1.isRunning());
+        assertTrue(!service2.isRunning());
+        assertTrue(!service3.isRunning());
+        assertTrue(!service4.isRunning());
+    }
 }

src/test/java/io/appium/java_client/localserver/ThreadSafetyTest.java

@@ -0,0 +1,240 @@
+package io.appium.java_client.localserver;
+
+
+import io.appium.java_client.service.local.AppiumDriverLocalService;
+import org.junit.Test;
+
+import static org.junit.Assert.assertTrue;
+
+public class ThreadSafetyTest {
+
+    private static abstract class Action implements Cloneable {
+        abstract Object perform();
+
+        public Action clone() {
+            try {
+                return (Action) super.clone();
+            }
+            catch (Throwable t) {
+                throw new RuntimeException(t);
+            }
+        }
+    }
+
+    private static class TestThread implements Runnable {
+        private final Action action;
+        private Object result;
+        private Throwable t;
+
+        TestThread(Action action) {
+            this.action = action;
+        }
+
+        @Override
+        public void run() {
+            try {
+                result = action.perform();
+            }
+            catch (Throwable t) {
+                this.t = t;
+            }
+
+        }
+    }
+
+    final AppiumDriverLocalService service = AppiumDriverLocalService.buildDefaultService();
+    final Action run = new Action() {
+        @Override
+        Object perform() {
+            service.start();
+            return "OK";
+        }
+    };
+    final Action run2 = run.clone();
+
+    final Action isRunning = new Action() {
+        @Override
+        Object perform() {
+            return service.isRunning();
+        }
+    };
+    final Action isRunning2 = isRunning.clone();
+
+    final Action stop = new Action() {
+        @Override
+        Object perform() {
+            service.stop();
+            return "OK";
+        }
+    };
+
+    final Action stop2 = stop.clone();
+
+    @Test
+    public void whenFewTreadsDoTheSameWork() throws Throwable {
+
+        TestThread runTestThread = new TestThread(run);
+        TestThread runTestThread2 = new TestThread(run2);
+
+        TestThread isRunningTestThread = new TestThread(isRunning);
+        TestThread isRunningTestThread2 = new TestThread(isRunning2);
+
+        TestThread stopTestThread = new TestThread(stop);
+        TestThread stopTestThread2 = new TestThread(stop2);
+
+        Thread runThread = new Thread(runTestThread);
+        Thread runThread2 = new Thread(runTestThread2);
+
+        Thread isRunningThread = new Thread(isRunningTestThread);
+        Thread isRunningThread2 = new Thread(isRunningTestThread2);
+
+        Thread stopThread = new Thread(stopTestThread);
+        Thread stopThread2 = new Thread(stopTestThread2);
+
+        try {
+            runThread.start();
+            runThread2.start();
+
+            while (runThread.isAlive() || runThread2.isAlive()) {
+                //do nothing
+            }
+
+            if (runTestThread.t != null) {
+                throw runTestThread.t;
+            }
+
+            if (runTestThread2.t != null) {
+                throw runTestThread2.t;
+            }
+
+            assertTrue(runTestThread.result.equals("OK"));
+            assertTrue(runTestThread2.result.equals("OK"));
+            assertTrue(service.isRunning());
+
+            isRunningThread.start();
+            isRunningThread2.start();
+
+            while (isRunningThread.isAlive() || isRunningThread2.isAlive()) {
+                //do nothing
+            }
+
+            if (isRunningTestThread.t != null) {
+                throw isRunningTestThread.t;
+            }
+
+            if (isRunningTestThread2.t != null) {
+                throw isRunningTestThread2.t;
+            }
+
+            assertTrue(isRunningTestThread.result.equals(true));
+            assertTrue(isRunningTestThread2.result.equals(true));
+
+            stopThread.start();
+            stopThread2.start();
+
+            while (stopThread.isAlive() || stopThread2.isAlive()) {
+                //do nothing
+            }
+
+            if (stopTestThread.t != null) {
+                throw stopTestThread.t;
+            }
+
+            if (stopTestThread2.t != null) {
+                throw stopTestThread2.t;
+            }
+
+            assertTrue(stopTestThread.result.equals("OK"));
+            assertTrue(stopTestThread2.result.equals("OK"));
+            assertTrue(!service.isRunning());
+        }
+        finally {
+            if (service.isRunning()) {
+                service.stop();
+            }
+        }
+
+    }
+
+    @Test
+    public void whenFewTreadsDoDifferentWork() throws Throwable {
+        TestThread runTestThread = new TestThread(run);
+        TestThread runTestThread2 = new TestThread(run2);
+
+        TestThread isRunningTestThread = new TestThread(isRunning);
+        TestThread isRunningTestThread2 = new TestThread(isRunning2);
+
+        TestThread stopTestThread = new TestThread(stop);
+        TestThread stopTestThread2 = new TestThread(stop2);
+
+        Thread runThread = new Thread(runTestThread);
+        Thread runThread2 = new Thread(runTestThread2);
+
+        Thread isRunningThread = new Thread(isRunningTestThread);
+        Thread isRunningThread2 = new Thread(isRunningTestThread2);
+
+        Thread stopThread = new Thread(stopTestThread);
+        Thread stopThread2 = new Thread(stopTestThread2);
+
+        try {
+            runThread.start(); //(1)
+            Thread.sleep(10);
+            isRunningThread.start();//(2)
+            Thread.sleep(10);
+            stopThread.start(); //(3)
+
+            while (runThread.isAlive() || isRunningThread.isAlive() || stopThread.isAlive()) {
+                //do nothing
+            }
+
+            if (runTestThread.t != null) {
+                throw runTestThread.t;
+            }
+
+            if (isRunningTestThread.t != null) {
+                throw isRunningTestThread.t;
+            }
+
+            if (stopTestThread.t != null) {
+                throw stopTestThread.t;
+            }
+
+            assertTrue(runTestThread.result.equals("OK")); //the service had been started firstly (see (1))
+            assertTrue(isRunningTestThread.result.equals(true)); //it was running (see (2))
+            assertTrue(stopTestThread.result.equals("OK")); //and then the test tried to shut down it (see (3))
+            assertTrue(!service.isRunning());
+
+            isRunningThread2.start(); // (1)
+            Thread.sleep(10);
+            stopThread2.start(); // (2)
+            Thread.sleep(10);
+            runThread2.start(); //(3)
+
+            while (runThread2.isAlive() || isRunningThread2.isAlive() || stopThread2.isAlive()) {
+                //do nothing
+            }
+
+            if (runTestThread2.t != null) {
+                throw runTestThread.t;
+            }
+
+            if (isRunningTestThread2.t != null) {
+                throw isRunningTestThread.t;
+            }
+
+            if (stopTestThread2.t != null) {
+                throw stopTestThread.t;
+            }
+
+            assertTrue(isRunningTestThread2.result.equals(false)); //the service wasn't being running (see (1))
+            assertTrue(stopTestThread2.result.equals("OK")); //the service had not been started firstly (see (2)), it is ok
+            assertTrue(runTestThread2.result.equals("OK")); //and then it was started (see (3))
+            assertTrue(service.isRunning());
+        }
+        finally {
+            if (service.isRunning()) {
+                service.stop();
+            }
+        }
+    }
+}