[JENKINS-50336] Fix loading optional extensions after installing a mi…

…ssing dependency (#3370) * [JENKINS-50336] Only keep extensions that could be loaded in memory So that delta computations will re-attempt to load any extension that previously failed or was skipped after a new plugin is installed. After installing a plugin, need to refresh extensions in order to pick up extensions that can be loaded thanks to this new plugin. * [JENKINS-50336] @OptionalExtension annotated classes were not being picked up after dynamic load of variant Recompute GuiceExtensionAnnotation list when refreshing extensions
jenkinsci · Jun 8, 2018 · 8c1b8b8 · 8c1b8b8
1 parent 8d438b7
commit 8c1b8b8
Show file tree

Hide file tree

Showing 5 changed files with 51 additions and 12 deletions.
diff --git a/core/src/main/java/hudson/ExtensionFinder.java b/core/src/main/java/hudson/ExtensionFinder.java
@@ -254,12 +254,9 @@ public static class GuiceFinder extends ExtensionFinder {
         private Map<Class<? extends Annotation>,GuiceExtensionAnnotation<?>> extensionAnnotations = Maps.newHashMap();
 
         public GuiceFinder() {
-            for (ExtensionComponent<GuiceExtensionAnnotation> ec : moduleFinder.find(GuiceExtensionAnnotation.class, Hudson.getInstance())) {
-                GuiceExtensionAnnotation gea = ec.getInstance();
-                extensionAnnotations.put(gea.annotationType,gea);
-            }
+            refreshExtensionAnnotations();
 
-            sezpozIndex = loadSezpozIndices(Jenkins.getInstance().getPluginManager().uberClassLoader);
+            SezpozModule extensions = new SezpozModule(loadSezpozIndices(Jenkins.getInstance().getPluginManager().uberClassLoader));
 
             List<Module> modules = new ArrayList<>();
             modules.add(new AbstractModule() {
@@ -270,14 +267,15 @@ protected void configure() {
                     bind(PluginManager.class).toInstance(j.getPluginManager());
                 }
             });
-            modules.add(new SezpozModule(sezpozIndex));
+            modules.add(extensions);
 
             for (ExtensionComponent<Module> ec : moduleFinder.find(Module.class, Hudson.getInstance())) {
                 modules.add(ec.getInstance());
             }
 
             try {
                 container = Guice.createInjector(modules);
+                sezpozIndex = extensions.getLoadedIndex();
             } catch (Throwable e) {
                 LOGGER.log(Level.SEVERE, "Failed to create Guice container from all the plugins",e);
                 // failing to load all bindings are disastrous, so recover by creating minimum that works
@@ -293,6 +291,13 @@ protected Injector resolve() {
             });
         }
 
+        private void refreshExtensionAnnotations() {
+            for (ExtensionComponent<GuiceExtensionAnnotation> ec : moduleFinder.find(GuiceExtensionAnnotation.class, Hudson.getInstance())) {
+                GuiceExtensionAnnotation gea = ec.getInstance();
+                extensionAnnotations.put(gea.annotationType,gea);
+            }
+        }
+
         private ImmutableList<IndexItem<?, Object>> loadSezpozIndices(ClassLoader classLoader) {
             List<IndexItem<?,Object>> indices = Lists.newArrayList();
             for (GuiceExtensionAnnotation<?> gea : extensionAnnotations.values()) {
@@ -315,24 +320,27 @@ public Injector getContainer() {
          */
         @Override
         public synchronized ExtensionComponentSet refresh() throws ExtensionRefreshException {
+            refreshExtensionAnnotations();
             // figure out newly discovered sezpoz components
             List<IndexItem<?, Object>> delta = Lists.newArrayList();
             for (Class<? extends Annotation> annotationType : extensionAnnotations.keySet()) {
                 delta.addAll(Sezpoz.listDelta(annotationType,sezpozIndex));
             }
-            List<IndexItem<?, Object>> l = Lists.newArrayList(sezpozIndex);
-            l.addAll(delta);
-            sezpozIndex = l;
+
+            SezpozModule deltaExtensions = new SezpozModule(delta);
 
             List<Module> modules = new ArrayList<>();
-            modules.add(new SezpozModule(delta));
+            modules.add(deltaExtensions);
             for (ExtensionComponent<Module> ec : moduleFinder.refresh().find(Module.class)) {
                 modules.add(ec.getInstance());
             }
 
             try {
                 final Injector child = container.createChildInjector(modules);
                 container = child;
+                List<IndexItem<?, Object>> l = Lists.newArrayList(sezpozIndex);
+                l.addAll(deltaExtensions.getLoadedIndex());
+                sezpozIndex = l;
 
                 return new ExtensionComponentSet() {
                     @Override
@@ -448,9 +456,11 @@ void error(Key<T> key, Throwable x) {
          */
         private class SezpozModule extends AbstractModule {
             private final List<IndexItem<?,Object>> index;
+            private final List<IndexItem<?,Object>> loadedIndex;
 
             public SezpozModule(List<IndexItem<?,Object>> index) {
                 this.index = index;
+                this.loadedIndex = new ArrayList<>();
             }
 
             /**
@@ -527,6 +537,7 @@ public Object get() {
                                     }
                                 }).in(scope);
                         }
+                        loadedIndex.add(item);
                     } catch (Exception|LinkageError e) {
                         // sometimes the instantiation fails in an indirect classloading failure,
                         // which results in a LinkageError
@@ -535,6 +546,10 @@ public Object get() {
                     }
                 }
             }
+
+            public List<IndexItem<?, Object>> getLoadedIndex() {
+                return Collections.unmodifiableList(loadedIndex);
+            }
         }
     }
 

diff --git a/core/src/main/java/hudson/PluginManager.java b/core/src/main/java/hudson/PluginManager.java
@@ -26,6 +26,7 @@
 import edu.umd.cs.findbugs.annotations.NonNull;
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import hudson.security.ACLContext;
+import jenkins.ExtensionRefreshException;
 import jenkins.util.SystemProperties;
 import hudson.PluginWrapper.Dependency;
 import hudson.init.InitMilestone;
@@ -921,6 +922,11 @@ protected boolean filter(Method e) {
             // Redo who depends on who.
             resolveDependantPlugins();
 
+            try {
+                Jenkins.get().refreshExtensions();
+            } catch (ExtensionRefreshException e) {
+                throw new IOException("Failed to refresh extensions after installing " + sn + " plugin", e);
+            }
             LOGGER.info("Plugin " + p.getShortName()+":"+p.getVersion() + " dynamically installed");
         }
     }

diff --git a/test/src/test/java/hudson/PluginManagerTest.java b/test/src/test/java/hudson/PluginManagerTest.java
@@ -46,6 +46,8 @@
 
 import jenkins.ClassLoaderReflectionToolkit;
 import jenkins.RestartRequiredException;
+import jenkins.model.GlobalConfiguration;
+
 import net.sf.json.JSONArray;
 import net.sf.json.JSONObject;
 import org.apache.commons.io.FileUtils;
@@ -342,8 +344,8 @@ private String callDependerValue() throws Exception {
         assertEquals("dependee", callDependerValue());
 
         // No extensions exist.
-        // extensions in depender is not loaded.
-        assertTrue(r.jenkins.getExtensionList("org.jenkinsci.plugins.dependencytest.dependee.DependeeExtensionPoint").isEmpty());
+        // extensions in depender are loaded.
+        assertFalse(r.jenkins.getExtensionList("org.jenkinsci.plugins.dependencytest.dependee.DependeeExtensionPoint").isEmpty());
     }
 
     @Issue("JENKINS-21486")
@@ -533,4 +535,20 @@ public void findResourceForPluginFirstClassLoader() throws Exception {
 
         assertEquals(fromPlugin, fromToolkit);
     }
+
+    // Sources for jenkins-50336.hpi are available at https://github.com/Vlatombe/jenkins-50336
+    //
+    // package io.jenkins.plugins;
+    // import org.jenkinsci.plugins.variant.OptionalExtension;
+    // import jenkins.model.GlobalConfiguration;
+    // @OptionalExtension public class MyGlobalConfiguration extends GlobalConfiguration {}
+    //
+    @Issue("JENKINS-50336")
+    @Test
+    public void optionalExtensionCanBeFoundAfterDynamicLoadOfVariant() throws Exception {
+        dynamicLoad("variant.hpi");
+        assertNotNull(r.jenkins.getPluginManager().getPlugin("variant"));
+        dynamicLoad("jenkins-50336.hpi");
+        assertTrue(ExtensionList.lookup(GlobalConfiguration.class).stream().anyMatch(gc -> "io.jenkins.plugins.MyGlobalConfiguration".equals(gc.getClass().getName())));
+    }
 }
diff --git a/test/src/test/resources/plugins/jenkins-50336.hpi b/test/src/test/resources/plugins/jenkins-50336.hpi
diff --git a/test/src/test/resources/plugins/variant.hpi b/test/src/test/resources/plugins/variant.hpi