Move exception handling of SPI startup checks when iterating over them

patch by Stefan Miklosovic; reviewed by Caleb Rackliffe for CASSANDRA-21409

CHANGES.txt

@@ -1,4 +1,5 @@
 6.0-alpha2
+ * Move exception handling of SPI startup checks when iterating over them (CASSANDRA-21409)
  * Avoid type lookup in SerializationHeader#getType if schema and SSTable are aligned (CASSANDRA-21402)
  * Replace LongAdder in metric-like logic with ThreadLocalCounter (CASSANDRA-21400)
  * BTreeRow.hasLiveData: avoid Cell iteration if there are no cell tombstones (CASSANDRA-21363)

src/java/org/apache/cassandra/service/StartupChecks.java

@@ -156,29 +156,24 @@ public class StartupChecks
 
     public StartupChecks withServiceLoaderTests()
     {
-        ServiceLoader<StartupCheck> loader;
-
-        try
-        {
-            loader = ServiceLoader.load(StartupCheck.class);
-        }
-        catch (ServiceConfigurationError t)
-        {
-            logger.warn("Unable to get startup checks via ServiceLoader. " +
-                        "Custom checks will not be triggered. Reason: " + t.getMessage());
-            return this;
-        }
-
         Set<StartupCheck> customChecks = new HashSet<>();
         Set<String> uniqueNames = new HashSet<>();
         Set<String> duplicitNames = new HashSet<>();
 
-        for (StartupCheck check : loader)
+        try
         {
-            if (!uniqueNames.add(check.name()))
-                duplicitNames.add(check.name());
-            else
-                customChecks.add(check);
+            for (StartupCheck check : ServiceLoader.load(StartupCheck.class))
+            {
+                if (!uniqueNames.add(check.name()))
+                    duplicitNames.add(check.name());
+                else
+                    customChecks.add(check);
+            }
+        }
+        catch (ServiceConfigurationError t)
+        {
+            throw new ConfigurationException("Unable to get startup checks via ServiceLoader. " +
+                                             "Custom checks will not be triggered.", t);
         }
 
         if (!duplicitNames.isEmpty())

test/unit/org/apache/cassandra/service/StartupChecksTest.java

@@ -28,8 +28,10 @@ import java.nio.file.attribute.FileTime;
 import java.nio.file.spi.FileSystemProvider;
 import java.time.Instant;
 import java.util.HashMap;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.ServiceConfigurationError;
 import java.util.ServiceLoader;
 import java.util.Set;
 import java.util.UUID;
@@ -55,6 +57,7 @@ import org.apache.cassandra.config.StartupChecksConfiguration;
 import org.apache.cassandra.db.ColumnFamilyStore;
 import org.apache.cassandra.db.Directories;
 import org.apache.cassandra.db.Keyspace;
+import org.apache.cassandra.exceptions.ConfigurationException;
 import org.apache.cassandra.exceptions.StartupException;
 import org.apache.cassandra.io.filesystem.ForwardingFileSystem;
 import org.apache.cassandra.io.filesystem.ForwardingFileSystemProvider;
@@ -308,6 +311,38 @@ public class StartupChecksTest
         testKernelBug1057843Check("ext4", DiskAccessMode.mmap, new Semver("6.1.64.1-generic"), false);
     }
 
+    @SuppressWarnings("unchecked")
+    @Test
+    public void testErrorneousCustomCheckFailsStartup()
+    {
+        // ServiceLoader instantiates providers lazily: a custom StartupCheck whose no-arg
+        // constructor throws does NOT fail ServiceLoader.load(), it fails later, during
+        // iteration, surfacing as a ServiceConfigurationError from the iterator. We model that
+        // here by having the iterator throw on next(). withServiceLoaderTests() must catch the
+        // error around the iteration loop and rethrow it as a ConfigurationException, rather than
+        // letting it escape uncaught (which would be wrapped by applyStartupChecks() into a
+        // misleading "Invalid configuration of startup_checks" failure).
+        ServiceConfigurationError error = new ServiceConfigurationError("org.example.BadCheck could not be instantiated",
+                                                                        new RuntimeException("Failure to instantiate"));
+
+        Iterator<StartupCheck> failingIterator = mock(Iterator.class);
+        when(failingIterator.hasNext()).thenReturn(true);
+        when(failingIterator.next()).thenThrow(error);
+
+        ServiceLoader<StartupCheck> loader = mock(ServiceLoader.class);
+        doReturn(failingIterator).when(loader).iterator();
+
+        try (MockedStatic<ServiceLoader> serviceLoader = Mockito.mockStatic(ServiceLoader.class))
+        {
+            serviceLoader.when(() -> ServiceLoader.load(StartupCheck.class)).thenReturn(loader);
+
+            assertThatExceptionOfType(ConfigurationException.class)
+                .isThrownBy(() -> new StartupChecks().withDefaultTests().withServiceLoaderTests())
+                .withMessageContaining("Unable to get startup checks via ServiceLoader")
+                .withCause(error);
+        }
+    }
+
     @SuppressWarnings("unchecked")
     @Test
     public void testExternalCheckIsLoaded() throws StartupException