From 3680d0518e1ceadacfc8d32cfb05aec7516c386e Mon Sep 17 00:00:00 2001 From: Antoine Sabot-Durand Date: Thu, 16 Jul 2020 12:25:36 +0200 Subject: [PATCH] microprofile-health-128 org.eclipse.microprofile.health.HealthCheckResponse leaks fixes #128 with multiple providers support addition Signed-off-by: Antoine Sabot-Durand --- api/pom.xml | 21 ++- .../health/HealthCheckResponse.java | 95 +------------ .../HealthCheckResponseProviderResolver.java | 129 ++++++++++++++++++ .../microprofile/health/SecurityActions.java | 75 ++++++++++ .../spi/HealthCheckResponseProvider.java | 4 +- .../health/DummyResponseProvider1.java | 31 +++++ .../health/DummyResponseProvider2.java | 31 +++++ ...althCheckResponseProviderResolverTest.java | 94 +++++++++++++ .../test/resources/META-INF/services/dummy | 0 pom.xml | 9 ++ tck/pom.xml | 2 - 11 files changed, 398 insertions(+), 93 deletions(-) create mode 100644 api/src/main/java/org/eclipse/microprofile/health/HealthCheckResponseProviderResolver.java create mode 100644 api/src/main/java/org/eclipse/microprofile/health/SecurityActions.java create mode 100644 api/src/test/java/org/eclipse/microprofile/health/DummyResponseProvider1.java create mode 100644 api/src/test/java/org/eclipse/microprofile/health/DummyResponseProvider2.java create mode 100644 api/src/test/java/org/eclipse/microprofile/health/HealthCheckResponseProviderResolverTest.java create mode 100644 api/src/test/resources/META-INF/services/dummy diff --git a/api/pom.xml b/api/pom.xml index 31dda9e..9978f79 100644 --- a/api/pom.xml +++ b/api/pom.xml @@ -37,6 +37,10 @@ org.osgi.annotation.versioning provided + + org.testng + testng + @@ -97,13 +101,28 @@ - baseline + baseline baseline + + org.apache.maven.plugins + maven-surefire-plugin + ${maven-surefire-plugin.version} + + + default-test + + + ${project.build.testOutputDirectory}/META-INF/services/ + + + + + diff --git a/api/src/main/java/org/eclipse/microprofile/health/HealthCheckResponse.java b/api/src/main/java/org/eclipse/microprofile/health/HealthCheckResponse.java index 1ad47ed..349ada8 100644 --- a/api/src/main/java/org/eclipse/microprofile/health/HealthCheckResponse.java +++ b/api/src/main/java/org/eclipse/microprofile/health/HealthCheckResponse.java @@ -24,19 +24,15 @@ import org.eclipse.microprofile.health.spi.HealthCheckResponseProvider; -import java.security.AccessController; -import java.security.PrivilegedAction; import java.util.Map; import java.util.Optional; -import java.util.ServiceLoader; -import java.util.logging.Level; import java.util.logging.Logger; /** * The response to a health check invocation. *

* The {@link HealthCheckResponse} class is reserved for an extension by implementation providers. - * An application should use one of the static methods to create a Response instance using a + * An application should use one of the static methods to create a Response instance using a * {@link HealthCheckResponseBuilder}. * When used on the consuming end, The class can also be instantiated directly. *

@@ -45,8 +41,6 @@ public class HealthCheckResponse { private static final Logger LOGGER = Logger.getLogger(HealthCheckResponse.class.getName()); - private static volatile HealthCheckResponseProvider provider = null; - private final String name; private final Status status; @@ -78,9 +72,10 @@ public HealthCheckResponse() { * Used by OSGi environment where the service loader pattern is not supported. * * @param provider the provider instance to use. + * @deprecated use {{@link HealthCheckResponseProviderResolver#setProvider}} instead */ public static void setResponseProvider(HealthCheckResponseProvider provider) { - HealthCheckResponse.provider = provider; + HealthCheckResponseProviderResolver.setProvider(provider); } /** @@ -91,7 +86,7 @@ public static void setResponseProvider(HealthCheckResponseProvider provider) { */ public static HealthCheckResponseBuilder named(String name) { - return getProvider().createResponseBuilder().name(name); + return HealthCheckResponseProviderResolver.getProvider().createResponseBuilder().name(name); } /** @@ -102,12 +97,12 @@ public static HealthCheckResponseBuilder named(String name) { * @return a new, empty health check builder */ public static HealthCheckResponseBuilder builder() { - return getProvider().createResponseBuilder(); + return HealthCheckResponseProviderResolver.getProvider().createResponseBuilder(); } /** * Creates a successful health check with a name. - * + * * @param name the check name * @return a new sucessful health check response with a name */ @@ -117,7 +112,7 @@ public static HealthCheckResponse up(String name) { /** * Creates a failed health check with a name. - * + * * @param name the check name * @return a new failed health check response with a name */ @@ -125,25 +120,6 @@ public static HealthCheckResponse down(String name) { return HealthCheckResponse.named(name).down().build(); } - private static HealthCheckResponseProvider getProvider() { - if (provider == null) { - synchronized (HealthCheckResponse.class) { - if (provider != null) { - return provider; - } - - HealthCheckResponseProvider newInstance = find(HealthCheckResponseProvider.class); - - if (newInstance == null) { - throw new IllegalStateException("No HealthCheckResponseProvider implementation found!"); - } - - provider = newInstance; - } - } - return provider; - } - // the actual contract public enum Status {UP, DOWN} @@ -160,61 +136,4 @@ public Optional> getData() { return data; } - private static T find(Class service) { - - T serviceInstance = find(service, HealthCheckResponse.getContextClassLoader()); - - // alternate classloader - if (null == serviceInstance) { - serviceInstance = find(service, HealthCheckResponse.class.getClassLoader()); - } - - // service cannot be found - if (null == serviceInstance) { - throw new IllegalStateException("Unable to find service " + service.getName()); - } - - return serviceInstance; - } - - private static T find(Class service, ClassLoader cl) { - - T serviceInstance = null; - - try { - ServiceLoader services = ServiceLoader.load(service, cl); - - for (T spi : services) { - if (serviceInstance != null) { - throw new IllegalStateException( - "Multiple service implementations found: " - + spi.getClass().getName() + " and " - + serviceInstance.getClass().getName()); - } - serviceInstance = spi; - } - } - catch (Throwable t) { - LOGGER.log(Level.SEVERE, "Error loading service " + service.getName() + ".", t); - } - - return serviceInstance; - } - - - private static ClassLoader getContextClassLoader() { - return AccessController.doPrivileged((PrivilegedAction) () -> { - ClassLoader cl = null; - try { - cl = Thread.currentThread().getContextClassLoader(); - } - catch (SecurityException ex) { - LOGGER.log( - Level.WARNING, - "Unable to get context classloader instance.", - ex); - } - return cl; - }); - } } diff --git a/api/src/main/java/org/eclipse/microprofile/health/HealthCheckResponseProviderResolver.java b/api/src/main/java/org/eclipse/microprofile/health/HealthCheckResponseProviderResolver.java new file mode 100644 index 0000000..71b6f85 --- /dev/null +++ b/api/src/main/java/org/eclipse/microprofile/health/HealthCheckResponseProviderResolver.java @@ -0,0 +1,129 @@ +/* + * Copyright (c) 2020 Contributors to the Eclipse Foundation + * + * See the NOTICES file(s) distributed with this work for additional + * information regarding copyright ownership. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * You may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * SPDX-License-Identifier: Apache-2.0 + * + */ + +package org.eclipse.microprofile.health; + +import java.util.Collections; +import java.util.ServiceConfigurationError; +import java.util.ServiceLoader; +import java.util.Set; +import java.util.HashSet; +import java.util.function.Predicate; + +import org.eclipse.microprofile.health.spi.HealthCheckResponseProvider; + + +/** + * This class an helper to retrieve the Microprofile HealthCheck implementation + * by using Java {@link ServiceLoader} api. + * It allows the usage of multiple implementations at the same time. + * + * @author Antoine Sabot-Durand + * @since 3.0 + */ +public class HealthCheckResponseProviderResolver { + + private static final Object LOCK = new Object(); + + private static volatile HealthCheckResponseProvider provider = null; + + private static volatile Set discoveredProviders = null; + + private static volatile Predicate providerPredicate = p -> true; + + protected HealthCheckResponseProviderResolver() { + } + + /** + * @return the selected {@link HealthCheckResponseProvider} in cache. If not available it tries to resolve it first + */ + public static HealthCheckResponseProvider getProvider() { + if (provider == null) { + if (discoveredProviders == null) { + synchronized (LOCK) { + if (discoveredProviders == null) { + findAllProviders(); + } + } + } + provider = discoveredProviders.stream() + .filter(providerPredicate) + .findFirst().orElseThrow(() -> new IllegalStateException("No HealthCheckResponseProvider implementation found!")); + } + return provider; + } + + /** + * + * Used to manually set the {@link HealthCheckResponseProvider}. + * Also used by OSGi environment where the service loader pattern is not supported. + * + * @param provider the provider instance to use. + */ + public static void setProvider(HealthCheckResponseProvider provider) { + HealthCheckResponseProviderResolver.provider = provider; + } + + /** + * + * Other way than {@link #setProvider} to set the {@link HealthCheckResponseProvider} + * + * @param providerPredicate a predicate to choose the matching provider + */ + public static void setProviderPredicate(Predicate providerPredicate) { + HealthCheckResponseProviderResolver.providerPredicate = providerPredicate; + setProvider(null); + } + + private static void findAllProviders() { + ServiceLoader providerLoader; + Set providers = new HashSet<>(); + + Class clazz = HealthCheckResponseProvider.class; + ClassLoader cl = SecurityActions.getContextClassLoader(); + if (cl == null) { + cl = clazz.getClassLoader(); + } + providerLoader = SecurityActions.loadService(HealthCheckResponseProvider.class, cl); + + if (!providerLoader.iterator().hasNext()) { + throw new IllegalStateException("Unable to locate HealthCheckResponseProvider"); + } + + try { + providerLoader.forEach(providers::add); + } + catch (ServiceConfigurationError e) { + throw new IllegalStateException(e); + } + discoveredProviders = Collections.unmodifiableSet(providers); + } + + protected static void reset() { + provider = null; + discoveredProviders =null; + providerPredicate = p -> true; + + + } + +} diff --git a/api/src/main/java/org/eclipse/microprofile/health/SecurityActions.java b/api/src/main/java/org/eclipse/microprofile/health/SecurityActions.java new file mode 100644 index 0000000..8b0e835 --- /dev/null +++ b/api/src/main/java/org/eclipse/microprofile/health/SecurityActions.java @@ -0,0 +1,75 @@ +/* + * Copyright (c) 2020 Contributors to the Eclipse Foundation + * + * See the NOTICES file(s) distributed with this work for additional + * information regarding copyright ownership. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * You may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * SPDX-License-Identifier: Apache-2.0 + * + */ + +package org.eclipse.microprofile.health; + +import java.security.AccessController; +import java.security.PrivilegedAction; +import java.util.ServiceLoader; +import java.util.logging.Level; +import java.util.logging.Logger; + +/** + * + * This utility class is used to optimize invocation made through the SecurityManager + * + * @author Antoine Sabot-durand + */ + +final class SecurityActions { + + private static final Logger LOGGER = Logger.getLogger(SecurityActions.class.getName()); + + private SecurityActions() { + + } + + /** + * Obtaining a service loader with Security Manager support + * + * @param service class to load + * @param classLoader to use + * @param serice type + * @return the service loader + */ + static ServiceLoader loadService(Class service, ClassLoader classLoader) { + return AccessController.doPrivileged( + (PrivilegedAction>) () -> ServiceLoader.load(service, classLoader) + ); + } + + static ClassLoader getContextClassLoader() { + return AccessController.doPrivileged((PrivilegedAction) () -> { + ClassLoader cl = null; + try { + cl = Thread.currentThread().getContextClassLoader(); + } + catch (SecurityException ex) { + LOGGER.log( + Level.WARNING, + "Unable to get context classloader instance.", + ex); + } + return cl; + }); + } +} diff --git a/api/src/main/java/org/eclipse/microprofile/health/spi/HealthCheckResponseProvider.java b/api/src/main/java/org/eclipse/microprofile/health/spi/HealthCheckResponseProvider.java index 6b49f63..9b5548d 100644 --- a/api/src/main/java/org/eclipse/microprofile/health/spi/HealthCheckResponseProvider.java +++ b/api/src/main/java/org/eclipse/microprofile/health/spi/HealthCheckResponseProvider.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2017 Contributors to the Eclipse Foundation + * Copyright (c) 2017-2020 Contributors to the Eclipse Foundation * * See the NOTICES file(s) distributed with this work for additional * information regarding copyright ownership. @@ -35,7 +35,7 @@ public interface HealthCheckResponseProvider { /** * Provides an implementation of {@link HealthCheckResponseBuilder}. - * + * * @return a vendor specific implemenatation of {@link HealthCheckResponseBuilder} */ HealthCheckResponseBuilder createResponseBuilder(); diff --git a/api/src/test/java/org/eclipse/microprofile/health/DummyResponseProvider1.java b/api/src/test/java/org/eclipse/microprofile/health/DummyResponseProvider1.java new file mode 100644 index 0000000..2ad11ca --- /dev/null +++ b/api/src/test/java/org/eclipse/microprofile/health/DummyResponseProvider1.java @@ -0,0 +1,31 @@ +/* + * Copyright (c) 2020 Contributors to the Eclipse Foundation + * + * See the NOTICES file(s) distributed with this work for additional + * information regarding copyright ownership. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * You may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * SPDX-License-Identifier: Apache-2.0 + * + */ +package org.eclipse.microprofile.health; + +import org.eclipse.microprofile.health.spi.HealthCheckResponseProvider; + +public class DummyResponseProvider1 implements HealthCheckResponseProvider { + @Override + public HealthCheckResponseBuilder createResponseBuilder() { + return null; + } +} diff --git a/api/src/test/java/org/eclipse/microprofile/health/DummyResponseProvider2.java b/api/src/test/java/org/eclipse/microprofile/health/DummyResponseProvider2.java new file mode 100644 index 0000000..0516d56 --- /dev/null +++ b/api/src/test/java/org/eclipse/microprofile/health/DummyResponseProvider2.java @@ -0,0 +1,31 @@ +/* + * Copyright (c) 2020 Contributors to the Eclipse Foundation + * + * See the NOTICES file(s) distributed with this work for additional + * information regarding copyright ownership. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * You may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * SPDX-License-Identifier: Apache-2.0 + * + */ +package org.eclipse.microprofile.health; + +import org.eclipse.microprofile.health.spi.HealthCheckResponseProvider; + +public class DummyResponseProvider2 implements HealthCheckResponseProvider { + @Override + public HealthCheckResponseBuilder createResponseBuilder() { + return null; + } +} diff --git a/api/src/test/java/org/eclipse/microprofile/health/HealthCheckResponseProviderResolverTest.java b/api/src/test/java/org/eclipse/microprofile/health/HealthCheckResponseProviderResolverTest.java new file mode 100644 index 0000000..ef02375 --- /dev/null +++ b/api/src/test/java/org/eclipse/microprofile/health/HealthCheckResponseProviderResolverTest.java @@ -0,0 +1,94 @@ +/* + * Copyright (c) 2020 Contributors to the Eclipse Foundation + * + * See the NOTICES file(s) distributed with this work for additional + * information regarding copyright ownership. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * You may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * SPDX-License-Identifier: Apache-2.0 + * + */ +package org.eclipse.microprofile.health; + +import java.io.FileWriter; +import java.nio.file.FileSystems; +import java.nio.file.Files; + +import org.eclipse.microprofile.health.spi.HealthCheckResponseProvider; +import org.testng.Assert; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.Test; + +/** + * Testing {@link HealthCheckResponseProvider} resolution + */ +public class HealthCheckResponseProviderResolverTest { + + private static final String SERVICE_PATH = System.getProperty("serviceDir"); + private static final String SERVICE_FILE_NAME = SERVICE_PATH + HealthCheckResponseProvider.class.getName(); + + + private static class ProviderResolverChild extends HealthCheckResponseProviderResolver { + + static void resetResolver() { + reset(); + } + } + + @BeforeMethod + public void setUp() throws Exception { + ProviderResolverChild.resetResolver(); + Files.deleteIfExists(FileSystems.getDefault().getPath(SERVICE_FILE_NAME)); + } + + @Test(expectedExceptions = IllegalStateException.class) + public void testWithoutServiceFile() throws Exception { + HealthCheckResponse.builder(); + } + + + @Test + public void testWithOneGoodProvider() throws Exception { + System.out.println(SERVICE_FILE_NAME); + FileWriter fw = new FileWriter(SERVICE_FILE_NAME); + fw.write(DummyResponseProvider1.class.getName()); + fw.close(); + Assert.assertEquals(HealthCheckResponseProviderResolver.getProvider().getClass(),DummyResponseProvider1.class); + } + + @Test + public void testWithTwoGoodProvider() throws Exception { + FileWriter fw = new FileWriter(SERVICE_FILE_NAME); + fw.write(DummyResponseProvider1.class.getName()); + fw.write('\n'); + fw.write(DummyResponseProvider2.class.getName()); + fw.close(); + Assert.assertTrue(HealthCheckResponseProviderResolver.getProvider().getClass().equals(DummyResponseProvider1.class) || + HealthCheckResponseProviderResolver.getProvider().getClass().equals(DummyResponseProvider2.class)); + } + + @Test + public void testPredicates() throws Exception { + FileWriter fw = new FileWriter(SERVICE_FILE_NAME); + fw.write(DummyResponseProvider1.class.getName()); + fw.write('\n'); + fw.write(DummyResponseProvider2.class.getName()); + fw.close(); + HealthCheckResponseProviderResolver.setProviderPredicate( p -> p.getClass().equals(DummyResponseProvider1.class)); + Assert.assertEquals(HealthCheckResponseProviderResolver.getProvider().getClass(),DummyResponseProvider1.class); + HealthCheckResponseProviderResolver.setProviderPredicate( p -> p.getClass().equals(DummyResponseProvider2.class)); + Assert.assertEquals(HealthCheckResponseProviderResolver.getProvider().getClass(),DummyResponseProvider2.class); + } + +} diff --git a/api/src/test/resources/META-INF/services/dummy b/api/src/test/resources/META-INF/services/dummy new file mode 100644 index 0000000..e69de29 diff --git a/pom.xml b/pom.xml index 85df2c8..a6d0fbb 100644 --- a/pom.xml +++ b/pom.xml @@ -46,6 +46,8 @@ 1.0.0 3.4.0 4.2.0 + 2.22.0 + 6.9.9 @@ -146,6 +148,12 @@ ${org.osgi.annotation.versioning.version} provided + + org.testng + testng + ${testng.version} + test + @@ -331,6 +339,7 @@ .factorypath .editorconfig src/main/resources/**/* + src/test/resources/**/* diff --git a/tck/pom.xml b/tck/pom.xml index 64936c6..a04b135 100644 --- a/tck/pom.xml +++ b/tck/pom.xml @@ -29,7 +29,6 @@ 1.4.1.Final ^_?[a-z][a-zA-Z0-9_]*$ - 6.9.9 1.1.6 4.5.2 1.2 @@ -77,7 +76,6 @@ org.testng testng - ${testng.version} compile