From dc2cdd90450ca875bce864e1f89f2f88f08c4044 Mon Sep 17 00:00:00 2001 From: Les Hazlewood <121180+lhazlewood@users.noreply.github.com> Date: Tue, 16 Jan 2024 18:36:09 -0800 Subject: [PATCH] Blend of pre-0.11.0 behavior that cached implementation instances and post-0.11.0 behavior using the JDK ServiceLoader to find/create instances of an SPI interface. This change: - Reinstates the <= 0.10.x behavior of caching application singleton service implementation instances in a thread-safe reference (previously an AtomicReference, but in this change, a ConcurrentMap). If an app singleton instance is cached and found, it is returned to be (re)used immediately when requested. This is ok for JJWT's purposes because all service implementations instances must be thread-safe application singletons by API contract/design, so caching them for repeated use is fine. - Ensures that only if a service implementation instance is not in the app singleton cache, a new instance is located/created using a new JDK ServiceLoader instance, which doesn't require thread-safe considerations since it is used only in a single-threaded model for the short time it is used to discover a service implementation. This PR/change removes the post-0.11.0 concurrent cache of ServiceLoader instances since they themselves are not designed to be thread-safe. - Ensures that if a ServiceLoader discovers an implementation and returns a new instance, that instance is then cached as an application singleton in the aforementioned ConcurrentMap for continued reuse. - Renames Services#loadFirst to Services#get to more accurately reflect calling expectations: The fact that any 'loading' via the ServiceLoader may occur is not important for Services callers, and the previous method name was unnecessarily exposing internal implementation concepts. This is safe to do in a point release (0.12.3 -> 0.12.4) because the Services class and its methods, while public, are in the `impl` module, only to be used internally for JJWT's purpose and never intended to be used by application developers. - Updates all test methods to use the renamed method accordingly. --- .../jsonwebtoken/impl/DefaultJwtBuilder.java | 2 +- .../impl/DefaultJwtParserBuilder.java | 2 +- .../impl/io/AbstractParserBuilder.java | 2 +- .../io/jsonwebtoken/impl/lang/Services.java | 100 ++++++------------ .../impl/security/JwksBridge.java | 2 +- .../groovy/io/jsonwebtoken/JwtsTest.groovy | 4 +- .../jsonwebtoken/RFC7515AppendixETest.groovy | 4 +- .../groovy/io/jsonwebtoken/RFC7797Test.groovy | 4 +- .../impl/DefaultJwtBuilderTest.groovy | 6 +- .../impl/DefaultJwtParserTest.groovy | 2 +- .../io/jsonwebtoken/impl/RfcTests.groovy | 2 +- .../impl/lang/ServicesTest.groovy | 25 ++--- .../impl/security/RFC7518AppendixCTest.groovy | 2 +- 13 files changed, 57 insertions(+), 100 deletions(-) diff --git a/impl/src/main/java/io/jsonwebtoken/impl/DefaultJwtBuilder.java b/impl/src/main/java/io/jsonwebtoken/impl/DefaultJwtBuilder.java index b6b178c46..ef41c7aec 100644 --- a/impl/src/main/java/io/jsonwebtoken/impl/DefaultJwtBuilder.java +++ b/impl/src/main/java/io/jsonwebtoken/impl/DefaultJwtBuilder.java @@ -508,7 +508,7 @@ public String compact() { if (this.serializer == null) { // try to find one based on the services available //noinspection unchecked - json(Services.loadFirst(Serializer.class)); + json(Services.get(Serializer.class)); } if (!Collections.isEmpty(claims)) { // normalize so we have one object to deal with: diff --git a/impl/src/main/java/io/jsonwebtoken/impl/DefaultJwtParserBuilder.java b/impl/src/main/java/io/jsonwebtoken/impl/DefaultJwtParserBuilder.java index 9b57af0c4..192904807 100644 --- a/impl/src/main/java/io/jsonwebtoken/impl/DefaultJwtParserBuilder.java +++ b/impl/src/main/java/io/jsonwebtoken/impl/DefaultJwtParserBuilder.java @@ -370,7 +370,7 @@ public JwtParser build() { if (this.deserializer == null) { //noinspection unchecked - json(Services.loadFirst(Deserializer.class)); + json(Services.get(Deserializer.class)); } if (this.signingKeyResolver != null && this.signatureVerificationKey != null) { String msg = "Both a 'signingKeyResolver and a 'verifyWith' key cannot be configured. " + diff --git a/impl/src/main/java/io/jsonwebtoken/impl/io/AbstractParserBuilder.java b/impl/src/main/java/io/jsonwebtoken/impl/io/AbstractParserBuilder.java index b7d8eb78d..b5549d599 100644 --- a/impl/src/main/java/io/jsonwebtoken/impl/io/AbstractParserBuilder.java +++ b/impl/src/main/java/io/jsonwebtoken/impl/io/AbstractParserBuilder.java @@ -50,7 +50,7 @@ public B json(Deserializer> reader) { public final Parser build() { if (this.deserializer == null) { //noinspection unchecked - this.deserializer = Services.loadFirst(Deserializer.class); + this.deserializer = Services.get(Deserializer.class); } return doBuild(); } diff --git a/impl/src/main/java/io/jsonwebtoken/impl/lang/Services.java b/impl/src/main/java/io/jsonwebtoken/impl/lang/Services.java index cd9c2f11a..6125ebc09 100644 --- a/impl/src/main/java/io/jsonwebtoken/impl/lang/Services.java +++ b/impl/src/main/java/io/jsonwebtoken/impl/lang/Services.java @@ -15,25 +15,24 @@ */ package io.jsonwebtoken.impl.lang; +import io.jsonwebtoken.lang.Arrays; import io.jsonwebtoken.lang.Assert; -import java.util.ArrayList; +import java.util.Iterator; import java.util.List; import java.util.ServiceLoader; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; -import static io.jsonwebtoken.lang.Collections.arrayToList; - /** * Helper class for loading services from the classpath, using a {@link ServiceLoader}. Decouples loading logic for * better separation of concerns and testability. */ public final class Services { - private static ConcurrentMap, ServiceLoader> SERVICE_CACHE = new ConcurrentHashMap<>(); + private static final ConcurrentMap, Object> SERVICES = new ConcurrentHashMap<>(); - private static final List CLASS_LOADER_ACCESSORS = arrayToList(new ClassLoaderAccessor[] { + private static final List CLASS_LOADER_ACCESSORS = Arrays.asList(new ClassLoaderAccessor[]{ new ClassLoaderAccessor() { @Override public ClassLoader getClassLoader() { @@ -54,86 +53,57 @@ public ClassLoader getClassLoader() { } }); - private Services() {} - - /** - * Loads and instantiates all service implementation of the given SPI class and returns them as a List. - * - * @param spi The class of the Service Provider Interface - * @param The type of the SPI - * @return An unmodifiable list with an instance of all available implementations of the SPI. No guarantee is given - * on the order of implementations, if more than one. - */ - public static List loadAll(Class spi) { - Assert.notNull(spi, "Parameter 'spi' must not be null."); - - ServiceLoader serviceLoader = serviceLoader(spi); - if (serviceLoader != null) { - - List implementations = new ArrayList<>(); - for (T implementation : serviceLoader) { - implementations.add(implementation); - } - return implementations; - } - - throw new UnavailableImplementationException(spi); + private Services() { } /** - * Loads the first available implementation the given SPI class from the classpath. Uses the {@link ServiceLoader} - * to find implementations. When multiple implementations are available it will return the first one that it - * encounters. There is no guarantee with regard to ordering. + * Returns the first available implementation for the given SPI class, checking an internal thread-safe cache first, + * and, if not found, using a {@link ServiceLoader} to find implementations. When multiple implementations are + * available it will return the first one that it encounters. There is no guarantee with regard to ordering. * * @param spi The class of the Service Provider Interface * @param The type of the SPI - * @return A new instance of the service. - * @throws UnavailableImplementationException When no implementation the SPI is available on the classpath. + * @return The first available instance of the service. + * @throws UnavailableImplementationException When no implementation of the SPI class can be found. */ - public static T loadFirst(Class spi) { - Assert.notNull(spi, "Parameter 'spi' must not be null."); - - ServiceLoader serviceLoader = serviceLoader(spi); - if (serviceLoader != null) { - return serviceLoader.iterator().next(); + public static T get(Class spi) { + // TODO: JDK8, replace this find/putIfAbsent logic with ConcurrentMap.computeIfAbsent + T instance = findCached(spi); + if (instance == null) { + instance = loadFirst(spi); // throws UnavailableImplementationException if not found, which is what we want + SERVICES.putIfAbsent(spi, instance); // cache if not already cached } - - throw new UnavailableImplementationException(spi); + return instance; } - /** - * Returns a ServiceLoader for spi class, checking multiple classloaders. The ServiceLoader - * will be cached if it contains at least one implementation of the spi class.
- * - * NOTE: Only the first Serviceloader will be cached. - * @param spi The interface or abstract class representing the service loader. - * @return A service loader, or null if no implementations are found - * @param The type of the SPI. - */ - private static ServiceLoader serviceLoader(Class spi) { - // TODO: JDK8, replace this get/putIfAbsent logic with ConcurrentMap.computeIfAbsent - ServiceLoader serviceLoader = (ServiceLoader) SERVICE_CACHE.get(spi); - if (serviceLoader != null) { - return serviceLoader; + private static T findCached(Class spi) { + Assert.notNull(spi, "Service interface cannot be null."); + Object obj = SERVICES.get(spi); + if (obj != null) { + return Assert.isInstanceOf(spi, obj, "Unexpected cached service implementation type."); } + return null; + } - for (ClassLoaderAccessor classLoaderAccessor : CLASS_LOADER_ACCESSORS) { - serviceLoader = ServiceLoader.load(spi, classLoaderAccessor.getClassLoader()); - if (serviceLoader.iterator().hasNext()) { - SERVICE_CACHE.putIfAbsent(spi, serviceLoader); - return serviceLoader; + private static T loadFirst(Class spi) { + for (ClassLoaderAccessor accessor : CLASS_LOADER_ACCESSORS) { + ServiceLoader loader = ServiceLoader.load(spi, accessor.getClassLoader()); + Assert.stateNotNull(loader, "JDK ServiceLoader#load should never return null."); + Iterator i = loader.iterator(); + Assert.stateNotNull(i, "JDK ServiceLoader#iterator() should never return null."); + if (i.hasNext()) { + return i.next(); } } - - return null; + throw new UnavailableImplementationException(spi); } /** - * Clears internal cache of ServiceLoaders. This is useful when testing, or for applications that dynamically + * Clears internal cache of service singletons. This is useful when testing, or for applications that dynamically * change classloaders. */ public static void reload() { - SERVICE_CACHE.clear(); + SERVICES.clear(); } private interface ClassLoaderAccessor { diff --git a/impl/src/main/java/io/jsonwebtoken/impl/security/JwksBridge.java b/impl/src/main/java/io/jsonwebtoken/impl/security/JwksBridge.java index 8b5de6a80..4ab494c08 100644 --- a/impl/src/main/java/io/jsonwebtoken/impl/security/JwksBridge.java +++ b/impl/src/main/java/io/jsonwebtoken/impl/security/JwksBridge.java @@ -32,7 +32,7 @@ private JwksBridge() { @SuppressWarnings({"unchecked", "unused"}) // used via reflection by io.jsonwebtoken.security.Jwks public static String UNSAFE_JSON(Jwk jwk) { - Serializer> serializer = Services.loadFirst(Serializer.class); + Serializer> serializer = Services.get(Serializer.class); Assert.stateNotNull(serializer, "Serializer lookup failed. Ensure JSON impl .jar is in the runtime classpath."); NamedSerializer ser = new NamedSerializer("JWK", serializer); ByteArrayOutputStream out = new ByteArrayOutputStream(512); diff --git a/impl/src/test/groovy/io/jsonwebtoken/JwtsTest.groovy b/impl/src/test/groovy/io/jsonwebtoken/JwtsTest.groovy index 95e2e8e07..2f1c432c8 100644 --- a/impl/src/test/groovy/io/jsonwebtoken/JwtsTest.groovy +++ b/impl/src/test/groovy/io/jsonwebtoken/JwtsTest.groovy @@ -75,7 +75,7 @@ class JwtsTest { } static def toJson(def o) { - def serializer = Services.loadFirst(Serializer) + def serializer = Services.get(Serializer) def out = new ByteArrayOutputStream() serializer.serialize(o, out) return Strings.utf8(out.toByteArray()) @@ -1192,7 +1192,7 @@ class JwtsTest { int j = jws.lastIndexOf('.') def b64 = jws.substring(i, j) def json = Strings.utf8(Decoders.BASE64URL.decode(b64)) - def deser = Services.loadFirst(Deserializer) + def deser = Services.get(Deserializer) def m = deser.deserialize(new StringReader(json)) as Map assertEquals aud, m.get('aud') // single string value diff --git a/impl/src/test/groovy/io/jsonwebtoken/RFC7515AppendixETest.groovy b/impl/src/test/groovy/io/jsonwebtoken/RFC7515AppendixETest.groovy index 52d1e4a28..847a8022d 100644 --- a/impl/src/test/groovy/io/jsonwebtoken/RFC7515AppendixETest.groovy +++ b/impl/src/test/groovy/io/jsonwebtoken/RFC7515AppendixETest.groovy @@ -29,8 +29,8 @@ import static org.junit.Assert.fail class RFC7515AppendixETest { - static final Serializer> serializer = Services.loadFirst(Serializer) - static final Deserializer> deserializer = Services.loadFirst(Deserializer) + static final Serializer> serializer = Services.get(Serializer) + static final Deserializer> deserializer = Services.get(Deserializer) static byte[] ser(def value) { ByteArrayOutputStream baos = new ByteArrayOutputStream(512) diff --git a/impl/src/test/groovy/io/jsonwebtoken/RFC7797Test.groovy b/impl/src/test/groovy/io/jsonwebtoken/RFC7797Test.groovy index deb24973b..b32036f89 100644 --- a/impl/src/test/groovy/io/jsonwebtoken/RFC7797Test.groovy +++ b/impl/src/test/groovy/io/jsonwebtoken/RFC7797Test.groovy @@ -100,11 +100,9 @@ class RFC7797Test { def claims = Jwts.claims().subject('me').build() ByteArrayOutputStream out = new ByteArrayOutputStream() - Services.loadFirst(Serializer).serialize(claims, out) + Services.get(Serializer).serialize(claims, out) byte[] content = out.toByteArray() - //byte[] content = Services.loadFirst(Serializer).serialize(claims) - String s = Jwts.builder().signWith(key).content(content).encodePayload(false).compact() // But verify with 3 types of sources: string, byte array, and two different kinds of InputStreams: diff --git a/impl/src/test/groovy/io/jsonwebtoken/impl/DefaultJwtBuilderTest.groovy b/impl/src/test/groovy/io/jsonwebtoken/impl/DefaultJwtBuilderTest.groovy index 3d0dfa949..591b4b07d 100644 --- a/impl/src/test/groovy/io/jsonwebtoken/impl/DefaultJwtBuilderTest.groovy +++ b/impl/src/test/groovy/io/jsonwebtoken/impl/DefaultJwtBuilderTest.groovy @@ -45,7 +45,7 @@ class DefaultJwtBuilderTest { private DefaultJwtBuilder builder private static byte[] serialize(Map map) { - def serializer = Services.loadFirst(Serializer) + def serializer = Services.get(Serializer) ByteArrayOutputStream out = new ByteArrayOutputStream(512) serializer.serialize(map, out) return out.toByteArray() @@ -53,7 +53,7 @@ class DefaultJwtBuilderTest { private static Map deser(byte[] data) { def reader = Streams.reader(data) - Map m = Services.loadFirst(Deserializer).deserialize(reader) as Map + Map m = Services.get(Deserializer).deserialize(reader) as Map return m } @@ -749,7 +749,7 @@ class DefaultJwtBuilderTest { // so we need to check the raw payload: def encoded = new JwtTokenizer().tokenize(Streams.reader(jwt)).getPayload() byte[] bytes = Decoders.BASE64URL.decode(encoded) - def claims = Services.loadFirst(Deserializer).deserialize(Streams.reader(bytes)) + def claims = Services.get(Deserializer).deserialize(Streams.reader(bytes)) assertEquals two, claims.aud } diff --git a/impl/src/test/groovy/io/jsonwebtoken/impl/DefaultJwtParserTest.groovy b/impl/src/test/groovy/io/jsonwebtoken/impl/DefaultJwtParserTest.groovy index 8084b32f2..4c41142c2 100644 --- a/impl/src/test/groovy/io/jsonwebtoken/impl/DefaultJwtParserTest.groovy +++ b/impl/src/test/groovy/io/jsonwebtoken/impl/DefaultJwtParserTest.groovy @@ -54,7 +54,7 @@ class DefaultJwtParserTest { } private static byte[] serialize(Map map) { - def serializer = Services.loadFirst(Serializer) + def serializer = Services.get(Serializer) ByteArrayOutputStream out = new ByteArrayOutputStream(512) serializer.serialize(map, out) return out.toByteArray() diff --git a/impl/src/test/groovy/io/jsonwebtoken/impl/RfcTests.groovy b/impl/src/test/groovy/io/jsonwebtoken/impl/RfcTests.groovy index 548cf408d..99cf1dd12 100644 --- a/impl/src/test/groovy/io/jsonwebtoken/impl/RfcTests.groovy +++ b/impl/src/test/groovy/io/jsonwebtoken/impl/RfcTests.groovy @@ -38,7 +38,7 @@ class RfcTests { static final Map jsonToMap(String json) { Reader r = new CharSequenceReader(json) - Map m = Services.loadFirst(Deserializer).deserialize(r) as Map + Map m = Services.get(Deserializer).deserialize(r) as Map return m } diff --git a/impl/src/test/groovy/io/jsonwebtoken/impl/lang/ServicesTest.groovy b/impl/src/test/groovy/io/jsonwebtoken/impl/lang/ServicesTest.groovy index 8af9300e7..0f880d2ba 100644 --- a/impl/src/test/groovy/io/jsonwebtoken/impl/lang/ServicesTest.groovy +++ b/impl/src/test/groovy/io/jsonwebtoken/impl/lang/ServicesTest.groovy @@ -20,32 +20,21 @@ import io.jsonwebtoken.impl.DefaultStubService import org.junit.After import org.junit.Test -import static org.junit.Assert.* +import static org.junit.Assert.assertEquals +import static org.junit.Assert.assertNotNull class ServicesTest { @Test void testSuccessfulLoading() { - def factory = Services.loadFirst(StubService) - assertNotNull factory - assertEquals(DefaultStubService, factory.class) + def service = Services.get(StubService) + assertNotNull service + assertEquals(DefaultStubService, service.class) } @Test(expected = UnavailableImplementationException) - void testLoadFirstUnavailable() { - Services.loadFirst(NoService.class) - } - - @Test - void testLoadAllAvailable() { - def list = Services.loadAll(StubService.class) - assertEquals 1, list.size() - assertTrue list[0] instanceof StubService - } - - @Test(expected = UnavailableImplementationException) - void testLoadAllUnavailable() { - Services.loadAll(NoService.class) + void testLoadUnavailable() { + Services.get(NoService.class) } @Test diff --git a/impl/src/test/groovy/io/jsonwebtoken/impl/security/RFC7518AppendixCTest.groovy b/impl/src/test/groovy/io/jsonwebtoken/impl/security/RFC7518AppendixCTest.groovy index abdb1a94e..b1d068c7a 100644 --- a/impl/src/test/groovy/io/jsonwebtoken/impl/security/RFC7518AppendixCTest.groovy +++ b/impl/src/test/groovy/io/jsonwebtoken/impl/security/RFC7518AppendixCTest.groovy @@ -43,7 +43,7 @@ class RFC7518AppendixCTest { } private static final Map fromJson(String s) { - return Services.loadFirst(Deserializer).deserialize(new StringReader(s)) as Map + return Services.get(Deserializer).deserialize(new StringReader(s)) as Map } private static EcPrivateJwk readJwk(String json) {