Merge pull request #327 from olivergondza/prepare-for-guava-upgrade

[JENKINS-66306] Prepare for guava upgrade
jenkinsci · Aug 9, 2021 · 63064c6 · 63064c6
2 parents 5c6e435 + 44c45b8
commit 63064c6
Show file tree

Hide file tree

Showing 6 changed files with 87 additions and 112 deletions.
diff --git a/plugin/pom.xml b/plugin/pom.xml
@@ -108,6 +108,11 @@
             <artifactId>instance-identity</artifactId>
             <version>2.2</version>
         </dependency>
+        <dependency>
+            <groupId>io.jenkins.plugins</groupId>
+            <artifactId>caffeine-api</artifactId>
+            <version>2.9.1-23.v51c4e2c879c8</version>
+        </dependency>
 
         <!-- Hardcoding upper-bound versions of dependencies -->
         <dependency>

diff --git a/plugin/src/main/java/jenkins/plugins/openstack/compute/JCloudsBuildWrapper.java b/plugin/src/main/java/jenkins/plugins/openstack/compute/JCloudsBuildWrapper.java
@@ -20,8 +20,10 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 import java.util.concurrent.Callable;
 import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
 
 import javax.annotation.Nonnull;
 
@@ -34,8 +36,6 @@
 import org.kohsuke.stapler.DataBoundConstructor;
 import org.openstack4j.model.compute.Server;
 
-import com.google.common.base.Function;
-import com.google.common.collect.Iterables;
 import com.google.common.util.concurrent.MoreExecutors;
 
 public class JCloudsBuildWrapper extends BuildWrapper {
@@ -60,17 +60,13 @@ public Environment setUp(final AbstractBuild build, Launcher launcher, final Bui
         final ServerScope.Build scope = new ServerScope.Build(build);
 
         // eagerly lookup node supplier so that errors occur before we attempt to provision things
-        Iterable<NodePlan> nodePlans = Iterables.transform(instancesToRun, new Function<InstancesToRun, NodePlan>() {
-
-            @SuppressWarnings("unchecked")
-            public NodePlan apply(InstancesToRun instance) {
-                JCloudsCloud cloud = JCloudsCloud.getByName(instance.cloudName);
-                String templateName = Util.replaceMacro(instance.getActualTemplateName(), build.getBuildVariableResolver());
-                JCloudsSlaveTemplate template = cloud.getTemplate(templateName);
-                if (template == null) throw new IllegalArgumentException("No such template " + templateName);
-                return new NodePlan(cloud, template, instance.count, scope);
-            }
-        });
+        Iterable<NodePlan> nodePlans = instancesToRun.stream().map(instance -> {
+            JCloudsCloud cloud = JCloudsCloud.getByName(Objects.requireNonNull(instance.cloudName));
+            String templateName = Util.replaceMacro(instance.getActualTemplateName(), build.getBuildVariableResolver());
+            JCloudsSlaveTemplate template = cloud.getTemplate(templateName);
+            if (template == null) throw new IllegalArgumentException("No such template " + templateName);
+            return new NodePlan(cloud, template, instance.count, scope);
+        }).collect(Collectors.toList());
 
         ListeningExecutorService executor = MoreExecutors.listeningDecorator(Computer.threadPoolForRemoting);
         final ImmutableList.Builder<RunningNode> cloudTemplateNodeBuilder = ImmutableList.builder();
@@ -89,7 +85,7 @@ public NodePlan apply(InstancesToRun instance) {
 
                 ListenableFuture<Server> provisionTemplate = executor.submit(nodePlan.getNodeSupplier());
 
-                Futures.addCallback(provisionTemplate, new FutureCallback<Server>() {
+                FutureCallback<Server> callback = new FutureCallback<Server>() {
                     public void onSuccess(Server result) {
                         if (result != null) {
                             synchronized (cloudTemplateNodeBuilder) {
@@ -108,7 +104,8 @@ public void onFailure(@Nonnull Throwable t) {
                                 index, nodePlan.getCloud(), nodePlan.getTemplate(), Functions.printThrowable(t)
                         );
                     }
-                });
+                };
+                Futures.addCallback(provisionTemplate, callback, MoreExecutors.directExecutor());
 
                 plannedInstancesBuilder.add(provisionTemplate);
             }

diff --git a/plugin/src/main/java/jenkins/plugins/openstack/compute/JCloudsSlave.java b/plugin/src/main/java/jenkins/plugins/openstack/compute/JCloudsSlave.java
@@ -1,5 +1,8 @@
 package jenkins.plugins.openstack.compute;
 
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
+import com.google.common.collect.ImmutableList;
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import hudson.EnvVars;
 import hudson.Extension;
@@ -31,10 +34,6 @@
 import org.openstack4j.model.compute.Flavor;
 import org.openstack4j.model.compute.Server;
 
-import com.google.common.cache.Cache;
-import com.google.common.cache.CacheBuilder;
-import com.google.common.collect.ImmutableList;
-
 import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
 import java.io.IOException;
@@ -44,9 +43,8 @@
 import java.util.Map;
 import java.util.NoSuchElementException;
 import java.util.Objects;
-import java.util.concurrent.Callable;
-import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
+import java.util.function.Function;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
@@ -187,7 +185,7 @@ protected Object readResolve() {
 
     /** Creates a cache where data will be kept for a short duration before being discarded. */
     private static @Nonnull <K, V> Cache<K, V> makeCache() {
-        return CacheBuilder.newBuilder().expireAfterWrite(5, TimeUnit.MINUTES).build();
+        return Caffeine.newBuilder().expireAfterWrite(5, TimeUnit.MINUTES).build();
     }
 
     /**
@@ -219,7 +217,7 @@ protected Object readResolve() {
      */
     @Restricted(DoNotUse.class) // Jelly
     public @Nonnull Map<String, String> getLiveOpenstackServerDetails() {
-        return getCachableData("liveData", this::readLiveOpenstackServerDetails);
+        return getCachableData("liveData", (String unused) -> readLiveOpenstackServerDetails());
     }
 
     private @Nonnull Map<String, String> readLiveOpenstackServerDetails() {
@@ -303,15 +301,15 @@ private static void putIfNotNullOrEmpty(
     }
 
     /** Gets something from the cache, loading it into the cache if necessary. */
-    private @Nonnull Map<String, String> getCachableData(@Nonnull final String key, @Nonnull final Callable<Map<String, String>> dataloader) {
+    private @Nonnull Map<String, String> getCachableData(@Nonnull final String key, @Nonnull final Function<String, Map<String, String>> dataloader) {
         try {
-            return cache.get(key, dataloader);
-        } catch (ExecutionException e) {
+            return Objects.requireNonNull(cache.get(key, dataloader));
+        } catch (RuntimeException e) { // Propagated from cacheMissFunction
             final Throwable cause = e.getCause();
             if (cause instanceof RuntimeException) {
                 throw (RuntimeException) cause;
             }
-            throw new RuntimeException(e);
+            throw e;
         }
     }
 

diff --git a/plugin/src/main/java/jenkins/plugins/openstack/compute/internal/Openstack.java b/plugin/src/main/java/jenkins/plugins/openstack/compute/internal/Openstack.java
@@ -23,58 +23,28 @@
  */
 package jenkins.plugins.openstack.compute.internal;
 
-import java.io.File;
-import java.io.IOException;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.Collections;
-import java.util.Comparator;
-import java.util.Date;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.List;
-import java.util.Map;
-import java.util.NoSuchElementException;
-import java.util.Objects;
-import java.util.Optional;
-import java.util.TreeMap;
-import java.util.TreeSet;
-import java.util.concurrent.Callable;
-import java.util.concurrent.ExecutionException;
-import java.util.concurrent.TimeUnit;
-import java.util.logging.Level;
-import java.util.logging.Logger;
-import java.util.stream.Collectors;
-
-import javax.annotation.CheckForNull;
-import javax.annotation.Nonnegative;
-import javax.annotation.Nonnull;
-import javax.annotation.concurrent.ThreadSafe;
-
 import com.cloudbees.plugins.credentials.common.PasswordCredentials;
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
 import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.MoreObjects;
-import com.google.common.cache.Cache;
-import com.google.common.cache.CacheBuilder;
-import com.google.common.util.concurrent.UncheckedExecutionException;
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import hudson.Extension;
 import hudson.ExtensionList;
 import hudson.ExtensionPoint;
 import hudson.Util;
-import hudson.remoting.Which;
 import hudson.util.FormValidation;
+import jenkins.model.Jenkins;
 import jenkins.plugins.openstack.compute.auth.OpenstackCredential;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
 import org.openstack4j.api.Builders;
-import org.openstack4j.api.networking.NetFloatingIPService;
-import org.openstack4j.api.networking.NetworkingService;
-import org.openstack4j.core.transport.Config;
 import org.openstack4j.api.OSClient;
 import org.openstack4j.api.client.IOSClientBuilder;
 import org.openstack4j.api.compute.ServerService;
 import org.openstack4j.api.exceptions.ResponseException;
+import org.openstack4j.api.networking.NetFloatingIPService;
+import org.openstack4j.api.networking.NetworkingService;
+import org.openstack4j.core.transport.Config;
 import org.openstack4j.model.common.ActionResponse;
 import org.openstack4j.model.compute.Address;
 import org.openstack4j.model.compute.Fault;
@@ -88,16 +58,38 @@
 import org.openstack4j.model.image.v2.Image;
 import org.openstack4j.model.network.NetFloatingIP;
 import org.openstack4j.model.network.Network;
-import org.openstack4j.model.network.Router;
 import org.openstack4j.model.network.Port;
+import org.openstack4j.model.network.Router;
 import org.openstack4j.model.network.ext.NetworkIPAvailability;
 import org.openstack4j.model.network.options.PortListOptions;
 import org.openstack4j.model.storage.block.Volume;
 import org.openstack4j.model.storage.block.Volume.Status;
 import org.openstack4j.model.storage.block.VolumeSnapshot;
 import org.openstack4j.openstack.OSFactory;
 
-import jenkins.model.Jenkins;
+import javax.annotation.CheckForNull;
+import javax.annotation.Nonnegative;
+import javax.annotation.Nonnull;
+import javax.annotation.concurrent.ThreadSafe;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.Date;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.NoSuchElementException;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.TreeMap;
+import java.util.TreeSet;
+import java.util.concurrent.TimeUnit;
+import java.util.function.Function;
+import java.util.logging.Level;
+import java.util.logging.Logger;
+import java.util.stream.Collectors;
 
 /**
  * Encapsulate {@link OSClient}.
@@ -820,7 +812,7 @@ private static void debug(@Nonnull String msg, @Nonnull String... args) {
 
     @Restricted(NoExternalUse.class) // Extension point just for testing
     public static abstract class FactoryEP implements ExtensionPoint {
-        private final transient @Nonnull Cache<String, Openstack> cache = CacheBuilder.newBuilder()
+        private final transient @Nonnull Cache<String, Openstack> cache = Caffeine.newBuilder()
                 // There is no clear reasoning behind particular expiration policy except that individual instances can
                 // have different token expiration time, which is something guava does not support. This expiration needs
                 // to be implemented separately.
@@ -847,12 +839,18 @@ public static abstract class FactoryEP implements ExtensionPoint {
                     + (auth instanceof PasswordCredentials ? ((PasswordCredentials) auth).getPassword().getEncryptedValue() + '\n' : "")
                     + region);
             final FactoryEP ep = ExtensionList.lookup(FactoryEP.class).get(0);
-            final Callable<Openstack> cacheMissFunction = () -> ep.getOpenstack(endPointUrl, ignoreSsl, auth, region);
+            final Function<String, Openstack> cacheMissFunction = (String unused) -> {
+                try {
+                    return ep.getOpenstack(endPointUrl, ignoreSsl, auth, region);
+                } catch (FormValidation ex) {
+                    throw new RuntimeException(ex);
+                }
+            };
 
-            // Get an instance, creating a new one if necessary.
             try {
-                return ep.cache.get(fingerprint, cacheMissFunction);
-            } catch (UncheckedExecutionException | ExecutionException e) {
+                // cacheMissFunction is guaranteed to return nonnull
+                return Objects.requireNonNull(ep.cache.get(fingerprint, cacheMissFunction));
+            } catch (RuntimeException e) { // Propagated from cacheMissFunction
                 // Exception was thrown when creating a new instance.
                 final Throwable cause = e.getCause();
                 if (cause instanceof FormValidation) {
@@ -861,7 +859,7 @@ public static abstract class FactoryEP implements ExtensionPoint {
                 if (cause instanceof RuntimeException) {
                     throw (RuntimeException) cause;
                 }
-                throw new RuntimeException(e);
+                throw e;
             }
         }
 
@@ -974,16 +972,4 @@ private SessionClientV3Provider(OSClient.OSClientV3 toStore, String usedRegion,
             }
         }
     }
-
-    static {
-        // Log where guava is coming from. This can not be reliably tested as jenkins-test-harness, hpi:run and actual
-        // jenkins deployed plugin have different classloader environments. Messing around with maven-hpi-plugin opts can
-        // fix or break any of that and there is no regression test to catch that.
-        try {
-            File path = Which.jarFile(MoreObjects.ToStringHelper.class);
-            LOGGER.info("com.google.common.base.Objects loaded from " + path);
-        } catch (IOException e) {
-            LOGGER.log(Level.WARNING, "Unable to get source of com.google.common.base.Objects", e);
-        }
-    }
 }
diff --git a/plugin/src/test/java/jenkins/plugins/openstack/compute/JCloudsCloudTest.java b/plugin/src/test/java/jenkins/plugins/openstack/compute/JCloudsCloudTest.java
@@ -11,7 +11,7 @@
 import com.gargoylesoftware.htmlunit.html.HtmlForm;
 import com.gargoylesoftware.htmlunit.html.HtmlFormUtil;
 import com.gargoylesoftware.htmlunit.html.HtmlPage;
-import com.google.common.cache.Cache;
+import com.github.benmanes.caffeine.cache.Cache;
 import hudson.ExtensionList;
 import hudson.model.Item;
 import hudson.model.Label;
@@ -49,7 +49,6 @@
 import org.kohsuke.stapler.StaplerRequest;
 import org.mockito.stubbing.Answer;
 import org.openstack4j.api.OSClient;
-import org.openstack4j.openstack.compute.domain.NovaServer;
 
 import javax.annotation.Nonnull;
 import java.io.IOException;
@@ -62,18 +61,15 @@
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.anyOf;
 import static org.hamcrest.Matchers.containsString;
-import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.sameInstance;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
-import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 import static org.mockito.Matchers.any;
-import static org.mockito.Mockito.CALLS_REAL_METHODS;
 import static org.mockito.Mockito.RETURNS_DEEP_STUBS;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.times;
@@ -86,13 +82,6 @@ public class JCloudsCloudTest {
     @Rule
     public PluginTestRule j = new PluginTestRule();
 
-    @Test @Issue("JENKINS-39282") // The problem does not manifest in jenkins-test-harness - created as a regression test
-    public void guavaLeak() {
-        NovaServer server = mock(NovaServer.class, CALLS_REAL_METHODS);
-        server.id = "424242";
-        assertThat(server.toString(), containsString("424242"));
-    }
-
     @Test
     public void failToTestConnection() {
         DescriptorImpl desc = j.getCloudDescriptor();