Fail when a command returns a code != 0

RuntimeExecutor now throws an exception and log the command output (standard and error output).
jcgay · Aug 16, 2017 · 76e7f4f · 76e7f4f
1 parent 6b1faaf
commit 76e7f4f
Show file tree

Hide file tree

Showing 16 changed files with 149 additions and 127 deletions.
diff --git a/send-notification/src/main/java/fr/jcgay/notification/NotifierProvider.java b/send-notification/src/main/java/fr/jcgay/notification/NotifierProvider.java
@@ -57,7 +57,6 @@ class NotifierProvider {
     private static final ChosenNotifiers BURNT_TOAST = ChosenNotifiers.from("burnttoast");
     private static final ChosenNotifiers SLACK = ChosenNotifiers.from("slack");
 
-    private final RuntimeExecutor executor = new RuntimeExecutor();
     private final OperatingSystem os;
 
     public NotifierProvider(OperatingSystem os) {
@@ -82,10 +81,10 @@ public DiscoverableNotifier byName(ChosenNotifiers notifier, Properties properti
             return new GrowlNotifier(application, GrowlConfiguration.create(properties), ERROR);
         }
         if (NOTIFICATION_CENTER.equals(notifier)) {
-            return new TerminalNotifier(application, TerminalNotifierConfiguration.create(properties), executor);
+            return new TerminalNotifier(application, TerminalNotifierConfiguration.create(properties), new RuntimeExecutor());
         }
         if (NOTIFY_SEND.equals(notifier)) {
-            return new NotifySendNotifier(application, NotifySendConfiguration.create(properties), executor);
+            return new NotifySendNotifier(application, NotifySendConfiguration.create(properties), new RuntimeExecutor());
         }
         if (PUSHBULLET.equals(notifier)) {
             return new PushbulletNotifier(application, PushbulletConfiguration.create(properties));
@@ -97,19 +96,19 @@ public DiscoverableNotifier byName(ChosenNotifiers notifier, Properties properti
             return new SystemTrayNotifier(application);
         }
         if (NOTIFU.equals(notifier)) {
-            return new NotifuNotifier(application, NotifuConfiguration.create(properties), executor);
+            return new NotifuNotifier(application, NotifuConfiguration.create(properties), new RuntimeExecutor());
         }
         if (KDIALOG.equals(notifier)) {
-            return new KdialogNotifier(application, KdialogConfiguration.create(properties), executor);
+            return new KdialogNotifier(application, KdialogConfiguration.create(properties), new RuntimeExecutor());
         }
         if (ANYBAR.equals(notifier)) {
             return AnyBarNotifier.create(application, AnyBarConfiguration.create(properties));
         }
         if (SIMPLE_NOTIFICATION_CENTER.equals(notifier)) {
-            return new SimpleNotificationCenterNotifier(TerminalNotifierConfiguration.create(properties), executor);
+            return new SimpleNotificationCenterNotifier(TerminalNotifierConfiguration.create(properties), new RuntimeExecutor());
         }
         if (TOASTER.equals(notifier)) {
-            return new ToasterNotifier(ToasterConfiguration.create(properties), executor);
+            return new ToasterNotifier(ToasterConfiguration.create(properties), new RuntimeExecutor());
         }
         if (NOTIFY.equals(notifier)) {
             return new NotifyNotifier(application, NotifyConfiguration.create(properties));

diff --git a/send-notification/src/main/java/fr/jcgay/notification/notifier/executor/Executor.java b/send-notification/src/main/java/fr/jcgay/notification/notifier/executor/Executor.java
@@ -1,6 +1,7 @@
 package fr.jcgay.notification.notifier.executor;
 
 public interface Executor {
-    Process exec(String[] command);
+    void exec(String[] command);
     boolean tryExec(String[] command);
+    void close();
 }
diff --git a/send-notification/src/main/java/fr/jcgay/notification/notifier/executor/RuntimeExecutor.java b/send-notification/src/main/java/fr/jcgay/notification/notifier/executor/RuntimeExecutor.java
@@ -2,39 +2,109 @@
 
 import com.google.common.base.Joiner;
 import com.google.common.base.Throwables;
+import fr.jcgay.notification.SendNotificationException;
 import org.slf4j.Logger;
 
+import java.io.BufferedReader;
 import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.TimeoutException;
 
+import static com.google.common.io.Closeables.closeQuietly;
+import static java.util.concurrent.TimeUnit.MILLISECONDS;
 import static org.slf4j.LoggerFactory.getLogger;
 
 public class RuntimeExecutor implements Executor {
 
     private static final Logger LOGGER = getLogger(RuntimeExecutor.class);
 
+    private final ExecutorService executor = Executors.newSingleThreadExecutor();
+
     @Override
-    public Process exec(String[] command) {
+    public void exec(final String[] command) {
 
         if (LOGGER.isDebugEnabled()) {
-            LOGGER.debug("Will execute command line: " + Joiner.on(" ").join(command));
+            LOGGER.debug("Will execute command line: {}", logCommand(command));
         }
 
+        Future<Integer> task = executor.submit(new Callable<Integer>() {
+            @Override
+            public Integer call() {
+                try {
+                    Process execution = new ProcessBuilder(command)
+                        .redirectErrorStream(true)
+                        .start();
+
+                    int returnCode = execution.waitFor();
+                    if (returnCode != 0) {
+                        String message = "Command <[" + logCommand(command) + "]> returns code: " + returnCode + ".\n" + asString(execution.getInputStream());
+                        LOGGER.debug(message);
+                        throw new SendNotificationException(message);
+                    }
+
+                    LOGGER.debug("Command <[{}]> ends successfully.", logCommand(command));
+
+                    return returnCode;
+
+                } catch (IOException e) {
+                    throw Throwables.propagate(e);
+                } catch (InterruptedException e) {
+                    Thread.currentThread().interrupt();
+                    throw Throwables.propagate(e);
+                }
+            }
+        });
+
         try {
-            return Runtime.getRuntime().exec(command);
-        } catch (IOException e) {
+            task.get(200, MILLISECONDS);
+        } catch (InterruptedException e) {
+            Thread.currentThread().interrupt();
             throw Throwables.propagate(e);
+        } catch (ExecutionException e) {
+            LOGGER.debug("An error occurs while running command: <[{}]>", logCommand(command), e);
+            throw Throwables.propagate(e.getCause());
+        } catch (TimeoutException e) {
+            LOGGER.debug("Command <[{}]> takes too long to execute. Do not wait for the result...", logCommand(command), e);
         }
     }
 
     @Override
     public boolean tryExec(String[] command) {
         try {
-            return exec(command).waitFor() == 0;
-        } catch (InterruptedException e) {
-            Thread.currentThread().interrupt();
-            return false;
+            exec(command);
+            return true;
         } catch (RuntimeException e) {
             return false;
         }
     }
+
+    private static String logCommand(String[] command) {
+        return Joiner.on(" ").join(command);
+    }
+
+    private static String asString(InputStream inputStream) throws IOException {
+        BufferedReader reader = null;
+        try {
+            reader = new BufferedReader(new InputStreamReader(inputStream));
+            StringBuilder output = new StringBuilder();
+            String line;
+            while ((line = reader.readLine()) != null) {
+                output.append(line).append("\n");
+            }
+            return output.toString();
+        } finally {
+            closeQuietly(reader);
+        }
+    }
+
+    @Override
+    public void close() {
+        executor.shutdownNow();
+    }
 }
diff --git a/send-notification/src/main/java/fr/jcgay/notification/notifier/kdialog/KdialogNotifier.java b/send-notification/src/main/java/fr/jcgay/notification/notifier/kdialog/KdialogNotifier.java
@@ -58,7 +58,7 @@ public void send(Notification notification) {
 
     @Override
     public void close() {
-        // do nothing
+        executor.close();
     }
 
     @Override

diff --git a/...a/fr/jcgay/notification/notifier/notificationcenter/SimpleNotificationCenterNotifier.java b/...a/fr/jcgay/notification/notifier/notificationcenter/SimpleNotificationCenterNotifier.java
@@ -71,7 +71,7 @@ private String buildAppleScript(Notification notification) {
 
     @Override
     public void close() {
-        // do nothing
+        executor.close();
     }
 
     @Override

diff --git a/...ion/src/main/java/fr/jcgay/notification/notifier/notificationcenter/TerminalNotifier.java b/...ion/src/main/java/fr/jcgay/notification/notifier/notificationcenter/TerminalNotifier.java
@@ -77,7 +77,7 @@ public void send(Notification notification) {
 
     @Override
     public void close() {
-        // do nothing
+        executor.close();
     }
 
     @Override

diff --git a/send-notification/src/main/java/fr/jcgay/notification/notifier/notifu/NotifuNotifier.java b/send-notification/src/main/java/fr/jcgay/notification/notifier/notifu/NotifuNotifier.java
@@ -77,7 +77,7 @@ private static String toType(Notification.Level level) {
 
     @Override
     public void close() {
-        // do nothing
+        executor.close();
     }
 
     @Override

diff --git a/...ification/src/main/java/fr/jcgay/notification/notifier/notifysend/NotifySendNotifier.java b/...ification/src/main/java/fr/jcgay/notification/notifier/notifysend/NotifySendNotifier.java
@@ -61,7 +61,7 @@ public void send(Notification notification) {
 
     @Override
     public void close() {
-        // do nothing
+        executor.close();
     }
 
     @Override

diff --git a/send-notification/src/main/java/fr/jcgay/notification/notifier/toaster/ToasterNotifier.java b/send-notification/src/main/java/fr/jcgay/notification/notifier/toaster/ToasterNotifier.java
@@ -51,7 +51,7 @@ public void send(Notification notification) {
 
     @Override
     public void close() {
-        // do nothing
+        executor.close();
     }
 
     @Override

diff --git a/send-notification/src/test/groovy/fr/jcgay/notification/executor/FakeExecutor.groovy b/send-notification/src/test/groovy/fr/jcgay/notification/executor/FakeExecutor.groovy
@@ -0,0 +1,22 @@
+package fr.jcgay.notification.executor
+
+import fr.jcgay.notification.notifier.executor.Executor
+
+
+class FakeExecutor implements Executor {
+
+    List<String> executedCommand = []
+
+    @Override
+    void exec(String[] command) {
+        executedCommand = command
+    }
+
+    @Override
+    boolean tryExec(String[] command) {
+        throw new IllegalStateException("This method should not be called!")
+    }
+
+    @Override
+    void close() {}
+}
diff --git a/...ication/src/test/groovy/fr/jcgay/notification/notifier/kdialog/KdialogNotifierSpec.groovy b/...ication/src/test/groovy/fr/jcgay/notification/notifier/kdialog/KdialogNotifierSpec.groovy
@@ -1,8 +1,9 @@
 package fr.jcgay.notification.notifier.kdialog
+
 import fr.jcgay.notification.Application
 import fr.jcgay.notification.Notification
 import fr.jcgay.notification.TestIcon
-import fr.jcgay.notification.notifier.executor.Executor
+import fr.jcgay.notification.executor.FakeExecutor
 import fr.jcgay.notification.notifier.executor.RuntimeExecutor
 import spock.lang.Specification
 
@@ -13,19 +14,7 @@ class KdialogNotifierSpec extends Specification {
     Notification notification = Notification.builder('title', 'message', TestIcon.ok()).build()
     Application application = Application.builder('id', 'name', TestIcon.ok()).build()
 
-    List<String> executedCommand
-    Executor executor = new Executor() {
-        @Override
-        Process exec(String[] command) {
-            executedCommand = command
-            null
-        }
-
-        @Override
-        boolean tryExec(String[] command) {
-            throw new IllegalStateException("This method should not be called!")
-        }
-    }
+    FakeExecutor executor = new FakeExecutor()
 
     def "should build command line to call kdialog"() {
         given:
@@ -36,7 +25,7 @@ class KdialogNotifierSpec extends Specification {
         notifier.send(notification)
 
         then:
-        executedCommand == [
+        executor.executedCommand == [
                 'kdialog',
                 '--passivepopup', notification.message(), '3',
                 '--title', notification.title(),
@@ -52,7 +41,7 @@ class KdialogNotifierSpec extends Specification {
         notifier.send(notification)
 
         then:
-        executedCommand == [
+        executor.executedCommand == [
                 'kdialog',
                 '--passivepopup', notification.message(),
                 '--title', notification.title(),

diff --git a/...ovy/fr/jcgay/notification/notifier/notificationcenter/SimpleNotificationCenterSpec.groovy b/...ovy/fr/jcgay/notification/notifier/notificationcenter/SimpleNotificationCenterSpec.groovy
@@ -2,25 +2,13 @@ package fr.jcgay.notification.notifier.notificationcenter
 
 import fr.jcgay.notification.Notification
 import fr.jcgay.notification.TestIcon
-import fr.jcgay.notification.notifier.executor.Executor
+import fr.jcgay.notification.executor.FakeExecutor
 import spock.lang.Specification
 import spock.lang.Subject
 
 class SimpleNotificationCenterSpec extends Specification {
 
-    List<String> executedCommand
-    Executor executor = new Executor() {
-        @Override
-        Process exec(String[] command) {
-            executedCommand = command
-            null
-        }
-
-        @Override
-        boolean tryExec(String[] command) {
-            throw new IllegalStateException("This method should not be called!")
-        }
-    }
+    FakeExecutor executor = new FakeExecutor()
 
     @Subject
     SimpleNotificationCenterNotifier notifier
@@ -44,7 +32,7 @@ class SimpleNotificationCenterSpec extends Specification {
         notifier.send(notification)
 
         then:
-        executedCommand == [
+        executor.executedCommand == [
                 'osascript',
                 '-e',
                 /display notification "${notification.message()}" sound name "default" with title "${notification.title()}" subtitle "${notification.subtitle()}"/
@@ -59,7 +47,7 @@ class SimpleNotificationCenterSpec extends Specification {
         notifier.send(notification)
 
         then:
-        !executedCommand.any { it.contains('subtitle') }
+        !executor.executedCommand.any { it.contains('subtitle') }
     }
 
     def "should not set sound when configuration does not includes one"() {
@@ -71,7 +59,7 @@ class SimpleNotificationCenterSpec extends Specification {
         notifier.send(notification)
 
         then:
-        !executedCommand.any { it.contains('sound name') }
-        !executedCommand.any { it.contains('default') }
+        !executor.executedCommand.any { it.contains('sound name') }
+        !executor.executedCommand.any { it.contains('default') }
     }
 }