[GR-60293] Throw a ServiceConfigurationError if the service is not JCA-compliant.

jovanstevanovic · jovanstevanovic · commit f2f6136a0b70 · 2024-12-20T03:23:58.000Z
PullRequest: graal/19647
diff --git a/substratevm/mx.substratevm/mx_substratevm.py b/substratevm/mx.substratevm/mx_substratevm.py
@@ -334,7 +334,7 @@ def native_image_func(args, **kwargs):
     yield native_image_func
 
 native_image_context.hosted_assertions = ['-J-ea', '-J-esa']
-_native_unittest_features = '--features=com.oracle.svm.test.ImageInfoTest$TestFeature,com.oracle.svm.test.ServiceLoaderTest$TestFeature,com.oracle.svm.test.SecurityServiceTest$TestFeature,com.oracle.svm.test.ReflectionRegistrationTest$TestFeature'
+_native_unittest_features = '--features=com.oracle.svm.test.ImageInfoTest$TestFeature,com.oracle.svm.test.services.ServiceLoaderTest$TestFeature,com.oracle.svm.test.services.SecurityServiceTest$TestFeature,com.oracle.svm.test.ReflectionRegistrationTest$TestFeature'
 
 IMAGE_ASSERTION_FLAGS = svm_experimental_options(['-H:+VerifyGraalGraphs', '-H:+VerifyPhases'])
 
@@ -545,8 +545,8 @@ def native_unittests_task(extra_build_args=None):
             out.write(f"Simple file{i}" + '\n')
 
     additional_build_args = svm_experimental_options([
-        '-H:AdditionalSecurityProviders=com.oracle.svm.test.SecurityServiceTest$NoOpProvider',
-        '-H:AdditionalSecurityServiceTypes=com.oracle.svm.test.SecurityServiceTest$JCACompliantNoOpService',
+        '-H:AdditionalSecurityProviders=com.oracle.svm.test.services.SecurityServiceTest$NoOpProvider',
+        '-H:AdditionalSecurityServiceTypes=com.oracle.svm.test.services.SecurityServiceTest$JCACompliantNoOpService',
         '-cp', cp_entry_name
     ])
     if extra_build_args is not None:
diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/ServiceLoaderFeature.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/ServiceLoaderFeature.java
@@ -41,8 +41,8 @@
 import com.oracle.svm.core.feature.AutomaticallyRegisteredFeature;
 import com.oracle.svm.core.feature.InternalFeature;
 import com.oracle.svm.core.jdk.ServiceCatalogSupport;
-import com.oracle.svm.core.option.HostedOptionKey;
 import com.oracle.svm.core.option.AccumulatingLocatableMultiOptionValue;
+import com.oracle.svm.core.option.HostedOptionKey;
 import com.oracle.svm.hosted.analysis.Inflation;
 
 import jdk.graal.compiler.options.Option;
@@ -218,7 +218,21 @@ void handleServiceClassIsReachable(DuringAnalysisAccess access, Class<?> service
             if (nullaryConstructor != null || nullaryProviderMethod != null) {
                 RuntimeReflection.register(providerClass);
                 if (nullaryConstructor != null) {
+                    /*
+                     * Registering a constructor with
+                     * RuntimeReflection.registerConstructorLookup(providerClass) does not produce
+                     * the same behavior as using RuntimeReflection.register(nullaryConstructor). In
+                     * the first case, the constructor is marked for query purposes only, so this
+                     * if-statement cannot be eliminated.
+                     * 
+                     */
                     RuntimeReflection.register(nullaryConstructor);
+                } else {
+                    /*
+                     * If there's no nullary constructor, register it as negative lookup to avoid
+                     * throwing a MissingReflectionRegistrationError at run time.
+                     */
+                    RuntimeReflection.registerConstructorLookup(providerClass);
                 }
                 if (nullaryProviderMethod != null) {
                     RuntimeReflection.register(nullaryProviderMethod);
@@ -229,8 +243,14 @@ void handleServiceClassIsReachable(DuringAnalysisAccess access, Class<?> service
                      */
                     RuntimeReflection.registerMethodLookup(providerClass, "provider");
                 }
-                registeredProviders.add(provider);
             }
+            /*
+             * Register the provider in both cases: when it is JCA-compliant (has a nullary
+             * constructor or a provider method) or when it lacks both. If neither is present, a
+             * ServiceConfigurationError will be thrown at runtime, consistent with HotSpot
+             * behavior.
+             */
+            registeredProviders.add(provider);
         }
         if (!registeredProviders.isEmpty()) {
             String serviceResourceLocation = "META-INF/services/" + serviceProvider.getName();
diff --git a/substratevm/src/com.oracle.svm.test/src/META-INF/native-image/com.oracle.svm.test/native-image.properties b/substratevm/src/com.oracle.svm.test/src/META-INF/native-image/com.oracle.svm.test/native-image.properties
@@ -2,8 +2,8 @@ Args= \
   --initialize-at-run-time=com.oracle.svm.test \
   --initialize-at-build-time=com.oracle.svm.test.AbstractClassSerializationTest,com.oracle.svm.test.SerializationRegistrationTest,com.oracle.svm.test.SerializationRegistrationTest$SerializableTestClass,com.oracle.svm.test.SerializationRegistrationTest$AssociatedRegistrationTestClass,com.oracle.svm.test.LambdaClassSerializationTest,com.oracle.svm.test.LambdaClassDeserializationTest \
   --features=com.oracle.svm.test.SerializationRegistrationTestFeature \
-  --features=com.oracle.svm.test.AbstractServiceLoaderTest$TestFeature \
-  --features=com.oracle.svm.test.NoProviderConstructorServiceLoaderTest$TestFeature \
+  --features=com.oracle.svm.test.services.AbstractServiceLoaderTest$TestFeature \
+  --features=com.oracle.svm.test.services.NoProviderConstructorServiceLoaderTest$TestFeature \
   --features=com.oracle.svm.test.NativeImageResourceUtils$TestFeature \
   --features=com.oracle.svm.test.jfr.JfrTestFeature \
   --add-opens=java.base/java.lang=ALL-UNNAMED \
diff --git a/substratevm/src/com.oracle.svm.test/src/META-INF/services/com.oracle.svm.test.AbstractServiceLoaderTest$ServiceInterface b/substratevm/src/com.oracle.svm.test/src/META-INF/services/com.oracle.svm.test.AbstractServiceLoaderTest$ServiceInterface
diff --git a/substratevm/src/com.oracle.svm.test/src/META-INF/services/com.oracle.svm.test.NoProviderConstructorServiceLoaderTest$ServiceInterface b/substratevm/src/com.oracle.svm.test/src/META-INF/services/com.oracle.svm.test.NoProviderConstructorServiceLoaderTest$ServiceInterface
diff --git a/substratevm/src/com.oracle.svm.test/src/META-INF/services/com.oracle.svm.test.ServiceLoaderTest$ServiceInterface b/substratevm/src/com.oracle.svm.test/src/META-INF/services/com.oracle.svm.test.ServiceLoaderTest$ServiceInterface
diff --git a/substratevm/src/com.oracle.svm.test/src/META-INF/services/com.oracle.svm.test.services.AbstractServiceLoaderTest$ServiceInterface b/substratevm/src/com.oracle.svm.test/src/META-INF/services/com.oracle.svm.test.services.AbstractServiceLoaderTest$ServiceInterface
@@ -0,0 +1,2 @@
+com.oracle.svm.test.services.AbstractServiceLoaderTest$ConcreteService
+com.oracle.svm.test.services.AbstractServiceLoaderTest$AbstractService
diff --git a/substratevm/src/com.oracle.svm.test/src/META-INF/services/com.oracle.svm.test.services.NoProviderConstructorServiceLoaderTest$ServiceInterface b/substratevm/src/com.oracle.svm.test/src/META-INF/services/com.oracle.svm.test.services.NoProviderConstructorServiceLoaderTest$ServiceInterface
@@ -0,0 +1,2 @@
+com.oracle.svm.test.services.NoProviderConstructorServiceLoaderTest$ProperService
+com.oracle.svm.test.services.NoProviderConstructorServiceLoaderTest$NoProviderConstructorService
diff --git a/substratevm/src/com.oracle.svm.test/src/META-INF/services/com.oracle.svm.test.services.ServiceLoaderTest$ServiceInterface b/substratevm/src/com.oracle.svm.test/src/META-INF/services/com.oracle.svm.test.services.ServiceLoaderTest$ServiceInterface
@@ -0,0 +1,3 @@
+com.oracle.svm.test.services.ServiceLoaderTest$ServiceA
+com.oracle.svm.test.services.ServiceLoaderTest$ServiceB
+com.oracle.svm.test.services.ServiceLoaderTest$ServiceHostedOnly
diff --git a/substratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/services/AbstractServiceLoaderTest.java b/substratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/services/AbstractServiceLoaderTest.java
@@ -22,7 +22,7 @@
  * or visit www.oracle.com if you need additional information or have any
  * questions.
  */
-package com.oracle.svm.test;
+package com.oracle.svm.test.services;
 
 import java.util.HashSet;
 import java.util.ServiceConfigurationError;
diff --git a/substratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/services/NoProviderConstructorServiceLoaderTest.java b/substratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/services/NoProviderConstructorServiceLoaderTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2021, 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2021, 2024, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -22,7 +22,7 @@
  * or visit www.oracle.com if you need additional information or have any
  * questions.
  */
-package com.oracle.svm.test;
+package com.oracle.svm.test.services;
 
 import java.util.HashSet;
 import java.util.ServiceConfigurationError;
@@ -40,12 +40,8 @@
 // Checkstyle: allow Class.getSimpleName
 
 /**
- * Tests a workaround for {@linkplain ServiceLoader services} without a {@linkplain ServiceLoader
- * provider constructor} (nullary constructor) [GR-19958]. The workaround completely ignores
- * services without a provider constructor, instead of throwing an {@link ServiceConfigurationError}
- * when iterating the services. See the Github issue
- * <a href="https://github.com/oracle/graal/issues/2652">"Spring Service Registry native-image
- * failure due to service loader handling in jersey #2652"</a> for more details.
+ * Test both JCA-compliant services and non-JCA-compliant services (without a nullary constructor),
+ * and compare the behavior between Native Image and Hotspot.
  */
 public class NoProviderConstructorServiceLoaderTest {
 
@@ -73,81 +69,77 @@ public abstract static class NoProviderConstructorService implements ServiceInte
 
     private static final Set<String> EXPECTED = Set.of(ProperService.class.getSimpleName());
 
-    /**
-     * This should actually throw an {@link ServiceConfigurationError}.
-     *
-     * @see #testLazyStreamHotspot()
-     */
-    @Test
+    @Test(expected = ServiceConfigurationError.class)
     public void testLazyStreamNativeImage() {
-        Assume.assumeTrue("native image specific behavior", ImageInfo.inImageRuntimeCode());
-        Set<String> simpleNames = ServiceLoader.load(ServiceInterface.class).stream()
-                        .map(provider -> provider.type().getSimpleName())
-                        .collect(Collectors.toSet());
+        assumeEnvironment(true);
+        Set<String> simpleNames = loadLazyStreamNames();
         Assert.assertEquals(EXPECTED, simpleNames);
     }
 
-    /**
-     * This should actually throw an {@link ServiceConfigurationError}.
-     *
-     * @see #testEagerStreamHotspot()
-     */
-    @Test
+    @Test(expected = ServiceConfigurationError.class)
     public void testEagerStreamNativeImage() {
-        Assume.assumeTrue("native image specific behavior", ImageInfo.inImageRuntimeCode());
-        Set<String> simpleNames = ServiceLoader.load(ServiceInterface.class).stream()
-                        .map(provider -> provider.get().getClass().getSimpleName())
-                        .collect(Collectors.toSet());
+        assumeEnvironment(true);
+        Set<String> simpleNames = loadEagerStreamNames();
         Assert.assertEquals(EXPECTED, simpleNames);
     }
 
-    /**
-     * This should actually throw an {@link ServiceConfigurationError}.
-     *
-     * @see #testEagerIteratorHotspot()
-     */
-    @Test
+    @Test(expected = ServiceConfigurationError.class)
     public void testEagerIteratorNativeImage() {
-        Assume.assumeTrue("native image specific behavior", ImageInfo.inImageRuntimeCode());
-        Set<String> simpleNames = new HashSet<>();
-        ServiceLoader.load(ServiceInterface.class).iterator()
-                        .forEachRemaining(s -> simpleNames.add(s.getClass().getSimpleName()));
+        assumeEnvironment(true);
+        Set<String> simpleNames = loadEagerIteratorNames();
         Assert.assertEquals(EXPECTED, simpleNames);
     }
 
-    /**
-     * @see #testLazyStreamNativeImage()
-     */
     @Test(expected = ServiceConfigurationError.class)
     public void testLazyStreamHotspot() {
-        Assume.assumeFalse("hotspot specific behavior", ImageInfo.inImageRuntimeCode());
-        Set<String> simpleNames = ServiceLoader.load(ServiceInterface.class).stream()
+        assumeEnvironment(false);
+        Set<String> simpleNames = loadLazyStreamNames();
+        Assert.assertNull("should not reach", simpleNames);
+    }
+
+    @Test(expected = ServiceConfigurationError.class)
+    public void testEagerIteratorHotspot() {
+        assumeEnvironment(false);
+        Set<String> simpleNames = loadEagerIteratorNames();
+        Assert.assertNull("should not reach", simpleNames);
+    }
+
+    /**
+     * Helper method to assume the environment (hotspot/native image).
+     */
+    private static void assumeEnvironment(boolean isNativeImage) {
+        if (isNativeImage) {
+            Assume.assumeTrue("native image specific behavior", ImageInfo.inImageRuntimeCode());
+        } else {
+            Assume.assumeFalse("hotspot specific behavior", ImageInfo.inImageRuntimeCode());
+        }
+    }
+
+    /**
+     * Helper method for lazy stream tests.
+     */
+    private static Set<String> loadLazyStreamNames() {
+        return ServiceLoader.load(ServiceInterface.class).stream()
                         .map(provider -> provider.type().getSimpleName())
                         .collect(Collectors.toSet());
-        Assert.assertNull("should not reach", simpleNames);
     }
 
     /**
-     * @see #testEagerStreamNativeImage()
+     * Helper method for eager stream tests.
      */
-    @Test(expected = ServiceConfigurationError.class)
-    public void testEagerStreamHotspot() {
-        Assume.assumeFalse("hotspot specific behavior", ImageInfo.inImageRuntimeCode());
-        Set<String> simpleNames = ServiceLoader.load(ServiceInterface.class).stream()
+    private static Set<String> loadEagerStreamNames() {
+        return ServiceLoader.load(ServiceInterface.class).stream()
                         .map(provider -> provider.get().getClass().getSimpleName())
                         .collect(Collectors.toSet());
-        Assert.assertNull("should not reach", simpleNames);
     }
 
     /**
-     * @see #testEagerIteratorNativeImage()
+     * Helper method for eager iterator tests.
      */
-    @Test(expected = ServiceConfigurationError.class)
-    public void testEagerIteratorHotspot() {
-        Assume.assumeFalse("hotspot specific behavior", ImageInfo.inImageRuntimeCode());
+    private static Set<String> loadEagerIteratorNames() {
         Set<String> simpleNames = new HashSet<>();
         ServiceLoader.load(ServiceInterface.class).iterator()
                         .forEachRemaining(s -> simpleNames.add(s.getClass().getSimpleName()));
-        Assert.assertNull("should not reach", simpleNames);
+        return simpleNames;
     }
 }
diff --git a/substratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/services/SecurityServiceTest.java b/substratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/services/SecurityServiceTest.java
@@ -22,7 +22,7 @@
  * or visit www.oracle.com if you need additional information or have any
  * questions.
  */
-package com.oracle.svm.test;
+package com.oracle.svm.test.services;
 
 import java.security.NoSuchAlgorithmException;
 import java.security.Provider;
diff --git a/substratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/services/ServiceLoaderTest.java b/substratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/services/ServiceLoaderTest.java
@@ -22,7 +22,7 @@
  * or visit www.oracle.com if you need additional information or have any
  * questions.
  */
-package com.oracle.svm.test;
+package com.oracle.svm.test.services;
 
 import java.util.ArrayList;
 import java.util.List;

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+com.oracle.svm.test.services.AbstractServiceLoaderTest$ConcreteService`
	`2`	`+com.oracle.svm.test.services.AbstractServiceLoaderTest$AbstractService`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+com.oracle.svm.test.services.NoProviderConstructorServiceLoaderTest$ProperService`
	`2`	`+com.oracle.svm.test.services.NoProviderConstructorServiceLoaderTest$NoProviderConstructorService`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+com.oracle.svm.test.services.ServiceLoaderTest$ServiceA`
	`2`	`+com.oracle.svm.test.services.ServiceLoaderTest$ServiceB`
	`3`	`+com.oracle.svm.test.services.ServiceLoaderTest$ServiceHostedOnly`