From 1dd8b1e2dc2b7a27488000df5ea733ba2ff9410e Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Fri, 7 May 2021 16:23:41 -0400 Subject: [PATCH 1/2] =?UTF-8?q?Replacing=20Guava=E2=80=99s=20`CacheBuilder?= =?UTF-8?q?`=20with=20Java=20Platform=20equivalents?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- core/pom.xml | 5 -- .../org/kohsuke/stapler/AbstractTearOff.java | 36 ++++++++----- .../kohsuke/stapler/CachingScriptLoader.java | 47 +++++++++-------- .../java/org/kohsuke/stapler/Function.java | 15 +++--- .../java/org/kohsuke/stapler/Optional.java | 51 ------------------- .../kohsuke/stapler/ScriptLoadException.java | 5 +- 6 files changed, 57 insertions(+), 102 deletions(-) delete mode 100644 core/src/main/java/org/kohsuke/stapler/Optional.java diff --git a/core/pom.xml b/core/pom.xml index bbcf5e5781..5dc03bcf05 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -87,11 +87,6 @@ 1.8.3 true - - com.google.guava - guava - 11.0.1 - org.kohsuke asm5 diff --git a/core/src/main/java/org/kohsuke/stapler/AbstractTearOff.java b/core/src/main/java/org/kohsuke/stapler/AbstractTearOff.java index 07907e6b26..e4798c8948 100644 --- a/core/src/main/java/org/kohsuke/stapler/AbstractTearOff.java +++ b/core/src/main/java/org/kohsuke/stapler/AbstractTearOff.java @@ -23,14 +23,16 @@ package org.kohsuke.stapler; -import com.google.common.cache.Cache; -import com.google.common.cache.CacheBuilder; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import java.io.File; +import java.lang.ref.Reference; +import java.lang.ref.SoftReference; import java.net.URI; import java.net.URISyntaxException; import java.net.URL; +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; import java.util.logging.Level; import java.util.logging.Logger; import java.util.regex.Matcher; @@ -52,13 +54,13 @@ public abstract class AbstractTearOff extends Caching private static final class ExpirableCacheHit { private final long timestamp; - private final S script; + private final Reference script; ExpirableCacheHit(long timestamp, S script) { this.timestamp = timestamp; - this.script = script; + this.script = new SoftReference<>(script); } } - private final Cache> cachedScripts = CacheBuilder.newBuilder().softValues().build(); + private final Map> cachedScripts = new ConcurrentHashMap<>(); protected AbstractTearOff(MetaClass owner, Class cltClass) { this.owner = owner; @@ -122,7 +124,7 @@ public S resolveScript(String name) throws E { LOGGER.log(Level.FINE, "no timestamp associated with {0}", file); return parseScript(res); } else { - ExpirableCacheHit cached = cachedScripts.getIfPresent(res); + ExpirableCacheHit cached = cachedScripts.get(res); if (cached == null) { S script; if (LOGGER.isLoggable(Level.FINE)) { @@ -138,13 +140,23 @@ public S resolveScript(String name) throws E { } cachedScripts.put(res, new ExpirableCacheHit<>(timestamp, script)); return script; - } else if (timestamp == cached.timestamp) { - LOGGER.log(Level.FINE, "cache hit on {0}", res); - return cached.script; } else { - LOGGER.log(Level.FINE, "expired cache hit on {0}", res); - S script = parseScript(res); - cachedScripts.put(res, new ExpirableCacheHit<>(timestamp, script)); + S script; + if (timestamp == cached.timestamp) { + script = cached.script.get(); + if (script == null) { + LOGGER.log(Level.FINE, "cache hit on {0} but value collected", res); + } else { + LOGGER.log(Level.FINE, "cache hit on {0}", res); + } + } else { + LOGGER.log(Level.FINE, "expired cache hit on {0}", res); + script = null; + } + if (script == null) { + script = parseScript(res); + cachedScripts.put(res, new ExpirableCacheHit<>(timestamp, script)); + } return script; } } diff --git a/core/src/main/java/org/kohsuke/stapler/CachingScriptLoader.java b/core/src/main/java/org/kohsuke/stapler/CachingScriptLoader.java index 0bb83135c7..8e779e33f2 100644 --- a/core/src/main/java/org/kohsuke/stapler/CachingScriptLoader.java +++ b/core/src/main/java/org/kohsuke/stapler/CachingScriptLoader.java @@ -1,10 +1,11 @@ package org.kohsuke.stapler; -import com.google.common.cache.CacheBuilder; -import com.google.common.cache.CacheLoader; -import com.google.common.cache.LoadingCache; - +import java.lang.ref.Reference; +import java.lang.ref.SoftReference; import java.net.URL; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.ConcurrentHashMap; /** * Convenient base class for caching loaded scripts. @@ -22,20 +23,8 @@ public abstract class CachingScriptLoader { * * So it's important to allow Scripts to be garbage collected. * This is not an ideal fix, but it works. - * - * {@link Optional} is used as Google Collection doesn't allow null values in a map. */ - private final LoadingCache> scripts = CacheBuilder.newBuilder().softValues().build(new CacheLoader>() { - public Optional load(String from) { - try { - return Optional.create(loadScript(from)); - } catch (RuntimeException e) { - throw e; // pass through - } catch (Exception e) { - throw new ScriptLoadException(e); - } - } - }); + private final Map>> scripts = new ConcurrentHashMap<>(); /** * Locates the view script of the given name. @@ -56,10 +45,24 @@ public Optional load(String from) { * @return null if none was found. */ public S findScript(String name) throws E { - if (MetaClass.NO_CACHE) + if (MetaClass.NO_CACHE) { return loadScript(name); - else - return scripts.getUnchecked(name).get(); + } else { + Optional> sr = scripts.get(name); + S s; + if (sr == null) { // never before computed + s = null; + } else if (!sr.isPresent()) { // cached as null + return null; + } else { // cached as non-null; may or may not still have value + s = sr.get().get(); + } + if (s == null) { // needs to be computed + s = loadScript(name); + scripts.put(name, s == null ? Optional.empty() : Optional.of(new SoftReference<>(s))); + } + return s; + } } /** @@ -70,8 +73,8 @@ public S findScript(String name) throws E { /** * Discards the cached script. */ - public synchronized void clearScripts() { - scripts.invalidateAll(); + public void clearScripts() { + scripts.clear(); } protected abstract URL getResource(String name); diff --git a/core/src/main/java/org/kohsuke/stapler/Function.java b/core/src/main/java/org/kohsuke/stapler/Function.java index 63678f80bf..8d97042d0d 100644 --- a/core/src/main/java/org/kohsuke/stapler/Function.java +++ b/core/src/main/java/org/kohsuke/stapler/Function.java @@ -23,9 +23,6 @@ package org.kohsuke.stapler; -import com.google.common.cache.CacheBuilder; -import com.google.common.cache.CacheLoader; -import com.google.common.cache.LoadingCache; import org.apache.commons.lang.StringUtils; import org.kohsuke.stapler.interceptor.Interceptor; import org.kohsuke.stapler.interceptor.InterceptorAnnotation; @@ -38,7 +35,6 @@ import java.lang.invoke.MethodHandle; import java.lang.invoke.WrongMethodTypeException; import java.lang.reflect.AnnotatedElement; -import java.lang.reflect.Executable; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import java.lang.reflect.Modifier; @@ -195,7 +191,7 @@ Object bindAndInvoke(Object o, StaplerRequest req, StaplerResponse rsp, Object.. } // if the databinding method is provided, call that - Function binder = PARSE_METHODS.getUnchecked(t); + Function binder = PARSE_METHODS.get(t); if (binder!=RETURN_NULL) { arguments[i] = binder.bindAndInvoke(null,req,rsp); continue; @@ -216,7 +212,7 @@ Object bindAndInvoke(Object o, StaplerRequest req, StaplerResponse rsp, Object.. * Computing map that discovers the static 'fromStapler' method from a class. * The discovered method will be returned as a Function so that the invocation can do parameter injections. */ - private static final LoadingCache PARSE_METHODS; + private static final ClassValue PARSE_METHODS; private static final Function RETURN_NULL; static { @@ -226,8 +222,9 @@ Object bindAndInvoke(Object o, StaplerRequest req, StaplerResponse rsp, Object.. throw new AssertionError(e); // impossible } - PARSE_METHODS = CacheBuilder.newBuilder().weakKeys().build(new CacheLoader() { - public Function load(Class from) { + PARSE_METHODS = new ClassValue() { + @Override + public Function computeValue(Class from) { // MethdFunction for invoking a static method as a static method FunctionList methods = new ClassDescriptor(from).methods.name("fromStapler"); switch (methods.size()) { @@ -259,7 +256,7 @@ public Object invoke(StaplerRequest req, StaplerResponse rsp, Object o, Object.. }; } } - }); + }; } /** diff --git a/core/src/main/java/org/kohsuke/stapler/Optional.java b/core/src/main/java/org/kohsuke/stapler/Optional.java deleted file mode 100644 index 3ae2ff7a60..0000000000 --- a/core/src/main/java/org/kohsuke/stapler/Optional.java +++ /dev/null @@ -1,51 +0,0 @@ -/* - * Copyright (c) 2004-2010, Kohsuke Kawaguchi - * All rights reserved. - * - * Redistribution and use in source and binary forms, with or without modification, are permitted provided - * that the following conditions are met: - * - * * Redistributions of source code must retain the above copyright notice, this list of - * conditions and the following disclaimer. - * * Redistributions in binary form must reproduce the above copyright notice, this list of - * conditions and the following disclaimer in the documentation and/or other materials - * provided with the distribution. - * - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS - * OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY - * AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS - * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES - * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, - * OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER - * IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF - * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - */ - -package org.kohsuke.stapler; - -/** - * Represents a T value or no value at all. Used to work around the lack of null value in the computing map. - * - * @author Kohsuke Kawaguchi - */ -final class Optional { - private final T value; - - Optional(T value) { - this.value = value; - } - - public T get() { return value; } - - - private static Optional NONE = new Optional(null); - - public static Optional none() { - return NONE; - } - - public static Optional create(T value) { - if (value==null) return NONE; // cache - return new Optional(value); - } -} diff --git a/core/src/main/java/org/kohsuke/stapler/ScriptLoadException.java b/core/src/main/java/org/kohsuke/stapler/ScriptLoadException.java index accce3a53b..78d872dded 100644 --- a/core/src/main/java/org/kohsuke/stapler/ScriptLoadException.java +++ b/core/src/main/java/org/kohsuke/stapler/ScriptLoadException.java @@ -24,10 +24,9 @@ package org.kohsuke.stapler; /** - * Indicates a failure to load a script. - * - * @author Kohsuke Kawaguchi + * @deprecated No longer used. */ +@Deprecated public class ScriptLoadException extends RuntimeException { public ScriptLoadException(String message, Throwable cause) { super(message, cause); From 3338be17c99bed936f1e6b60e35ff69d0f9d36d3 Mon Sep 17 00:00:00 2001 From: Jesse Glick Date: Fri, 7 May 2021 16:34:16 -0400 Subject: [PATCH 2/2] `URL`-as-map-key bug was actually present before, but SpotBugs did not grok `CacheBuilder` --- .../main/java/org/kohsuke/stapler/AbstractTearOff.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/org/kohsuke/stapler/AbstractTearOff.java b/core/src/main/java/org/kohsuke/stapler/AbstractTearOff.java index e4798c8948..c109d198eb 100644 --- a/core/src/main/java/org/kohsuke/stapler/AbstractTearOff.java +++ b/core/src/main/java/org/kohsuke/stapler/AbstractTearOff.java @@ -60,7 +60,7 @@ private static final class ExpirableCacheHit { this.script = new SoftReference<>(script); } } - private final Map> cachedScripts = new ConcurrentHashMap<>(); + private final Map> cachedScripts = new ConcurrentHashMap<>(); protected AbstractTearOff(MetaClass owner, Class cltClass) { this.owner = owner; @@ -124,7 +124,7 @@ public S resolveScript(String name) throws E { LOGGER.log(Level.FINE, "no timestamp associated with {0}", file); return parseScript(res); } else { - ExpirableCacheHit cached = cachedScripts.get(res); + ExpirableCacheHit cached = cachedScripts.get(res.toString()); if (cached == null) { S script; if (LOGGER.isLoggable(Level.FINE)) { @@ -138,7 +138,7 @@ public S resolveScript(String name) throws E { LOGGER.log(Level.FINE, "cache miss on {0}", res); script = parseScript(res); } - cachedScripts.put(res, new ExpirableCacheHit<>(timestamp, script)); + cachedScripts.put(res.toString(), new ExpirableCacheHit<>(timestamp, script)); return script; } else { S script; @@ -155,7 +155,7 @@ public S resolveScript(String name) throws E { } if (script == null) { script = parseScript(res); - cachedScripts.put(res, new ExpirableCacheHit<>(timestamp, script)); + cachedScripts.put(res.toString(), new ExpirableCacheHit<>(timestamp, script)); } return script; }