# HG changeset patch # User Omair Majid # Date 1365453639 14400 # Node ID 653f57559b4277636ae06377d337761961eca398 # Parent c1edec7797bdc75b817370c5662b0d7ecb6d3300 Make launcher thread safe Reviewed-by: vanaltj Review-thread: http://icedtea.classpath.org/pipermail/thermostat/2013-April/006306.html diff -r c1edec7797bd -r 653f57559b42 agent/cli/src/main/java/com/redhat/thermostat/agent/cli/impl/ServiceCommand.java --- 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: diff -r c1edec7797bd -r 653f57559b42 client/cli/src/main/java/com/redhat/thermostat/client/cli/internal/ShellCommand.java --- 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"); } diff -r c1edec7797bd -r 653f57559b42 client/cli/src/test/java/com/redhat/thermostat/client/cli/internal/ShellCommandTest.java --- 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(); diff -r c1edec7797bd -r 653f57559b42 common/core/src/main/java/com/redhat/thermostat/common/Launcher.java --- 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> 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> listeners, boolean inShell); } diff -r c1edec7797bd -r 653f57559b42 launcher/src/main/java/com/redhat/thermostat/launcher/internal/LauncherImpl.java --- 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 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> listeners, boolean inShell) { - - usageCount++; - waitForArgs(); - + public void run(String[] args, Collection> 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> listeners, boolean inShell) { + private void runCommandFromArguments(String[] args, Collection> 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; diff -r c1edec7797bd -r 653f57559b42 launcher/src/test/java/com/redhat/thermostat/launcher/internal/LauncherImplTest.java --- 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> listeners) { + private void wrappedRun(LauncherImpl launcher, String[] args, boolean inShell, Collection> listeners) { try { - launcher.run(listeners, inShell); + launcher.run(args, listeners, inShell); } catch (ExitException e) { System.out.println(e.getMessage()); } @@ -454,8 +452,7 @@ ArgumentCaptor 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 diff -r c1edec7797bd -r 653f57559b42 main/pom.xml --- 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 @@ Red Hat, Inc. - com.redhat.thermostat.main.impl.Activator com.redhat.thermostat.main com.redhat.thermostat.main, diff -r c1edec7797bd -r 653f57559b42 main/src/main/java/com/redhat/thermostat/main/Thermostat.java --- 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])); } diff -r c1edec7797bd -r 653f57559b42 main/src/main/java/com/redhat/thermostat/main/impl/Activator.java --- 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 - * . - * - * 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(); - } - -} - diff -r c1edec7797bd -r 653f57559b42 main/src/main/java/com/redhat/thermostat/main/impl/FrameworkProvider.java --- 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 Class preferPrimitiveClass(Class boxedPrimitive) { + HashMap, 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) map.get(boxedPrimitive); + } else { + return boxedPrimitive; + } + } + private String actualLocation(String resourceName) { return new File(configuration.getLibRoot(), resourceName).toURI().toString(); }