Mercurial > hg > release > thermostat-0.13

changeset 1214:69692e087b13

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
PR1514: Thermostat should not start bundles with the same symbolic name/version twice Reviewed-by: jerboaa, neugens Review-thread: http://icedtea.classpath.org/pipermail/thermostat/2013-August/007809.html
author Omair Majid <omajid@redhat.com>
date Wed, 14 Aug 2013 13:11:11 -0400
parents d44f06cf8a58
children 7012500bb995
files integration-tests/src/test/java/com/redhat/thermostat/itest/IntegrationTest.java integration-tests/src/test/java/com/redhat/thermostat/itest/VmCommandsTest.java launcher/src/main/java/com/redhat/thermostat/launcher/internal/BuiltInCommandInfo.java launcher/src/main/java/com/redhat/thermostat/launcher/internal/BundleManagerImpl.java launcher/src/main/java/com/redhat/thermostat/launcher/internal/CommandInfo.java main/src/main/java/com/redhat/thermostat/main/impl/FrameworkProvider.java
diffstat 6 files changed, 80 insertions(+), 13 deletions(-) [+]
line wrap: on
line diff
--- a/integration-tests/src/test/java/com/redhat/thermostat/itest/IntegrationTest.java	Wed Aug 14 10:38:38 2013 -0400
+++ b/integration-tests/src/test/java/com/redhat/thermostat/itest/IntegrationTest.java	Wed Aug 14 13:11:11 2013 -0400
@@ -296,11 +296,21 @@
         }
     }
 
+    /** Confirm that there are no 'command not found'-like messages in the spawn's stdout/stderr */
+    public static void assertCommandIsFound(Spawn spawn) {
+        assertCommandIsFound(spawn.getCurrentStandardOutContents(), spawn.getCurrentStandardErrContents());
+    }
+
     public static void assertCommandIsFound(String stdOutContents, String stdErrContents) {
         assertFalse(stdOutContents.contains("unknown command"));
         assertFalse(stdErrContents.contains("unknown command"));
     }
 
+    /** Confirm that there are no exception stack traces in the spawn's stdout/stderr */
+    public static void assertNoExceptions(Spawn spawn) {
+        assertNoExceptions(spawn.getCurrentStandardOutContents(), spawn.getCurrentStandardErrContents());
+    }
+
     public static void assertNoExceptions(String stdOutContents, String stdErrContents) {
         assertFalse(stdOutContents.contains("Exception"));
         assertFalse(stdErrContents.contains("Exception"));
--- a/integration-tests/src/test/java/com/redhat/thermostat/itest/VmCommandsTest.java	Wed Aug 14 10:38:38 2013 -0400
+++ b/integration-tests/src/test/java/com/redhat/thermostat/itest/VmCommandsTest.java	Wed Aug 14 13:11:11 2013 -0400
@@ -125,6 +125,26 @@
         }
     }
 
+    @Test
+    public void testNormalCommandAndPluginInShell() throws Exception {
+        Spawn shell = spawnThermostat("shell");
+
+        shell.expect(SHELL_PROMPT);
+        shell.send("list-vms\n");
+
+        shell.expect(SHELL_PROMPT);
+
+        shell.send("dump-heap\n");
+
+        shell.expect(SHELL_PROMPT);
+
+        shell.send("exit\n");
+        shell.expectClose();
+
+        assertCommandIsFound(shell);
+        assertNoExceptions(shell);
+    }
+
     private static Spawn commandAgainstMongo(String... args) throws IOException {
         if (args == null || args.length == 0) {
             throw new IllegalArgumentException("args must be an array with something");
--- a/launcher/src/main/java/com/redhat/thermostat/launcher/internal/BuiltInCommandInfo.java	Wed Aug 14 10:38:38 2013 -0400
+++ b/launcher/src/main/java/com/redhat/thermostat/launcher/internal/BuiltInCommandInfo.java	Wed Aug 14 13:11:11 2013 -0400
@@ -37,6 +37,7 @@
 package com.redhat.thermostat.launcher.internal;
 
 import java.io.File;
+import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
@@ -104,14 +105,21 @@
                 continue;
             }
             File file = new File(libRoot, value.trim());
-            String path = file.toURI().toString();
-            if (!file.exists()) {
-                logger.severe("Bundle " + path + " required by " + getName() +
+            try {
+                String path = file.getCanonicalFile().toURI().toString();
+                if (!file.exists()) {
+                    logger.severe("Bundle " + path + " required by " + getName() +
+                            " command does not exist in the filesystem.  This will cause" +
+                            " osgi wiring issue when attempting to run this command.");
+                    // Allow to proceed because this command may never be called.
+                } else {
+                    dependencies.add(path);
+                }
+            } catch (IOException e) {
+                logger.severe("Bundle " + file + " required by " + getName() +
                         " command does not exist in the filesystem.  This will cause" +
                         " osgi wiring issue when attempting to run this command.");
                 // Allow to proceed because this command may never be called.
-            } else {
-                dependencies.add(path);
             }
         }
     }
--- a/launcher/src/main/java/com/redhat/thermostat/launcher/internal/BundleManagerImpl.java	Wed Aug 14 10:38:38 2013 -0400
+++ b/launcher/src/main/java/com/redhat/thermostat/launcher/internal/BundleManagerImpl.java	Wed Aug 14 13:11:11 2013 -0400
@@ -72,7 +72,12 @@
 
     private static final Logger logger = LoggingUtils.getLogger(BundleManagerImpl.class);
 
-    // Bundle Name and version -> path
+    // Bundle Name and version -> path (with symlinks resolved)
+    // Match FrameworkProvider which uses canonical/symlink-resolved paths. If
+    // there is a mismatch, there are going to be clashes with trying to load
+    // two files (which are really the same) with the same bundle symbolic
+    // name/version. See
+    // http://icedtea.classpath.org/bugzilla/show_bug.cgi?id=1514
     private final Map<BundleInformation, Path> known;
     private Configuration configuration;
     private BundleLoader loader;
@@ -102,7 +107,17 @@
                                 if (name == null || version == null) {
                                     logger.config("file " + file.toString() + " is missing osgi metadata; wont be usable for dependencies");
                                 } else {
-                                    known.put(new BundleInformation(name, version), file);
+                                    BundleInformation info = new BundleInformation(name, version);
+                                    Path old = known.get(info);
+                                    // Path is completely resolved. First one wins.
+                                    if (old == null) {
+                                        known.put(info, file.toRealPath());
+                                    } else {
+                                        if (!old.equals(file.toRealPath())) {
+                                            logger.warning("bundles " + old + " and " + file + " both provide " + info);
+                                        }
+                                        // leave old
+                                    }
                                 }
                             } catch (IOException e) {
                                 logger.severe("Error in reading " + file);
@@ -121,10 +136,10 @@
         if (configuration.getPrintOSGiInfo()) {
             logger.fine("Found: " + known.size() + " bundles");
             logger.fine("Took " + (t2 -t1) + "ns");
+        }
 
-            for (Entry<BundleInformation, Path> bundles : known.entrySet()) {
-                logger.finest(bundles.getKey().toString() + " is at " + bundles.getValue().toString());
-            }
+        for (Entry<BundleInformation, Path> bundles : known.entrySet()) {
+            logger.finest(bundles.getKey().toString() + " is at " + bundles.getValue().toString());
         }
     }
 
@@ -143,7 +158,7 @@
                 logger.warning("no known bundle matching " + info.toString());
                 continue;
             }
-            paths.add(bundlePath.toFile().toURI().toString());
+            paths.add(bundlePath.toFile().getCanonicalFile().toURI().toString());
         }
         loadBundlesByPath(paths);
     }
--- a/launcher/src/main/java/com/redhat/thermostat/launcher/internal/CommandInfo.java	Wed Aug 14 10:38:38 2013 -0400
+++ b/launcher/src/main/java/com/redhat/thermostat/launcher/internal/CommandInfo.java	Wed Aug 14 13:11:11 2013 -0400
@@ -75,7 +75,10 @@
 
     List<BundleInformation> getBundles();
 
-    /** Returns a list of jar that this command depends on */
+    /**
+     * Returns a list of jar that this command depends on
+     * @return a list of strings containing the URIs of jars-on-disk
+     */
     List<String> getDependencyResourceNames();
 
 }
--- a/main/src/main/java/com/redhat/thermostat/main/impl/FrameworkProvider.java	Wed Aug 14 10:38:38 2013 -0400
+++ b/main/src/main/java/com/redhat/thermostat/main/impl/FrameworkProvider.java	Wed Aug 14 13:11:11 2013 -0400
@@ -62,6 +62,7 @@
 import com.redhat.thermostat.common.Launcher;
 import com.redhat.thermostat.launcher.BundleManager;
 import com.redhat.thermostat.shared.config.Configuration;
+import com.redhat.thermostat.shared.config.InvalidConfigurationException;
 
 public class FrameworkProvider {
 
@@ -283,8 +284,18 @@
         }
     }
 
+    // Resolve symlinks completely; This makes the behaviour of this class
+    // consistent with BundleManagerImpl and avoid problems where two different
+    // files across bootstrap and later code provide the same bundle symbolic
+    // name version
     private String actualLocation(String resourceName) {
-        return new File(configuration.getSystemLibRoot(), resourceName).toURI().toString();
+        File file = new File(configuration.getSystemLibRoot(), resourceName);
+        try {
+            return file.getCanonicalFile().toURI().toString();
+        } catch (IOException e) {
+            // okay, lets not canonicalize the path
+            return file.toURI().toString();
+        }
     }
 }