[FIXED JENKINS-12753] Added PluginStrategy.getShortName to avoid actu…

…ally trying to unpack a plugin before throwing RestartRequiredException.
jenkinsci · May 20, 2014 · 7f856b5 · oleg-nenashev · May 30, 2014 · oleg-nenashev
1 parent 9062a94
commit 7f856b5
Show file tree

Hide file tree

Showing 6 changed files with 78 additions and 15 deletions.
diff --git a/changelog.html b/changelog.html
@@ -55,6 +55,9 @@
 <!-- Record your changes in the trunk here. -->
 <div id="trunk" style="display:none"><!--=TRUNK-BEGIN=-->
 <ul class=image>
+  <li class="bug">
+    Misleading error message trying to dynamically update a plugin (which is impossible) on an NFS filesystem.
+    (<a href="https://issues.jenkins-ci.org/browse/JENKINS-12753">issue 12753</a>)
   <li class="bug">
     Updated component used by the swap space monitor to better support Debian and AIX.
   <li class="bug">

diff --git a/core/src/main/java/hudson/ClassicPluginStrategy.java b/core/src/main/java/hudson/ClassicPluginStrategy.java
@@ -66,6 +66,7 @@
 import java.util.List;
 import java.util.Vector;
 import java.util.jar.Attributes;
+import java.util.jar.JarFile;
 import java.util.jar.Manifest;
 import java.util.logging.Level;
 import java.util.logging.Logger;
@@ -92,15 +93,26 @@ public ClassicPluginStrategy(PluginManager pluginManager) {
         this.pluginManager = pluginManager;
     }
 
-    public PluginWrapper createPluginWrapper(File archive) throws IOException {
-        final Manifest manifest;
-        URL baseResourceURL;
+    @Override public String getShortName(File archive) throws IOException {
+        Manifest manifest;
+        if (isLinked(archive)) {
+            manifest = loadLinkedManifest(archive);
+        } else {
+            JarFile jf = new JarFile(archive, false);
+            try {
+                manifest = jf.getManifest();
+            } finally {
+                jf.close();
+            }
+        }
+        return PluginWrapper.computeShortName(manifest, archive);
+    }
 
-        File expandDir = null;
-        // if .hpi, this is the directory where war is expanded
+    private static boolean isLinked(File archive) {
+        return archive.getName().endsWith(".hpl") || archive.getName().endsWith(".jpl");
+    }
 
-        boolean isLinked = archive.getName().endsWith(".hpl") || archive.getName().endsWith(".jpl");
-        if (isLinked) {
+    private static Manifest loadLinkedManifest(File archive) throws IOException {
             // resolve the .hpl file to the location of the manifest file
             final String firstLine = IOUtils.readFirstLine(new FileInputStream(archive), "UTF-8");
             if (firstLine.startsWith("Manifest-Version:")) {
@@ -112,12 +124,24 @@ public PluginWrapper createPluginWrapper(File archive) throws IOException {
             // then parse manifest
             FileInputStream in = new FileInputStream(archive);
             try {
-                manifest = new Manifest(in);
+                return new Manifest(in);
             } catch (IOException e) {
                 throw new IOException("Failed to load " + archive, e);
             } finally {
                 in.close();
             }
+    }
+
+    @Override public PluginWrapper createPluginWrapper(File archive) throws IOException {
+        final Manifest manifest;
+        URL baseResourceURL;
+
+        File expandDir = null;
+        // if .hpi, this is the directory where war is expanded
+
+        boolean isLinked = isLinked(archive);
+        if (isLinked) {
+            manifest = loadLinkedManifest(archive);
         } else {
             if (archive.isDirectory()) {// already expanded
                 expandDir = archive;

diff --git a/core/src/main/java/hudson/PluginManager.java b/core/src/main/java/hudson/PluginManager.java
@@ -412,11 +412,21 @@ private boolean containsHpiJpi(Collection<String> bundledPlugins, String name) {
      */
     public void dynamicLoad(File arc) throws IOException, InterruptedException, RestartRequiredException {
         LOGGER.info("Attempting to dynamic load "+arc);
-        final PluginWrapper p = strategy.createPluginWrapper(arc);
-        String sn = p.getShortName();
+        PluginWrapper p = null;
+        String sn;
+        try {
+            sn = strategy.getShortName(arc);
+        } catch (AbstractMethodError x) {
+            LOGGER.log(WARNING, "JENKINS-12753 fix not active: {0}", x.getMessage());
+            p = strategy.createPluginWrapper(arc);
+            sn = p.getShortName();
+        }
         if (getPlugin(sn)!=null)
             throw new RestartRequiredException(Messages._PluginManager_PluginIsAlreadyInstalled_RestartRequired(sn));
 
+        if (p == null) {
+            p = strategy.createPluginWrapper(arc);
+        }
         if (p.supportsDynamicLoad()== YesNoMaybe.NO)
             throw new RestartRequiredException(Messages._PluginManager_PluginDoesntSupportDynamicLoad_RestartRequired(sn));
 
@@ -442,10 +452,11 @@ public void dynamicLoad(File arc) throws IOException, InterruptedException, Rest
 
         // run initializers in the added plugin
         Reactor r = new Reactor(InitMilestone.ordering());
-        r.addAll(new InitializerFinder(p.classLoader) {
+        final ClassLoader loader = p.classLoader;
+        r.addAll(new InitializerFinder(loader) {
             @Override
             protected boolean filter(Method e) {
-                return e.getDeclaringClass().getClassLoader()!=p.classLoader || super.filter(e);
+                return e.getDeclaringClass().getClassLoader() != loader || super.filter(e);
             }
         }.discoverTasks(r));
         try {

diff --git a/core/src/main/java/hudson/PluginStrategy.java b/core/src/main/java/hudson/PluginStrategy.java
@@ -24,11 +24,10 @@
 package hudson;
 
 import hudson.model.Hudson;
-import jenkins.model.Jenkins;
-
 import java.io.File;
 import java.io.IOException;
 import java.util.List;
+import javax.annotation.Nonnull;
 
 /**
  * Pluggability point for how to create {@link PluginWrapper}.
@@ -50,6 +49,13 @@ public interface PluginStrategy extends ExtensionPoint {
 	PluginWrapper createPluginWrapper(File archive)
 			throws IOException;
 
+    /**
+     * Finds the plugin name without actually unpacking anything {@link #createPluginWrapper} would.
+     * Needed by {@link PluginManager#dynamicLoad} to decide whether such a plugin is already installed.
+     * @return the {@link PluginWrapper#getShortName}
+     */
+    @Nonnull String getShortName(File archive) throws IOException;
+
 	/**
 	 * Loads the plugin and starts it.
 	 * 

diff --git a/core/src/main/java/hudson/PluginWrapper.java b/core/src/main/java/hudson/PluginWrapper.java
@@ -238,7 +238,7 @@ public URL getIndexPage() {
         return idx != null && idx.toString().contains(shortName) ? idx : null;
     }
 
-    private String computeShortName(Manifest manifest, File archive) {
+    static String computeShortName(Manifest manifest, File archive) {
         // use the name captured in the manifest, as often plugins
         // depend on the specific short name in its URLs.
         String n = manifest.getMainAttributes().getValue("Short-Name");

diff --git a/test/src/test/java/hudson/PluginManagerTest.java b/test/src/test/java/hudson/PluginManagerTest.java
@@ -40,6 +40,7 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.concurrent.Future;
+import jenkins.RestartRequiredException;
 import org.apache.commons.io.FileUtils;
 import org.apache.tools.ant.filters.StringInputStream;
 import static org.junit.Assert.*;
@@ -356,4 +357,22 @@ private String callDependerValue() throws Exception {
         assertTrue(r.jenkins.getExtensionList("org.jenkinsci.plugins.dependencytest.dependee.DependeeExtensionPoint").isEmpty());
     }
 
+    @Bug(12753)
+    @WithPlugin("tasks.jpi")
+    @Test public void dynamicLoadRestartRequiredException() throws Exception {
+        File jpi = new File(r.jenkins.getRootDir(), "plugins/tasks.jpi");
+        assertTrue(jpi.isFile());
+        FileUtils.touch(jpi);
+        File timestamp = new File(r.jenkins.getRootDir(), "plugins/tasks/.timestamp2");
+        assertTrue(timestamp.isFile());
+        long lastMod = timestamp.lastModified();
+        try {
+            r.jenkins.getPluginManager().dynamicLoad(jpi);
+            fail("should not have worked");
+        } catch (RestartRequiredException x) {
+            // good
+        }
+        assertEquals("should not have tried to delete & unpack", lastMod, timestamp.lastModified());
+    }
+
 }