changeset 1064:653f57559b42

Make launcher thread safe Reviewed-by: vanaltj Review-thread: http://icedtea.classpath.org/pipermail/thermostat/2013-April/006306.html
author Omair Majid <omajid@redhat.com>
date Mon, 08 Apr 2013 16:40:39 -0400
parents c1edec7797bd
children ef9be67db1ad
files agent/cli/src/main/java/com/redhat/thermostat/agent/cli/impl/ServiceCommand.java client/cli/src/main/java/com/redhat/thermostat/client/cli/internal/ShellCommand.java client/cli/src/test/java/com/redhat/thermostat/client/cli/internal/ShellCommandTest.java common/core/src/main/java/com/redhat/thermostat/common/Launcher.java launcher/src/main/java/com/redhat/thermostat/launcher/internal/LauncherImpl.java launcher/src/test/java/com/redhat/thermostat/launcher/internal/LauncherImplTest.java main/pom.xml main/src/main/java/com/redhat/thermostat/main/Thermostat.java main/src/main/java/com/redhat/thermostat/main/impl/Activator.java main/src/main/java/com/redhat/thermostat/main/impl/FrameworkProvider.java
diffstat 10 files changed, 92 insertions(+), 200 deletions(-) [+]
line wrap: on
line diff
--- a/agent/cli/src/main/java/com/redhat/thermostat/agent/cli/impl/ServiceCommand.java	Fri Apr 05 15:27:57 2013 -0400
+++ b/agent/cli/src/main/java/com/redhat/thermostat/agent/cli/impl/ServiceCommand.java	Mon Apr 08 16:40:39 2013 -0400
@@ -85,13 +85,11 @@
         }
         launcher = (Launcher) context.getService(launcherRef);
         String[] storageStartArgs = new String[] { "storage", "--start" };
-        launcher.setArgs(storageStartArgs);
-        launcher.run(listeners, false);
+        launcher.run(storageStartArgs, listeners, false);
         agentBarrier.acquireUninterruptibly();
         
         String[] storageStopArgs = new String[] { "storage", "--stop" };
-        launcher.setArgs(storageStopArgs);
-        launcher.run(false);
+        launcher.run(storageStopArgs, false);
         
         context.ungetService(launcherRef);
     }
@@ -109,8 +107,7 @@
                 String dbUrl = storage.getConfiguration().getDBConnectionString();
                 String[] agentArgs =  new String[] {"agent", "-d", dbUrl};
                 System.err.println(translator.localize(LocaleResources.STARTING_AGENT));
-                launcher.setArgs(agentArgs);
-                launcher.run(false);
+                launcher.run(agentArgs, false);
                 agentBarrier.release();
                 break;
             case FAIL:
--- a/client/cli/src/main/java/com/redhat/thermostat/client/cli/internal/ShellCommand.java	Fri Apr 05 15:27:57 2013 -0400
+++ b/client/cli/src/main/java/com/redhat/thermostat/client/cli/internal/ShellCommand.java	Mon Apr 08 16:40:39 2013 -0400
@@ -170,8 +170,7 @@
         ServiceReference launcherRef = bundleContext.getServiceReference(Launcher.class.getName());
         if (launcherRef != null) {
             Launcher launcher = (Launcher) bundleContext.getService(launcherRef);
-            launcher.setArgs(parsed);
-            launcher.run(true);
+            launcher.run(parsed, true);
         } else {
             throw new CommandException("Severe: Could not locate launcher");
         }
--- a/client/cli/src/test/java/com/redhat/thermostat/client/cli/internal/ShellCommandTest.java	Fri Apr 05 15:27:57 2013 -0400
+++ b/client/cli/src/test/java/com/redhat/thermostat/client/cli/internal/ShellCommandTest.java	Mon Apr 08 16:40:39 2013 -0400
@@ -108,8 +108,7 @@
         Arguments args = new SimpleArguments();
         CommandContext ctx = ctxFactory.createContext(args);
         cmd.run(ctx);
-        verify(launcher).setArgs(new String[]{"help"});
-        verify(launcher).run(true);
+        verify(launcher).run(new String[]{"help"}, true);
     }
 
     @Test
@@ -179,8 +178,7 @@
         assertEquals(VERSION_OUTPUT + "Thermostat > old-history-value\nThermostat > exit\n", ctxFactory.getOutput());
         assertEquals("", ctxFactory.getError());
 
-        verify(launcher).setArgs(new String[] {"old-history-value"});
-        verify(launcher).run(true);
+        verify(launcher).run(new String[] {"old-history-value"}, true);
     }
 
     @Test
@@ -199,8 +197,7 @@
         CommandContext ctx = ctxFactory.createContext(args);
         cmd.run(ctx);
 
-        verify(launcher).setArgs(new String[] {"add-to-history"});
-        verify(launcher).run(true);
+        verify(launcher).run(new String[] {"add-to-history"}, true);
         verify(mockHistory).add("add-to-history");
         verify(mockHistory).flush();
 
--- a/common/core/src/main/java/com/redhat/thermostat/common/Launcher.java	Fri Apr 05 15:27:57 2013 -0400
+++ b/common/core/src/main/java/com/redhat/thermostat/common/Launcher.java	Mon Apr 08 16:40:39 2013 -0400
@@ -51,25 +51,21 @@
     /**
      * Invoked in order to start a command, either when Thermostat starts, or within
      * the thermostat shell.  Equivalent to calling run(null, inShell).
+     * @param args the name of the command and any arguments to it
      * @param inShell whether invoked from within the thermostat shell
      */
-    void run(boolean inShell);
+    void run(String[] args, boolean inShell);
 
     /**
      * Invoked in order to start a command, either when Thermostat starts, or within
      * the thermostat shell.  If the command being run happens to be a AbstractStateNotifyingCommand,
      * and the argument is non-null, the listeners will be added to the command for
      * life cycle notifications.  Otherwise, the argument is ignored.
+     * @param args the name of the command and any arguments to it
      * @param listeners the collection of listeners to be added to the command
      * @param inShell whether invoked from within the thermostat shell
      */
-    void run(Collection<ActionListener<ApplicationState>> listeners, boolean inShell);
-
-    /**
-     * Should be set before calling run()
-     * @param command line arguments to the program
-     */
-    void setArgs(String[] args);
+    void run(String[] args, Collection<ActionListener<ApplicationState>> listeners, boolean inShell);
 
 }
 
--- a/launcher/src/main/java/com/redhat/thermostat/launcher/internal/LauncherImpl.java	Fri Apr 05 15:27:57 2013 -0400
+++ b/launcher/src/main/java/com/redhat/thermostat/launcher/internal/LauncherImpl.java	Mon Apr 08 16:40:39 2013 -0400
@@ -40,7 +40,7 @@
 import java.io.PrintStream;
 import java.util.Arrays;
 import java.util.Collection;
-import java.util.concurrent.Semaphore;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.logging.Level;
 
 import org.apache.commons.cli.Options;
@@ -75,24 +75,23 @@
 import com.redhat.thermostat.storage.core.StorageException;
 import com.redhat.thermostat.utils.keyring.Keyring;
 
+/**
+ * This class is thread-safe.
+ */
 public class LauncherImpl implements Launcher {
 
     private static final Translate<LocaleResources> t = LocaleResources.createLocalizer();
 
-    private ClientPreferences prefs;
-
-    private String[] args;
-
-    private CommandContextFactory cmdCtxFactory;
-
-    private int usageCount = 0;
-    private final Semaphore argsBarrier = new Semaphore(0);
-
+    private final AtomicInteger usageCount = new AtomicInteger(0);
     private final BundleContext context;
     private final BundleManager registry;
+    private final CommandContextFactory cmdCtxFactory;
     private final DbServiceFactory dbServiceFactory;
     private final Version coreVersion;
 
+    /** MUST be mutated in a 'synchronized (this)' block */
+    private ClientPreferences prefs;
+
     public LauncherImpl(BundleContext context, CommandContextFactory cmdCtxFactory, BundleManager registry) {
         this(context, cmdCtxFactory, registry, new LoggingInitializer(), new DbServiceFactory(), new Version());
     }
@@ -108,53 +107,32 @@
     }
 
     @Override
-    public synchronized void run(boolean inShell) {
-        run(null, inShell);
+    public void run(String[] args, boolean inShell) {
+        run(args, null, inShell);
     }
 
     @Override
-    public synchronized void run(Collection<ActionListener<ApplicationState>> listeners, boolean inShell) {
-
-        usageCount++;
-        waitForArgs();
-
+    public void run(String[] args, Collection<ActionListener<ApplicationState>> listeners, boolean inShell) {
+        usageCount.incrementAndGet();
         try {
-            if (hasNoArguments()) {
+            if (hasNoArguments(args)) {
                 runHelpCommand();
-            } else if (isVersionQuery(inShell)) {
+            } else if (isVersionQuery(args, inShell)) {
                 // We want to print the version of core
                 // thermostat, so we use the no-arg constructor of Version
                 cmdCtxFactory.getConsole().getOutput().println(coreVersion.getVersionInfo());
             } else {
-                runCommandFromArguments(listeners, inShell);
+                runCommandFromArguments(args, listeners, inShell);
             }
         } finally {
             args = null;
-            usageCount--;
-            if (isLastLaunch()) {
+            boolean isLastLaunch = (usageCount.decrementAndGet() == 0);
+            if (isLastLaunch) {
                 shutdown();
             }
         }
     }
 
-    @Override
-    public void setArgs(String[] args) {
-        this.args = args;
-        argsBarrier.release();
-    }
-
-    private void waitForArgs() {
-        try {
-            argsBarrier.acquire();
-        } catch (InterruptedException e) {
-            e.printStackTrace();
-        }
-    }
-
-    private boolean isLastLaunch() {
-        return usageCount == 0;
-    }
-
     @SuppressWarnings({ "unchecked", "rawtypes" })
     private void shutdown() throws InternalError {
         try {
@@ -190,12 +168,12 @@
         }
     }
 
-    public void setPreferences(ClientPreferences prefs) {
+    synchronized void setPreferences(ClientPreferences prefs) {
         this.prefs = prefs;
     }
 
-    private boolean hasNoArguments() {
-        return args.length == 0;
+    private boolean hasNoArguments(String[] args) {
+        return args == null || args.length == 0;
     }
 
     private void runHelpCommand() {
@@ -206,7 +184,7 @@
         runCommand("help", new String[] { "--", cmdName }, null, false);
     }
 
-    private void runCommandFromArguments(Collection<ActionListener<ApplicationState>> listeners, boolean inShell) {
+    private void runCommandFromArguments(String[] args, Collection<ActionListener<ApplicationState>> listeners, boolean inShell) {
         runCommand(args[0], Arrays.copyOfRange(args, 1, args.length), listeners, inShell);
     }
 
@@ -313,11 +291,13 @@
 
         CommandContext ctx = cmdCtxFactory.createContext(args);
         
-        if (prefs == null) {
-            ServiceReference keyringReference = context.getServiceReference(Keyring.class);
-            @SuppressWarnings("unchecked")
-            Keyring keyring = (Keyring) context.getService(keyringReference);
-            prefs = new ClientPreferences(keyring);
+        synchronized (this) {
+            if (prefs == null) {
+                ServiceReference keyringReference = context.getServiceReference(Keyring.class);
+                @SuppressWarnings("unchecked")
+                Keyring keyring = (Keyring) context.getService(keyringReference);
+                prefs = new ClientPreferences(keyring);
+            }
         }
         
         if (cmd.isStorageRequired()) {
@@ -346,7 +326,7 @@
         return ctx;
     }
 
-    private boolean isVersionQuery(boolean inShell) {
+    private boolean isVersionQuery(String[] args, boolean inShell) {
         // don't allow --version in the shell
         if (inShell) {
             return false;
--- a/launcher/src/test/java/com/redhat/thermostat/launcher/internal/LauncherImplTest.java	Fri Apr 05 15:27:57 2013 -0400
+++ b/launcher/src/test/java/com/redhat/thermostat/launcher/internal/LauncherImplTest.java	Mon Apr 08 16:40:39 2013 -0400
@@ -419,26 +419,24 @@
         });
         ctxFactory.getCommandRegistry().registerCommands(Arrays.asList(errorCmd));
 
-        launcher.setArgs(new String[] { "error" });
-        wrappedRun(launcher, false);
+        wrappedRun(launcher, new String[] { "error" }, false);
         assertEquals("test error\n", ctxFactory.getError());
 
     }
 
     private void runAndVerifyCommand(String[] args, String expected, boolean inShell) {
-        launcher.setArgs(args);
-        wrappedRun(launcher, inShell);
+        wrappedRun(launcher, args, inShell);
         assertEquals(expected, ctxFactory.getOutput());
         assertTrue(timerFactory.isShutdown());
     }
     
-    private void wrappedRun(LauncherImpl launcher, boolean inShell) {
-        wrappedRun(launcher, inShell, null);
+    private void wrappedRun(LauncherImpl launcher, String[] args, boolean inShell) {
+        wrappedRun(launcher, args, inShell, null);
     }
     
-    private void wrappedRun(LauncherImpl launcher, boolean inShell, Collection<ActionListener<ApplicationState>> listeners) {
+    private void wrappedRun(LauncherImpl launcher, String[] args, boolean inShell, Collection<ActionListener<ApplicationState>> listeners) {
         try {
-            launcher.run(listeners, inShell);
+            launcher.run(args, listeners, inShell);
         } catch (ExitException e) {
             System.out.println(e.getMessage());
         }
@@ -454,8 +452,7 @@
         ArgumentCaptor<String> dbUrlCaptor = ArgumentCaptor.forClass(String.class);
         when(dbServiceFactory.createDbService(anyString(), anyString(), dbUrlCaptor.capture())).thenReturn(dbService);
         launcher.setPreferences(prefs);
-        launcher.setArgs(new String[] { "test3" });
-        wrappedRun(launcher, false);
+        wrappedRun(launcher, new String[] { "test3" }, false);
         verify(dbService).connect();
         verify(prefs).getConnectionUrl();
         assertEquals(dbUrl, dbUrlCaptor.getValue());
@@ -476,8 +473,7 @@
         DbService dbService = mock(DbService.class);
         when(dbServiceFactory.createDbService(anyString(), anyString(), anyString())).thenReturn(dbService);
 
-        launcher.setArgs(new String[] { "dummy" });
-        wrappedRun(launcher, false);
+        wrappedRun(launcher, new String[] { "dummy" }, false);
         verify(dbService).connect();
     }
 
@@ -489,8 +485,7 @@
 
         when(version.getVersionInfo()).thenReturn(versionString);
 
-        launcher.setArgs(new String[] {Version.VERSION_OPTION});
-        wrappedRun(launcher, false);
+        wrappedRun(launcher, new String[] {Version.VERSION_OPTION}, false);
 
         assertEquals(expectedVersionInfo, ctxFactory.getOutput());
         assertTrue(timerFactory.isShutdown());
@@ -503,32 +498,28 @@
         listeners.add(listener);
         String[] args = new String[] {"basic"};
 
-        launcher.setArgs(args);
-        wrappedRun(launcher, false, listeners);
+        wrappedRun(launcher, args, false, listeners);
         verify(notifier).addActionListener(listener);
     }
 
     @Test
     public void verifyLoggingIsInitialized() {
-        launcher.setArgs(new String[] { "test1" });
-        wrappedRun(launcher, false);
+        wrappedRun(launcher, new String[] { "test1" }, false);
 
         verify(loggingInitializer).initialize();
     }
 
     @Test
     public void verifyShutdown() throws BundleException {
-        launcher.setArgs(new String[] { "test1" });
-        wrappedRun(launcher, false);
+        wrappedRun(launcher, new String[] { "test1" }, false);
 
         verify(sysBundle).stop();
     }
     
     @Test
     public void verifySetExitStatus() {
-        launcher.setArgs(new String[] { "test1" });
         try {
-            launcher.run(false);
+            launcher.run(new String[] { "test1" }, false);
             fail("Should have called System.exit()");
         } catch (ExitException e) {
             // pass, by default launcher exits with an exit status
--- a/main/pom.xml	Fri Apr 05 15:27:57 2013 -0400
+++ b/main/pom.xml	Mon Apr 08 16:40:39 2013 -0400
@@ -106,7 +106,6 @@
         <configuration>
           <instructions>
             <Bundle-Vendor>Red Hat, Inc.</Bundle-Vendor>
-            <Bundle-Activator>com.redhat.thermostat.main.impl.Activator</Bundle-Activator>
             <Bundle-SymbolicName>com.redhat.thermostat.main</Bundle-SymbolicName>
             <Export-Package>
               com.redhat.thermostat.main,
--- a/main/src/main/java/com/redhat/thermostat/main/Thermostat.java	Fri Apr 05 15:27:57 2013 -0400
+++ b/main/src/main/java/com/redhat/thermostat/main/Thermostat.java	Mon Apr 08 16:40:39 2013 -0400
@@ -52,38 +52,10 @@
 import com.redhat.thermostat.common.config.Configuration;
 import com.redhat.thermostat.main.impl.FrameworkProvider;
 
-public class Thermostat implements Runnable {
+public class Thermostat {
 
     private static Configuration config;
 
-    private BundleContext context;
-
-    public Thermostat(BundleContext context) {
-        this.context = context;
-    }
-
-    @SuppressWarnings({ "rawtypes", "unchecked" })
-    private void launch()
-            throws NoSuchMethodException, IllegalAccessException, InvocationTargetException,
-            FileNotFoundException, IOException, BundleException, InterruptedException {
-        Launcher launcher = null;
-        ServiceTracker tracker = new ServiceTracker(context, Launcher.class.getName(), null);
-        tracker.open();
-        launcher = (Launcher) tracker.waitForService(0);
-        launcher.run(false);
-        tracker.close();
-    }
-
-    @Override
-    public void run() {
-        try {
-            launch();
-        } catch (IOException | NoSuchMethodException | InterruptedException |
-                IllegalAccessException | InvocationTargetException | BundleException e) {
-            e.printStackTrace();
-        }
-    }
-
     /**
      * @param args
      * @throws Exception
@@ -103,7 +75,7 @@
         }
 
         FrameworkProvider frameworkProvider = new FrameworkProvider(config);
-        frameworkProvider.startFramework(toProcess.toArray(new String[0]));
+        frameworkProvider.start(toProcess.toArray(new String[0]));
 
     }
 
--- a/main/src/main/java/com/redhat/thermostat/main/impl/Activator.java	Fri Apr 05 15:27:57 2013 -0400
+++ /dev/null	Thu Jan 01 00:00:00 1970 +0000
@@ -1,67 +0,0 @@
-/*
- * Copyright 2012, 2013 Red Hat, Inc.
- *
- * This file is part of Thermostat.
- *
- * Thermostat is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published
- * by the Free Software Foundation; either version 2, or (at your
- * option) any later version.
- *
- * Thermostat is distributed in the hope that it will be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with Thermostat; see the file COPYING.  If not see
- * <http://www.gnu.org/licenses/>.
- *
- * Linking this code with other modules is making a combined work
- * based on this code.  Thus, the terms and conditions of the GNU
- * General Public License cover the whole combination.
- *
- * As a special exception, the copyright holders of this code give
- * you permission to link this code with independent modules to
- * produce an executable, regardless of the license terms of these
- * independent modules, and to copy and distribute the resulting
- * executable under terms of your choice, provided that you also
- * meet, for each linked independent module, the terms and conditions
- * of the license of that module.  An independent module is a module
- * which is not derived from or based on this code.  If you modify
- * this code, you may extend this exception to your version of the
- * library, but you are not obligated to do so.  If you do not wish
- * to do so, delete this exception statement from your version.
- */
-
-package com.redhat.thermostat.main.impl;
-
-import java.util.concurrent.ExecutorService;
-import java.util.concurrent.Executors;
-
-import com.redhat.thermostat.main.Thermostat;
-
-import org.osgi.framework.BundleActivator;
-import org.osgi.framework.BundleContext;
-
-public class Activator implements BundleActivator {
-
-    ExecutorService e;
-
-    public Activator() {
-        e = Executors.newSingleThreadExecutor();
-    }
-
-    @Override
-    public void start(BundleContext context) throws Exception {
-        Thermostat thermostat = new Thermostat(context);
-        e.execute(thermostat);
-    }
-
-    @Override
-    public void stop(BundleContext context) throws Exception {
-        e.shutdownNow();
-    }
-
-}
-
--- a/main/src/main/java/com/redhat/thermostat/main/impl/FrameworkProvider.java	Fri Apr 05 15:27:57 2013 -0400
+++ b/main/src/main/java/com/redhat/thermostat/main/impl/FrameworkProvider.java	Mon Apr 08 16:40:39 2013 -0400
@@ -82,13 +82,13 @@
     // This is our ticket into OSGi land. Unfortunately, we to use a bit of reflection here.
     // The launcher and bundleloader are instantiated from within their bundles, ie loaded
     // by the bundle classloader.
-    public void startFramework(String[] args) {
+    public void start(String[] args) {
         try {
             Framework framework = makeFramework();
             prepareFramework(framework);
             loadBootstrapBundles(framework);
             setLoaderVerbosity(framework);
-            setLaunchArgs(framework, args);
+            runLauncher(framework, args);
         } catch (InterruptedException | BundleException | IOException e) {
             throw new RuntimeException("Could not start framework.", e);
         }
@@ -220,12 +220,12 @@
 
     private void setLoaderVerbosity(Framework framework) throws InterruptedException {
         Object loader = getService(framework, BundleManager.class.getName());
-        callVoidReflectedMethod(loader, "setPrintOSGiInfo", printOSGiInfo, Boolean.TYPE);
+        callVoidReflectedMethod(loader, "setPrintOSGiInfo", printOSGiInfo);
     }
 
-    private void setLaunchArgs(Framework framework, String[] args) throws InterruptedException {
+    private void runLauncher(Framework framework, String[] args) throws InterruptedException {
         Object launcher = getService(framework, Launcher.class.getName());
-        callVoidReflectedMethod(launcher, "setArgs", args, String[].class);
+        callVoidReflectedMethod(launcher, "run", args, false);
     }
 
     private Object getService(Framework framework, String name) throws InterruptedException {
@@ -238,11 +238,21 @@
         return service;
     }
 
-    private void callVoidReflectedMethod(Object object, String name, Object arguments, Class<?> argsClass) {
+    /**
+     * Call {@code object}.{@code name} with {@code args} as the arguments. The
+     * return value is ignored. The types of the method arguments must exactly
+     * match the types of the supplied arguments, but primitives are used unboxed.
+     */
+    private void callVoidReflectedMethod(Object object, String name, Object... args) {
         Class<?> clazz = object.getClass();
+        Class<?>[] classes = new Class<?>[args.length];
+        for (int i = 0; i < args.length; i++) {
+            classes[i] = preferPrimitiveClass(args[i].getClass());
+        }
+
         try {
-            Method m = clazz.getMethod(name, argsClass);
-            m.invoke(object, arguments);
+            Method m = clazz.getMethod(name, classes);
+            m.invoke(object, args);
         } catch (IllegalAccessException | NoSuchMethodException |
                 IllegalArgumentException | InvocationTargetException e) {
             // It's pretty evil to just swallow these exceptions.  But, these can only
@@ -252,6 +262,24 @@
         }
     }
 
+    private static <T> Class<T> preferPrimitiveClass(Class<T> boxedPrimitive) {
+        HashMap<Class<?>, Class<?>> map = new HashMap<>();
+        map.put(Byte.class, byte.class);
+        map.put(Short.class, short.class);
+        map.put(Integer.class, int.class);
+        map.put(Long.class, long.class);
+        map.put(Float.class, float.class);
+        map.put(Double.class, double.class);
+        map.put(Boolean.class, boolean.class);
+        map.put(Character.class, char.class);
+
+        if (map.containsKey(boxedPrimitive)) {
+            return (Class<T>) map.get(boxedPrimitive);
+        } else  {
+            return boxedPrimitive;
+        }
+    }
+
     private String actualLocation(String resourceName) {
         return new File(configuration.getLibRoot(), resourceName).toURI().toString();
     }