diff --git a/CHANGES.txt b/CHANGES.txt index 2b666949d5..38b95c916b 100644 --- a/CHANGES.txt +++ b/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) diff --git a/src/java/org/apache/cassandra/service/StartupChecks.java b/src/java/org/apache/cassandra/service/StartupChecks.java index 31a2686af7..86ab41688e 100644 --- a/src/java/org/apache/cassandra/service/StartupChecks.java +++ b/src/java/org/apache/cassandra/service/StartupChecks.java @@ -156,29 +156,24 @@ public class StartupChecks public StartupChecks withServiceLoaderTests() { - ServiceLoader 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 customChecks = new HashSet<>(); Set uniqueNames = new HashSet<>(); Set 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()) diff --git a/test/unit/org/apache/cassandra/service/StartupChecksTest.java b/test/unit/org/apache/cassandra/service/StartupChecksTest.java index 806e3cd65f..34fc287643 100644 --- a/test/unit/org/apache/cassandra/service/StartupChecksTest.java +++ b/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 failingIterator = mock(Iterator.class); + when(failingIterator.hasNext()).thenReturn(true); + when(failingIterator.next()).thenThrow(error); + + ServiceLoader loader = mock(ServiceLoader.class); + doReturn(failingIterator).when(loader).iterator(); + + try (MockedStatic 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