From ca20951e00e29ad76902411c7360fba063c4d243 Mon Sep 17 00:00:00 2001 From: Gabriel-Trintinalia Date: Mon, 23 Sep 2024 14:10:15 +1000 Subject: [PATCH 01/12] Add option to halt besu during plugin lifecycle Signed-off-by: Gabriel-Trintinalia --- .../dsl/node/ThreadBesuNodeRunner.java | 3 +- .../acceptance/plugins/TestPicoCLIPlugin.java | 28 +++ .../services/BesuPluginContextImplTest.java | 213 +++++++++++++++--- .../org/hyperledger/besu/cli/BesuCommand.java | 6 +- .../stable/PluginsConfigurationOptions.java | 15 ++ .../besu/services/BesuPluginContextImpl.java | 75 ++++-- .../core/plugins/PluginConfiguration.java | 18 +- 7 files changed, 304 insertions(+), 54 deletions(-) diff --git a/acceptance-tests/dsl/src/main/java/org/hyperledger/besu/tests/acceptance/dsl/node/ThreadBesuNodeRunner.java b/acceptance-tests/dsl/src/main/java/org/hyperledger/besu/tests/acceptance/dsl/node/ThreadBesuNodeRunner.java index 5d78f1460c4..90de0b0e952 100644 --- a/acceptance-tests/dsl/src/main/java/org/hyperledger/besu/tests/acceptance/dsl/node/ThreadBesuNodeRunner.java +++ b/acceptance-tests/dsl/src/main/java/org/hyperledger/besu/tests/acceptance/dsl/node/ThreadBesuNodeRunner.java @@ -503,8 +503,9 @@ public BesuPluginContextImpl providePluginContext( besuPluginContext.addService(PermissioningService.class, permissioningService); besuPluginContext.addService(PrivacyPluginService.class, new PrivacyPluginServiceImpl()); - besuPluginContext.registerPlugins( + besuPluginContext.initialize( new PluginConfiguration.Builder().pluginsDir(pluginsPath).build()); + besuPluginContext.registerPlugins(); commandLine.parseArgs(extraCLIOptions.toArray(new String[0])); // register built-in plugins diff --git a/acceptance-tests/test-plugins/src/main/java/org/hyperledger/besu/tests/acceptance/plugins/TestPicoCLIPlugin.java b/acceptance-tests/test-plugins/src/main/java/org/hyperledger/besu/tests/acceptance/plugins/TestPicoCLIPlugin.java index 0db5281537c..53905866fbf 100644 --- a/acceptance-tests/test-plugins/src/main/java/org/hyperledger/besu/tests/acceptance/plugins/TestPicoCLIPlugin.java +++ b/acceptance-tests/test-plugins/src/main/java/org/hyperledger/besu/tests/acceptance/plugins/TestPicoCLIPlugin.java @@ -66,6 +66,20 @@ public void register(final BesuContext context) { state = "registered"; } + @Override + public void beforeExternalServices() { + LOG.info("Before external services. Test Option is '{}'", testOption); + state = "beforeExternalServices"; + + if ("FAILBEFOREEXTERNALSERVICES".equals(testOption)) { + state = "failbeforeExternalServices"; + throw new RuntimeException("I was told to fail before external services"); + } + + writeSignal("beforeExternalServices"); + state = "beforeExternalServicesFinished"; + } + @Override public void start() { LOG.info("Starting. Test Option is '{}'", testOption); @@ -80,6 +94,20 @@ public void start() { state = "started"; } + @Override + public void afterExternalServicePostMainLoop() { + LOG.info("After external services post main loop. Test Option is '{}'", testOption); + state = "afterExternalServicePostMainLoop"; + + if ("FAILAFTEREXTERNALSERVICEPOSTMAINLOOP".equals(testOption)) { + state = "failafterExternalServicePostMainLoop"; + throw new RuntimeException("I was told to fail after external services post main loop"); + } + + writeSignal("afterExternalServicePostMainLoop"); + state = "afterExternalServicePostMainLoopFinished"; + } + @Override public void stop() { LOG.info("Stopping. Test Option is '{}'", testOption); diff --git a/acceptance-tests/test-plugins/src/test/java/org/hyperledger/besu/services/BesuPluginContextImplTest.java b/acceptance-tests/test-plugins/src/test/java/org/hyperledger/besu/services/BesuPluginContextImplTest.java index 7e277041776..46e54c26ad3 100644 --- a/acceptance-tests/test-plugins/src/test/java/org/hyperledger/besu/services/BesuPluginContextImplTest.java +++ b/acceptance-tests/test-plugins/src/test/java/org/hyperledger/besu/services/BesuPluginContextImplTest.java @@ -42,6 +42,9 @@ public class BesuPluginContextImplTest { private static final Path DEFAULT_PLUGIN_DIRECTORY = Paths.get("."); private BesuPluginContextImpl contextImpl; + private static final PluginConfiguration DEFAULT_CONFIGURATION = + PluginConfiguration.builder().pluginsDir(DEFAULT_PLUGIN_DIRECTORY).build(); + @BeforeAll public static void createFakePluginDir() throws IOException { if (System.getProperty("besu.plugins.dir") == null) { @@ -64,8 +67,8 @@ void setup() { @Test public void verifyEverythingGoesSmoothly() { assertThat(contextImpl.getRegisteredPlugins()).isEmpty(); - contextImpl.registerPlugins( - PluginConfiguration.builder().pluginsDir(DEFAULT_PLUGIN_DIRECTORY).build()); + contextImpl.initialize(DEFAULT_CONFIGURATION); + contextImpl.registerPlugins(); assertThat(contextImpl.getRegisteredPlugins()).isNotEmpty(); final Optional testPluginOptional = @@ -87,8 +90,8 @@ public void registrationErrorsHandledSmoothly() { System.setProperty("testPicoCLIPlugin.testOption", "FAILREGISTER"); assertThat(contextImpl.getRegisteredPlugins()).isEmpty(); - contextImpl.registerPlugins( - PluginConfiguration.builder().pluginsDir(DEFAULT_PLUGIN_DIRECTORY).build()); + contextImpl.initialize(DEFAULT_CONFIGURATION); + contextImpl.registerPlugins(); assertThat(contextImpl.getRegisteredPlugins()).isNotInstanceOfAny(TestPicoCLIPlugin.class); contextImpl.beforeExternalServices(); @@ -106,8 +109,8 @@ public void startErrorsHandledSmoothly() { System.setProperty("testPicoCLIPlugin.testOption", "FAILSTART"); assertThat(contextImpl.getRegisteredPlugins()).isEmpty(); - contextImpl.registerPlugins( - PluginConfiguration.builder().pluginsDir(DEFAULT_PLUGIN_DIRECTORY).build()); + contextImpl.initialize(DEFAULT_CONFIGURATION); + contextImpl.registerPlugins(); assertThat(contextImpl.getRegisteredPlugins()) .extracting("class") .contains(TestPicoCLIPlugin.class); @@ -132,8 +135,8 @@ public void stopErrorsHandledSmoothly() { System.setProperty("testPicoCLIPlugin.testOption", "FAILSTOP"); assertThat(contextImpl.getRegisteredPlugins()).isEmpty(); - contextImpl.registerPlugins( - PluginConfiguration.builder().pluginsDir(DEFAULT_PLUGIN_DIRECTORY).build()); + contextImpl.initialize(DEFAULT_CONFIGURATION); + contextImpl.registerPlugins(); assertThat(contextImpl.getRegisteredPlugins()) .extracting("class") .contains(TestPicoCLIPlugin.class); @@ -154,10 +157,8 @@ public void stopErrorsHandledSmoothly() { @Test public void lifecycleExceptions() throws Throwable { - final ThrowableAssert.ThrowingCallable registerPlugins = - () -> - contextImpl.registerPlugins( - PluginConfiguration.builder().pluginsDir(DEFAULT_PLUGIN_DIRECTORY).build()); + contextImpl.initialize(DEFAULT_CONFIGURATION); + final ThrowableAssert.ThrowingCallable registerPlugins = () -> contextImpl.registerPlugins(); assertThatExceptionOfType(IllegalStateException.class).isThrownBy(contextImpl::startPlugins); assertThatExceptionOfType(IllegalStateException.class).isThrownBy(contextImpl::stopPlugins); @@ -179,11 +180,9 @@ public void lifecycleExceptions() throws Throwable { @Test public void shouldRegisterAllPluginsWhenNoPluginsOption() { - final PluginConfiguration config = - PluginConfiguration.builder().pluginsDir(DEFAULT_PLUGIN_DIRECTORY).build(); - assertThat(contextImpl.getRegisteredPlugins()).isEmpty(); - contextImpl.registerPlugins(config); + contextImpl.initialize(DEFAULT_CONFIGURATION); + contextImpl.registerPlugins(); final Optional testPluginOptional = findTestPlugin(contextImpl.getRegisteredPlugins(), TestPicoCLIPlugin.class); assertThat(testPluginOptional).isPresent(); @@ -193,10 +192,9 @@ public void shouldRegisterAllPluginsWhenNoPluginsOption() { @Test public void shouldRegisterOnlySpecifiedPluginWhenPluginsOptionIsSet() { - final PluginConfiguration config = createConfigurationForSpecificPlugin("TestPicoCLIPlugin"); - assertThat(contextImpl.getRegisteredPlugins()).isEmpty(); - contextImpl.registerPlugins(config); + contextImpl.initialize(createConfigurationForSpecificPlugin("TestPicoCLIPlugin")); + contextImpl.registerPlugins(); final Optional requestedPlugin = findTestPlugin(contextImpl.getRegisteredPlugins(), TestPicoCLIPlugin.class); @@ -212,9 +210,9 @@ public void shouldRegisterOnlySpecifiedPluginWhenPluginsOptionIsSet() { @Test public void shouldNotRegisterUnspecifiedPluginsWhenPluginsOptionIsSet() { - final PluginConfiguration config = createConfigurationForSpecificPlugin("TestPicoCLIPlugin"); assertThat(contextImpl.getRegisteredPlugins()).isEmpty(); - contextImpl.registerPlugins(config); + contextImpl.initialize(createConfigurationForSpecificPlugin("TestPicoCLIPlugin")); + contextImpl.registerPlugins(); final Optional nonRequestedPlugin = findTestPlugin(contextImpl.getRegisteredPlugins(), TestBesuEventsPlugin.class); @@ -223,10 +221,9 @@ public void shouldNotRegisterUnspecifiedPluginsWhenPluginsOptionIsSet() { @Test void shouldThrowExceptionIfExplicitlySpecifiedPluginNotFound() { - PluginConfiguration config = createConfigurationForSpecificPlugin("NonExistentPlugin"); - + contextImpl.initialize(createConfigurationForSpecificPlugin("NonExistentPlugin")); String exceptionMessage = - assertThrows(NoSuchElementException.class, () -> contextImpl.registerPlugins(config)) + assertThrows(NoSuchElementException.class, () -> contextImpl.registerPlugins()) .getMessage(); final String expectedMessage = "The following requested plugins were not found: NonExistentPlugin"; @@ -241,7 +238,8 @@ void shouldNotRegisterAnyPluginsIfExternalPluginsDisabled() { .pluginsDir(DEFAULT_PLUGIN_DIRECTORY) .externalPluginsEnabled(false) .build(); - contextImpl.registerPlugins(config); + contextImpl.initialize(config); + contextImpl.registerPlugins(); assertThat(contextImpl.getRegisteredPlugins().isEmpty()).isTrue(); } @@ -252,10 +250,173 @@ void shouldRegisterPluginsIfExternalPluginsEnabled() { .pluginsDir(DEFAULT_PLUGIN_DIRECTORY) .externalPluginsEnabled(true) .build(); - contextImpl.registerPlugins(config); + contextImpl.initialize(config); + contextImpl.registerPlugins(); assertThat(contextImpl.getRegisteredPlugins().isEmpty()).isFalse(); } + @Test + void shouldHaltOnPluginErrorWhenFlagIsTrue() { + System.setProperty("testPicoCLIPlugin.testOption", "FAILREGISTER"); + + PluginConfiguration config = + PluginConfiguration.builder() + .requestedPlugins(List.of(new PluginInfo("TestPicoCLIPlugin"))) + .pluginsDir(DEFAULT_PLUGIN_DIRECTORY) + .haltOnPluginError(true) + .build(); + + contextImpl.initialize(config); + + String errorMessage = + String.format("Error registering plugin of type %s", TestPicoCLIPlugin.class.getName()); + assertThatExceptionOfType(RuntimeException.class) + .isThrownBy(() -> contextImpl.registerPlugins()) + .withMessageContaining(errorMessage); + } + + @Test + void shouldNotHaltOnPluginErrorWhenFlagIsFalse() { + System.setProperty("testPicoCLIPlugin.testOption", "FAILREGISTER"); + + PluginConfiguration config = + PluginConfiguration.builder() + .pluginsDir(DEFAULT_PLUGIN_DIRECTORY) + .haltOnPluginError(false) + .build(); + + contextImpl.initialize(config); + contextImpl.registerPlugins(); + assertThat(contextImpl.getRegisteredPlugins()).isNotInstanceOfAny(TestPicoCLIPlugin.class); + } + + @Test + void shouldHaltOnPluginBeforeExternalServicesErrorWhenFlagIsTrue() { + System.setProperty("testPicoCLIPlugin.testOption", "FAILBEFOREEXTERNALSERVICES"); + + PluginConfiguration config = + PluginConfiguration.builder() + .requestedPlugins(List.of(new PluginInfo("TestPicoCLIPlugin"))) + .pluginsDir(DEFAULT_PLUGIN_DIRECTORY) + .haltOnPluginError(true) + .build(); + + contextImpl.initialize(config); + contextImpl.registerPlugins(); + + String errorMessage = + String.format( + "Error calling `beforeExternalServices` on plugin of type %s", + TestPicoCLIPlugin.class.getName()); + assertThatExceptionOfType(RuntimeException.class) + .isThrownBy(() -> contextImpl.beforeExternalServices()) + .withMessageContaining(errorMessage); + } + + @Test + void shouldNotHaltOnPluginBeforeExternalServicesErrorWhenFlagIsFalse() { + System.setProperty("testPicoCLIPlugin.testOption", "FAILBEFOREEXTERNALSERVICES"); + + PluginConfiguration config = + PluginConfiguration.builder() + .requestedPlugins(List.of(new PluginInfo("TestPicoCLIPlugin"))) + .pluginsDir(DEFAULT_PLUGIN_DIRECTORY) + .haltOnPluginError(false) + .build(); + + contextImpl.initialize(config); + contextImpl.registerPlugins(); + contextImpl.beforeExternalServices(); + + assertThat(contextImpl.getRegisteredPlugins()).isNotInstanceOfAny(TestPicoCLIPlugin.class); + } + + @Test + void shouldHaltOnPluginBeforeMainLoopErrorWhenFlagIsTrue() { + System.setProperty("testPicoCLIPlugin.testOption", "FAILSTART"); + + PluginConfiguration config = + PluginConfiguration.builder() + .requestedPlugins(List.of(new PluginInfo("TestPicoCLIPlugin"))) + .pluginsDir(DEFAULT_PLUGIN_DIRECTORY) + .haltOnPluginError(true) + .build(); + + contextImpl.initialize(config); + contextImpl.registerPlugins(); + contextImpl.beforeExternalServices(); + + String errorMessage = + String.format("Error starting plugin of type %s", TestPicoCLIPlugin.class.getName()); + assertThatExceptionOfType(RuntimeException.class) + .isThrownBy(() -> contextImpl.startPlugins()) + .withMessageContaining(errorMessage); + } + + @Test + void shouldNotHaltOnPluginBeforeMainLoopErrorWhenFlagIsFalse() { + System.setProperty("testPicoCLIPlugin.testOption", "FAILBEFOREMAINLOOP"); + + PluginConfiguration config = + PluginConfiguration.builder() + .requestedPlugins(List.of(new PluginInfo("TestPicoCLIPlugin"))) + .pluginsDir(DEFAULT_PLUGIN_DIRECTORY) + .haltOnPluginError(false) + .build(); + + contextImpl.initialize(config); + contextImpl.registerPlugins(); + contextImpl.beforeExternalServices(); + contextImpl.startPlugins(); + + assertThat(contextImpl.getRegisteredPlugins()).isNotInstanceOfAny(TestPicoCLIPlugin.class); + } + + @Test + void shouldHaltOnPluginAfterExternalServicePostMainLoopErrorWhenFlagIsTrue() { + System.setProperty("testPicoCLIPlugin.testOption", "FAILAFTEREXTERNALSERVICEPOSTMAINLOOP"); + + PluginConfiguration config = + PluginConfiguration.builder() + .requestedPlugins(List.of(new PluginInfo("TestPicoCLIPlugin"))) + .pluginsDir(DEFAULT_PLUGIN_DIRECTORY) + .haltOnPluginError(true) + .build(); + + contextImpl.initialize(config); + contextImpl.registerPlugins(); + contextImpl.beforeExternalServices(); + contextImpl.startPlugins(); + + String errorMessage = + String.format( + "Error calling `afterExternalServicePostMainLoop` on plugin of type %s", + TestPicoCLIPlugin.class.getName()); + assertThatExceptionOfType(RuntimeException.class) + .isThrownBy(() -> contextImpl.afterExternalServicesMainLoop()) + .withMessageContaining(errorMessage); + } + + @Test + void shouldNotHaltOnPluginAfterExternalServicePostMainLoopErrorWhenFlagIsFalse() { + System.setProperty("testPicoCLIPlugin.testOption", "FAILAFTEREXTERNALSERVICEPOSTMAINLOOP"); + + PluginConfiguration config = + PluginConfiguration.builder() + .requestedPlugins(List.of(new PluginInfo("TestPicoCLIPlugin"))) + .pluginsDir(DEFAULT_PLUGIN_DIRECTORY) + .haltOnPluginError(false) + .build(); + + contextImpl.initialize(config); + contextImpl.registerPlugins(); + contextImpl.beforeExternalServices(); + contextImpl.startPlugins(); + contextImpl.afterExternalServicesMainLoop(); + + assertThat(contextImpl.getRegisteredPlugins()).isNotInstanceOfAny(TestPicoCLIPlugin.class); + } + private PluginConfiguration createConfigurationForSpecificPlugin(final String pluginName) { return PluginConfiguration.builder() .requestedPlugins(List.of(new PluginInfo(pluginName))) diff --git a/besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java b/besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java index 79879538bd5..be559be5efb 100644 --- a/besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java +++ b/besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java @@ -118,7 +118,6 @@ import org.hyperledger.besu.ethereum.core.MiningParametersMetrics; import org.hyperledger.besu.ethereum.core.PrivacyParameters; import org.hyperledger.besu.ethereum.core.VersionMetadata; -import org.hyperledger.besu.ethereum.core.plugins.PluginConfiguration; import org.hyperledger.besu.ethereum.eth.sync.SyncMode; import org.hyperledger.besu.ethereum.eth.sync.SynchronizerConfiguration; import org.hyperledger.besu.ethereum.eth.transactions.ImmutableTransactionPoolConfiguration; @@ -1080,9 +1079,8 @@ private IExecutionStrategy createExecuteTask(final IExecutionStrategy nextStep) private IExecutionStrategy createPluginRegistrationTask(final IExecutionStrategy nextStep) { return parseResult -> { - PluginConfiguration configuration = - PluginsConfigurationOptions.fromCommandLine(parseResult.commandSpec().commandLine()); - besuPluginContext.registerPlugins(configuration); + besuPluginContext.initialize(pluginsConfigurationOptions.toDomainObject()); + besuPluginContext.registerPlugins(); commandLine.setExecutionStrategy(nextStep); return commandLine.execute(parseResult.originalArgs().toArray(new String[0])); }; diff --git a/besu/src/main/java/org/hyperledger/besu/cli/options/stable/PluginsConfigurationOptions.java b/besu/src/main/java/org/hyperledger/besu/cli/options/stable/PluginsConfigurationOptions.java index 47df831c577..eca40ffb527 100644 --- a/besu/src/main/java/org/hyperledger/besu/cli/options/stable/PluginsConfigurationOptions.java +++ b/besu/src/main/java/org/hyperledger/besu/cli/options/stable/PluginsConfigurationOptions.java @@ -47,6 +47,15 @@ public class PluginsConfigurationOptions implements CLIOptions plugins; + @CommandLine.Option( + names = {"--halt-on-plugin-error"}, + description = + "Prevent Besu from starting if any plugins fail to initialize correctly (default: ${DEFAULT-VALUE})", + hidden = true, + defaultValue = "false", + arity = "1") + private final Boolean haltOnPluginError = false; + /** Default Constructor. */ public PluginsConfigurationOptions() {} @@ -55,6 +64,7 @@ public PluginConfiguration toDomainObject() { return new PluginConfiguration.Builder() .externalPluginsEnabled(externalPluginsEnabled) .requestedPlugins(plugins) + .haltOnPluginError(haltOnPluginError) .build(); } @@ -92,9 +102,14 @@ public static PluginConfiguration fromCommandLine(final CommandLine commandLine) CommandLineUtils.getOptionValueOrDefault( commandLine, DEFAULT_PLUGINS_EXTERNAL_ENABLED_OPTION_NAME, Boolean::parseBoolean); + boolean haltOnPluginError = + CommandLineUtils.getOptionValueOrDefault( + commandLine, "--halt-on-plugin-error", Boolean::parseBoolean); + return new PluginConfiguration.Builder() .requestedPlugins(plugins) .externalPluginsEnabled(externalPluginsEnabled) + .haltOnPluginError(haltOnPluginError) .build(); } } diff --git a/besu/src/main/java/org/hyperledger/besu/services/BesuPluginContextImpl.java b/besu/src/main/java/org/hyperledger/besu/services/BesuPluginContextImpl.java index c89733f9354..617bf21a643 100644 --- a/besu/src/main/java/org/hyperledger/besu/services/BesuPluginContextImpl.java +++ b/besu/src/main/java/org/hyperledger/besu/services/BesuPluginContextImpl.java @@ -56,6 +56,8 @@ public class BesuPluginContextImpl implements BesuContext, PluginVersionsProvide private enum Lifecycle { /** Uninitialized lifecycle. */ UNINITIALIZED, + /** Initialized lifecycle. */ + INITIALIZED, /** Registering lifecycle. */ REGISTERING, /** Registered lifecycle. */ @@ -83,6 +85,7 @@ private enum Lifecycle { private final List registeredPlugins = new ArrayList<>(); private final List pluginVersions = new ArrayList<>(); + private PluginConfiguration config; /** Instantiates a new Besu plugin context. */ public BesuPluginContextImpl() {} @@ -116,19 +119,24 @@ private List detectPlugins(final PluginConfiguration config) { return StreamSupport.stream(serviceLoader.spliterator(), false).toList(); } + public void initialize(PluginConfiguration config) { + checkState( + state == Lifecycle.UNINITIALIZED, + "Besu plugins have already been initialized. Cannot register additional plugins."); + this.config = config; + state = Lifecycle.INITIALIZED; + } + /** * Registers plugins based on the provided {@link PluginConfiguration}. This method finds plugins * according to the configuration settings, filters them if necessary and then registers the * filtered or found plugins * - * @param config The configuration settings used to find and filter plugins for registration. The - * configuration includes the plugin directory and any configured plugin identifiers if - * applicable. * @throws IllegalStateException if the system is not in the UNINITIALIZED state. */ - public void registerPlugins(final PluginConfiguration config) { + public void registerPlugins() { checkState( - state == Lifecycle.UNINITIALIZED, + state == Lifecycle.INITIALIZED, "Besu plugins have already been registered. Cannot register additional plugins."); state = Lifecycle.REGISTERING; @@ -195,11 +203,14 @@ private boolean registerPlugin(final BesuPlugin plugin) { LOG.info("Registered plugin of type {}.", plugin.getClass().getName()); pluginVersions.add(plugin.getVersion()); } catch (final Exception e) { - LOG.error( - "Error registering plugin of type " - + plugin.getClass().getName() - + ", start and stop will not be called.", - e); + if (config.isHaltOnPluginError()) { + throw new RuntimeException( + "Error registering plugin of type " + plugin.getClass().getName(), e); + } else + LOG.error( + "Error registering plugin of type {}, start and stop will not be called.", + plugin.getClass().getName(), + e); return false; } return true; @@ -223,11 +234,17 @@ public void beforeExternalServices() { LOG.debug( "beforeExternalServices called on plugin of type {}.", plugin.getClass().getName()); } catch (final Exception e) { - LOG.error( - "Error calling `beforeExternalServices` on plugin of type " - + plugin.getClass().getName() - + ", stop will not be called.", - e); + if (config.isHaltOnPluginError()) { + throw new RuntimeException( + "Error calling `beforeExternalServices` on plugin of type " + + plugin.getClass().getName(), + e); + } else { + LOG.error( + "Error calling `beforeExternalServices` on plugin of type {}, start will not be called.", + plugin.getClass().getName(), + e); + } pluginsIterator.remove(); } } @@ -253,11 +270,15 @@ public void startPlugins() { plugin.start(); LOG.debug("Started plugin of type {}.", plugin.getClass().getName()); } catch (final Exception e) { - LOG.error( - "Error starting plugin of type " - + plugin.getClass().getName() - + ", stop will not be called.", - e); + if (config.isHaltOnPluginError()) { + throw new RuntimeException( + "Error starting plugin of type " + plugin.getClass().getName(), e); + } else { + LOG.error( + "Error starting plugin of type {}, stop will not be called.", + plugin.getClass().getName(), + e); + } pluginsIterator.remove(); } } @@ -279,7 +300,19 @@ public void afterExternalServicesMainLoop() { final BesuPlugin plugin = pluginsIterator.next(); try { plugin.afterExternalServicePostMainLoop(); - } finally { + } catch (final Exception e) { + if (config.isHaltOnPluginError()) { + throw new RuntimeException( + "Error calling `afterExternalServicePostMainLoop` on plugin of type " + + plugin.getClass().getName(), + e); + } else { + LOG.error( + "Error calling `afterExternalServicePostMainLoop` on plugin of type " + + plugin.getClass().getName() + + ", stop will not be called.", + e); + } pluginsIterator.remove(); } } diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/plugins/PluginConfiguration.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/plugins/PluginConfiguration.java index ebc99f647af..6e5df3d5b4d 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/plugins/PluginConfiguration.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/plugins/PluginConfiguration.java @@ -23,14 +23,17 @@ public class PluginConfiguration { private final List requestedPlugins; private final Path pluginsDir; private final boolean externalPluginsEnabled; + private final boolean haltOnPluginError; public PluginConfiguration( final List requestedPlugins, final Path pluginsDir, - final boolean externalPluginsEnabled) { + final boolean externalPluginsEnabled, + final boolean haltOnPluginError) { this.requestedPlugins = requestedPlugins; this.pluginsDir = pluginsDir; this.externalPluginsEnabled = externalPluginsEnabled; + this.haltOnPluginError = haltOnPluginError; } public List getRequestedPlugins() { @@ -47,6 +50,10 @@ public boolean isExternalPluginsEnabled() { return externalPluginsEnabled; } + public boolean isHaltOnPluginError() { + return haltOnPluginError; + } + public static Path defaultPluginsDir() { String pluginsDirProperty = System.getProperty("besu.plugins.dir"); return pluginsDirProperty == null @@ -62,6 +69,7 @@ public static class Builder { private List requestedPlugins; private Path pluginsDir; private boolean externalPluginsEnabled = true; + private boolean haltOnPluginError = false; public Builder requestedPlugins(final List requestedPlugins) { this.requestedPlugins = requestedPlugins; @@ -78,11 +86,17 @@ public Builder externalPluginsEnabled(final boolean externalPluginsEnabled) { return this; } + public Builder haltOnPluginError(final boolean haltOnPluginError) { + this.haltOnPluginError = haltOnPluginError; + return this; + } + public PluginConfiguration build() { if (pluginsDir == null) { pluginsDir = PluginConfiguration.defaultPluginsDir(); } - return new PluginConfiguration(requestedPlugins, pluginsDir, externalPluginsEnabled); + return new PluginConfiguration( + requestedPlugins, pluginsDir, externalPluginsEnabled, haltOnPluginError); } } } From f2acc53242f57a6db7fdd9a901bdc95618ef1382 Mon Sep 17 00:00:00 2001 From: Gabriel-Trintinalia Date: Mon, 23 Sep 2024 14:24:16 +1000 Subject: [PATCH 02/12] Add option to halt besu during plugin lifecycle Signed-off-by: Gabriel-Trintinalia --- .../acceptance/plugins/TestPicoCLIPlugin.java | 27 +++-- .../services/BesuPluginContextImplTest.java | 102 ++++++++++-------- .../besu/cli/PluginsOptionsTest.java | 37 +++++-- 3 files changed, 107 insertions(+), 59 deletions(-) diff --git a/acceptance-tests/test-plugins/src/main/java/org/hyperledger/besu/tests/acceptance/plugins/TestPicoCLIPlugin.java b/acceptance-tests/test-plugins/src/main/java/org/hyperledger/besu/tests/acceptance/plugins/TestPicoCLIPlugin.java index 53905866fbf..375fbd490ec 100644 --- a/acceptance-tests/test-plugins/src/main/java/org/hyperledger/besu/tests/acceptance/plugins/TestPicoCLIPlugin.java +++ b/acceptance-tests/test-plugins/src/main/java/org/hyperledger/besu/tests/acceptance/plugins/TestPicoCLIPlugin.java @@ -1,5 +1,5 @@ /* - * Copyright ConsenSys AG. + * Copyright contributors to Hyperledger Besu. * * 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 @@ -32,16 +32,25 @@ public class TestPicoCLIPlugin implements BesuPlugin { private static final Logger LOG = LoggerFactory.getLogger(TestPicoCLIPlugin.class); + private static final String UNSET = "UNSET"; + private static final String FAIL_REGISTER = "FAILREGISTER"; + private static final String FAIL_BEFORE_EXTERNAL_SERVICES = "FAILBEFOREEXTERNALSERVICES"; + private static final String FAIL_START = "FAILSTART"; + private static final String FAIL_AFTER_EXTERNAL_SERVICE_POST_MAIN_LOOP = + "FAILAFTEREXTERNALSERVICEPOSTMAINLOOP"; + private static final String FAIL_STOP = "FAILSTOP"; + private static final String PLUGIN_LIFECYCLE_PREFIX = "pluginLifecycle."; + @Option( names = {"--Xplugin-test-option"}, hidden = true, - defaultValue = "UNSET") + defaultValue = UNSET) String testOption = System.getProperty("testPicoCLIPlugin.testOption"); @Option( names = {"--plugin-test-stable-option"}, hidden = true, - defaultValue = "UNSET") + defaultValue = UNSET) String stableOption = ""; private String state = "uninited"; @@ -52,7 +61,7 @@ public void register(final BesuContext context) { LOG.info("Registering. Test Option is '{}'", testOption); state = "registering"; - if ("FAILREGISTER".equals(testOption)) { + if (FAIL_REGISTER.equals(testOption)) { state = "failregister"; throw new RuntimeException("I was told to fail at registration"); } @@ -71,7 +80,7 @@ public void beforeExternalServices() { LOG.info("Before external services. Test Option is '{}'", testOption); state = "beforeExternalServices"; - if ("FAILBEFOREEXTERNALSERVICES".equals(testOption)) { + if (FAIL_BEFORE_EXTERNAL_SERVICES.equals(testOption)) { state = "failbeforeExternalServices"; throw new RuntimeException("I was told to fail before external services"); } @@ -85,7 +94,7 @@ public void start() { LOG.info("Starting. Test Option is '{}'", testOption); state = "starting"; - if ("FAILSTART".equals(testOption)) { + if (FAIL_START.equals(testOption)) { state = "failstart"; throw new RuntimeException("I was told to fail at startup"); } @@ -99,7 +108,7 @@ public void afterExternalServicePostMainLoop() { LOG.info("After external services post main loop. Test Option is '{}'", testOption); state = "afterExternalServicePostMainLoop"; - if ("FAILAFTEREXTERNALSERVICEPOSTMAINLOOP".equals(testOption)) { + if (FAIL_AFTER_EXTERNAL_SERVICE_POST_MAIN_LOOP.equals(testOption)) { state = "failafterExternalServicePostMainLoop"; throw new RuntimeException("I was told to fail after external services post main loop"); } @@ -113,7 +122,7 @@ public void stop() { LOG.info("Stopping. Test Option is '{}'", testOption); state = "stopping"; - if ("FAILSTOP".equals(testOption)) { + if (FAIL_STOP.equals(testOption)) { state = "failstop"; throw new RuntimeException("I was told to fail at stop"); } @@ -131,7 +140,7 @@ public String getState() { @SuppressWarnings("ResultOfMethodCallIgnored") private void writeSignal(final String signal) { try { - final File callbackFile = new File(callbackDir, "pluginLifecycle." + signal); + final File callbackFile = new File(callbackDir, PLUGIN_LIFECYCLE_PREFIX + signal); if (!callbackFile.getParentFile().exists()) { callbackFile.getParentFile().mkdirs(); callbackFile.getParentFile().deleteOnExit(); diff --git a/acceptance-tests/test-plugins/src/test/java/org/hyperledger/besu/services/BesuPluginContextImplTest.java b/acceptance-tests/test-plugins/src/test/java/org/hyperledger/besu/services/BesuPluginContextImplTest.java index 46e54c26ad3..4255c54b8de 100644 --- a/acceptance-tests/test-plugins/src/test/java/org/hyperledger/besu/services/BesuPluginContextImplTest.java +++ b/acceptance-tests/test-plugins/src/test/java/org/hyperledger/besu/services/BesuPluginContextImplTest.java @@ -1,5 +1,5 @@ /* - * Copyright ConsenSys AG. + * Copyright contributors to Hyperledger Besu. * * 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 @@ -40,6 +40,22 @@ public class BesuPluginContextImplTest { private static final Path DEFAULT_PLUGIN_DIRECTORY = Paths.get("."); + private static final String TEST_PICO_CLI_PLUGIN = "TestPicoCLIPlugin"; + private static final String TEST_PICO_CLI_PLUGIN_TEST_OPTION = "testPicoCLIPlugin.testOption"; + private static final String FAIL_REGISTER = "FAILREGISTER"; + private static final String FAIL_START = "FAILSTART"; + private static final String FAIL_STOP = "FAILSTOP"; + private static final String FAIL_BEFORE_EXTERNAL_SERVICES = "FAILBEFOREEXTERNALSERVICES"; + private static final String FAIL_BEFORE_MAIN_LOOP = "FAILBEFOREMAINLOOP"; + private static final String FAIL_AFTER_EXTERNAL_SERVICE_POST_MAIN_LOOP = + "FAILAFTEREXTERNALSERVICEPOSTMAINLOOP"; + private static final String NON_EXISTENT_PLUGIN = "NonExistentPlugin"; + private static final String REGISTERED = "registered"; + private static final String STARTED = "started"; + private static final String STOPPED = "stopped"; + private static final String FAIL_START_STATE = "failstart"; + private static final String FAIL_STOP_STATE = "failstop"; + private BesuPluginContextImpl contextImpl; private static final PluginConfiguration DEFAULT_CONFIGURATION = @@ -56,7 +72,7 @@ public static void createFakePluginDir() throws IOException { @AfterEach public void clearTestPluginState() { - System.clearProperty("testPicoCLIPlugin.testOption"); + System.clearProperty(TEST_PICO_CLI_PLUGIN_TEST_OPTION); } @BeforeEach @@ -75,19 +91,19 @@ public void verifyEverythingGoesSmoothly() { findTestPlugin(contextImpl.getRegisteredPlugins(), TestPicoCLIPlugin.class); assertThat(testPluginOptional).isPresent(); final TestPicoCLIPlugin testPicoCLIPlugin = testPluginOptional.get(); - assertThat(testPicoCLIPlugin.getState()).isEqualTo("registered"); + assertThat(testPicoCLIPlugin.getState()).isEqualTo(REGISTERED); contextImpl.beforeExternalServices(); contextImpl.startPlugins(); - assertThat(testPicoCLIPlugin.getState()).isEqualTo("started"); + assertThat(testPicoCLIPlugin.getState()).isEqualTo(STARTED); contextImpl.stopPlugins(); - assertThat(testPicoCLIPlugin.getState()).isEqualTo("stopped"); + assertThat(testPicoCLIPlugin.getState()).isEqualTo(STOPPED); } @Test public void registrationErrorsHandledSmoothly() { - System.setProperty("testPicoCLIPlugin.testOption", "FAILREGISTER"); + System.setProperty(TEST_PICO_CLI_PLUGIN_TEST_OPTION, FAIL_REGISTER); assertThat(contextImpl.getRegisteredPlugins()).isEmpty(); contextImpl.initialize(DEFAULT_CONFIGURATION); @@ -106,7 +122,7 @@ public void registrationErrorsHandledSmoothly() { @Test public void startErrorsHandledSmoothly() { - System.setProperty("testPicoCLIPlugin.testOption", "FAILSTART"); + System.setProperty(TEST_PICO_CLI_PLUGIN_TEST_OPTION, FAIL_START); assertThat(contextImpl.getRegisteredPlugins()).isEmpty(); contextImpl.initialize(DEFAULT_CONFIGURATION); @@ -119,11 +135,11 @@ public void startErrorsHandledSmoothly() { findTestPlugin(contextImpl.getRegisteredPlugins(), TestPicoCLIPlugin.class); assertThat(testPluginOptional).isPresent(); final TestPicoCLIPlugin testPicoCLIPlugin = testPluginOptional.get(); - assertThat(testPicoCLIPlugin.getState()).isEqualTo("registered"); + assertThat(testPicoCLIPlugin.getState()).isEqualTo(REGISTERED); contextImpl.beforeExternalServices(); contextImpl.startPlugins(); - assertThat(testPicoCLIPlugin.getState()).isEqualTo("failstart"); + assertThat(testPicoCLIPlugin.getState()).isEqualTo(FAIL_START_STATE); assertThat(contextImpl.getRegisteredPlugins()).isNotInstanceOfAny(TestPicoCLIPlugin.class); contextImpl.stopPlugins(); @@ -132,7 +148,7 @@ public void startErrorsHandledSmoothly() { @Test public void stopErrorsHandledSmoothly() { - System.setProperty("testPicoCLIPlugin.testOption", "FAILSTOP"); + System.setProperty(TEST_PICO_CLI_PLUGIN_TEST_OPTION, FAIL_STOP); assertThat(contextImpl.getRegisteredPlugins()).isEmpty(); contextImpl.initialize(DEFAULT_CONFIGURATION); @@ -145,14 +161,14 @@ public void stopErrorsHandledSmoothly() { findTestPlugin(contextImpl.getRegisteredPlugins(), TestPicoCLIPlugin.class); assertThat(testPluginOptional).isPresent(); final TestPicoCLIPlugin testPicoCLIPlugin = testPluginOptional.get(); - assertThat(testPicoCLIPlugin.getState()).isEqualTo("registered"); + assertThat(testPicoCLIPlugin.getState()).isEqualTo(REGISTERED); contextImpl.beforeExternalServices(); contextImpl.startPlugins(); - assertThat(testPicoCLIPlugin.getState()).isEqualTo("started"); + assertThat(testPicoCLIPlugin.getState()).isEqualTo(STARTED); contextImpl.stopPlugins(); - assertThat(testPicoCLIPlugin.getState()).isEqualTo("failstop"); + assertThat(testPicoCLIPlugin.getState()).isEqualTo(FAIL_STOP_STATE); } @Test @@ -187,20 +203,20 @@ public void shouldRegisterAllPluginsWhenNoPluginsOption() { findTestPlugin(contextImpl.getRegisteredPlugins(), TestPicoCLIPlugin.class); assertThat(testPluginOptional).isPresent(); final TestPicoCLIPlugin testPicoCLIPlugin = testPluginOptional.get(); - assertThat(testPicoCLIPlugin.getState()).isEqualTo("registered"); + assertThat(testPicoCLIPlugin.getState()).isEqualTo(REGISTERED); } @Test public void shouldRegisterOnlySpecifiedPluginWhenPluginsOptionIsSet() { assertThat(contextImpl.getRegisteredPlugins()).isEmpty(); - contextImpl.initialize(createConfigurationForSpecificPlugin("TestPicoCLIPlugin")); + contextImpl.initialize(createConfigurationForSpecificPlugin(TEST_PICO_CLI_PLUGIN)); contextImpl.registerPlugins(); final Optional requestedPlugin = findTestPlugin(contextImpl.getRegisteredPlugins(), TestPicoCLIPlugin.class); assertThat(requestedPlugin).isPresent(); - assertThat(requestedPlugin.get().getState()).isEqualTo("registered"); + assertThat(requestedPlugin.get().getState()).isEqualTo(REGISTERED); final Optional nonRequestedPlugin = findTestPlugin(contextImpl.getRegisteredPlugins(), TestBesuEventsPlugin.class); @@ -211,7 +227,7 @@ public void shouldRegisterOnlySpecifiedPluginWhenPluginsOptionIsSet() { @Test public void shouldNotRegisterUnspecifiedPluginsWhenPluginsOptionIsSet() { assertThat(contextImpl.getRegisteredPlugins()).isEmpty(); - contextImpl.initialize(createConfigurationForSpecificPlugin("TestPicoCLIPlugin")); + contextImpl.initialize(createConfigurationForSpecificPlugin(TEST_PICO_CLI_PLUGIN)); contextImpl.registerPlugins(); final Optional nonRequestedPlugin = @@ -221,12 +237,12 @@ public void shouldNotRegisterUnspecifiedPluginsWhenPluginsOptionIsSet() { @Test void shouldThrowExceptionIfExplicitlySpecifiedPluginNotFound() { - contextImpl.initialize(createConfigurationForSpecificPlugin("NonExistentPlugin")); + contextImpl.initialize(createConfigurationForSpecificPlugin(NON_EXISTENT_PLUGIN)); String exceptionMessage = assertThrows(NoSuchElementException.class, () -> contextImpl.registerPlugins()) .getMessage(); final String expectedMessage = - "The following requested plugins were not found: NonExistentPlugin"; + "The following requested plugins were not found: " + NON_EXISTENT_PLUGIN; assertThat(exceptionMessage).isEqualTo(expectedMessage); assertThat(contextImpl.getRegisteredPlugins()).isEmpty(); } @@ -256,12 +272,12 @@ void shouldRegisterPluginsIfExternalPluginsEnabled() { } @Test - void shouldHaltOnPluginErrorWhenFlagIsTrue() { - System.setProperty("testPicoCLIPlugin.testOption", "FAILREGISTER"); + void shouldHaltOnRegisterErrorWhenFlagIsTrue() { + System.setProperty(TEST_PICO_CLI_PLUGIN_TEST_OPTION, FAIL_REGISTER); PluginConfiguration config = PluginConfiguration.builder() - .requestedPlugins(List.of(new PluginInfo("TestPicoCLIPlugin"))) + .requestedPlugins(List.of(new PluginInfo(TEST_PICO_CLI_PLUGIN))) .pluginsDir(DEFAULT_PLUGIN_DIRECTORY) .haltOnPluginError(true) .build(); @@ -276,8 +292,8 @@ void shouldHaltOnPluginErrorWhenFlagIsTrue() { } @Test - void shouldNotHaltOnPluginErrorWhenFlagIsFalse() { - System.setProperty("testPicoCLIPlugin.testOption", "FAILREGISTER"); + void shouldNotHaltOnRegisterErrorWhenFlagIsFalse() { + System.setProperty(TEST_PICO_CLI_PLUGIN_TEST_OPTION, FAIL_REGISTER); PluginConfiguration config = PluginConfiguration.builder() @@ -291,12 +307,12 @@ void shouldNotHaltOnPluginErrorWhenFlagIsFalse() { } @Test - void shouldHaltOnPluginBeforeExternalServicesErrorWhenFlagIsTrue() { - System.setProperty("testPicoCLIPlugin.testOption", "FAILBEFOREEXTERNALSERVICES"); + void shouldHaltOnBeforeExternalServicesErrorWhenFlagIsTrue() { + System.setProperty(TEST_PICO_CLI_PLUGIN_TEST_OPTION, FAIL_BEFORE_EXTERNAL_SERVICES); PluginConfiguration config = PluginConfiguration.builder() - .requestedPlugins(List.of(new PluginInfo("TestPicoCLIPlugin"))) + .requestedPlugins(List.of(new PluginInfo(TEST_PICO_CLI_PLUGIN))) .pluginsDir(DEFAULT_PLUGIN_DIRECTORY) .haltOnPluginError(true) .build(); @@ -314,12 +330,12 @@ void shouldHaltOnPluginBeforeExternalServicesErrorWhenFlagIsTrue() { } @Test - void shouldNotHaltOnPluginBeforeExternalServicesErrorWhenFlagIsFalse() { - System.setProperty("testPicoCLIPlugin.testOption", "FAILBEFOREEXTERNALSERVICES"); + void shouldNotHaltOnBeforeExternalServicesErrorWhenFlagIsFalse() { + System.setProperty(TEST_PICO_CLI_PLUGIN_TEST_OPTION, FAIL_BEFORE_EXTERNAL_SERVICES); PluginConfiguration config = PluginConfiguration.builder() - .requestedPlugins(List.of(new PluginInfo("TestPicoCLIPlugin"))) + .requestedPlugins(List.of(new PluginInfo(TEST_PICO_CLI_PLUGIN))) .pluginsDir(DEFAULT_PLUGIN_DIRECTORY) .haltOnPluginError(false) .build(); @@ -332,12 +348,12 @@ void shouldNotHaltOnPluginBeforeExternalServicesErrorWhenFlagIsFalse() { } @Test - void shouldHaltOnPluginBeforeMainLoopErrorWhenFlagIsTrue() { - System.setProperty("testPicoCLIPlugin.testOption", "FAILSTART"); + void shouldHaltOnBeforeMainLoopErrorWhenFlagIsTrue() { + System.setProperty(TEST_PICO_CLI_PLUGIN_TEST_OPTION, FAIL_START); PluginConfiguration config = PluginConfiguration.builder() - .requestedPlugins(List.of(new PluginInfo("TestPicoCLIPlugin"))) + .requestedPlugins(List.of(new PluginInfo(TEST_PICO_CLI_PLUGIN))) .pluginsDir(DEFAULT_PLUGIN_DIRECTORY) .haltOnPluginError(true) .build(); @@ -354,12 +370,12 @@ void shouldHaltOnPluginBeforeMainLoopErrorWhenFlagIsTrue() { } @Test - void shouldNotHaltOnPluginBeforeMainLoopErrorWhenFlagIsFalse() { - System.setProperty("testPicoCLIPlugin.testOption", "FAILBEFOREMAINLOOP"); + void shouldNotHaltOnBeforeMainLoopErrorWhenFlagIsFalse() { + System.setProperty(TEST_PICO_CLI_PLUGIN_TEST_OPTION, FAIL_BEFORE_MAIN_LOOP); PluginConfiguration config = PluginConfiguration.builder() - .requestedPlugins(List.of(new PluginInfo("TestPicoCLIPlugin"))) + .requestedPlugins(List.of(new PluginInfo(TEST_PICO_CLI_PLUGIN))) .pluginsDir(DEFAULT_PLUGIN_DIRECTORY) .haltOnPluginError(false) .build(); @@ -373,12 +389,13 @@ void shouldNotHaltOnPluginBeforeMainLoopErrorWhenFlagIsFalse() { } @Test - void shouldHaltOnPluginAfterExternalServicePostMainLoopErrorWhenFlagIsTrue() { - System.setProperty("testPicoCLIPlugin.testOption", "FAILAFTEREXTERNALSERVICEPOSTMAINLOOP"); + void shouldHaltOnAfterExternalServicePostMainLoopErrorWhenFlagIsTrue() { + System.setProperty( + TEST_PICO_CLI_PLUGIN_TEST_OPTION, FAIL_AFTER_EXTERNAL_SERVICE_POST_MAIN_LOOP); PluginConfiguration config = PluginConfiguration.builder() - .requestedPlugins(List.of(new PluginInfo("TestPicoCLIPlugin"))) + .requestedPlugins(List.of(new PluginInfo(TEST_PICO_CLI_PLUGIN))) .pluginsDir(DEFAULT_PLUGIN_DIRECTORY) .haltOnPluginError(true) .build(); @@ -398,12 +415,13 @@ void shouldHaltOnPluginAfterExternalServicePostMainLoopErrorWhenFlagIsTrue() { } @Test - void shouldNotHaltOnPluginAfterExternalServicePostMainLoopErrorWhenFlagIsFalse() { - System.setProperty("testPicoCLIPlugin.testOption", "FAILAFTEREXTERNALSERVICEPOSTMAINLOOP"); + void shouldNotHaltOnAfterExternalServicePostMainLoopErrorWhenFlagIsFalse() { + System.setProperty( + TEST_PICO_CLI_PLUGIN_TEST_OPTION, FAIL_AFTER_EXTERNAL_SERVICE_POST_MAIN_LOOP); PluginConfiguration config = PluginConfiguration.builder() - .requestedPlugins(List.of(new PluginInfo("TestPicoCLIPlugin"))) + .requestedPlugins(List.of(new PluginInfo(TEST_PICO_CLI_PLUGIN))) .pluginsDir(DEFAULT_PLUGIN_DIRECTORY) .haltOnPluginError(false) .build(); diff --git a/besu/src/test/java/org/hyperledger/besu/cli/PluginsOptionsTest.java b/besu/src/test/java/org/hyperledger/besu/cli/PluginsOptionsTest.java index 1adfb585e9d..b0165cc76c2 100644 --- a/besu/src/test/java/org/hyperledger/besu/cli/PluginsOptionsTest.java +++ b/besu/src/test/java/org/hyperledger/besu/cli/PluginsOptionsTest.java @@ -33,7 +33,7 @@ public class PluginsOptionsTest extends CommandTestAbstract { @Test public void shouldParsePluginOptionForSinglePlugin() { parseCommand("--plugins", "pluginA"); - verify(mockBesuPluginContext).registerPlugins(pluginConfigurationArgumentCaptor.capture()); + verify(mockBesuPluginContext).initialize(pluginConfigurationArgumentCaptor.capture()); assertThat(pluginConfigurationArgumentCaptor.getValue().getRequestedPlugins()) .isEqualTo(List.of("pluginA")); assertThat(commandOutput.toString(UTF_8)).isEmpty(); @@ -43,7 +43,7 @@ public void shouldParsePluginOptionForSinglePlugin() { @Test public void shouldParsePluginOptionForMultiplePlugins() { parseCommand("--plugins", "pluginA,pluginB"); - verify(mockBesuPluginContext).registerPlugins(pluginConfigurationArgumentCaptor.capture()); + verify(mockBesuPluginContext).initialize(pluginConfigurationArgumentCaptor.capture()); assertThat(pluginConfigurationArgumentCaptor.getValue().getRequestedPlugins()) .isEqualTo(List.of("pluginA", "pluginB")); @@ -54,7 +54,7 @@ public void shouldParsePluginOptionForMultiplePlugins() { @Test public void shouldNotUsePluginOptionWhenNoPluginsSpecified() { parseCommand(); - verify(mockBesuPluginContext).registerPlugins(pluginConfigurationArgumentCaptor.capture()); + verify(mockBesuPluginContext).initialize(pluginConfigurationArgumentCaptor.capture()); assertThat(pluginConfigurationArgumentCaptor.getValue().getRequestedPlugins()) .isEqualTo(List.of()); assertThat(commandOutput.toString(UTF_8)).isEmpty(); @@ -64,7 +64,7 @@ public void shouldNotUsePluginOptionWhenNoPluginsSpecified() { @Test public void shouldNotParseAnyPluginsWhenPluginOptionIsEmpty() { parseCommand("--plugins", ""); - verify(mockBesuPluginContext).registerPlugins(pluginConfigurationArgumentCaptor.capture()); + verify(mockBesuPluginContext).initialize(pluginConfigurationArgumentCaptor.capture()); assertThat(pluginConfigurationArgumentCaptor.getValue().getRequestedPlugins()) .isEqualTo(List.of()); assertThat(commandOutput.toString(UTF_8)).isEmpty(); @@ -74,7 +74,7 @@ public void shouldNotParseAnyPluginsWhenPluginOptionIsEmpty() { @Test public void shouldParsePluginsExternalEnabledOptionWhenFalse() { parseCommand("--Xplugins-external-enabled=false"); - verify(mockBesuPluginContext).registerPlugins(pluginConfigurationArgumentCaptor.capture()); + verify(mockBesuPluginContext).initialize(pluginConfigurationArgumentCaptor.capture()); assertThat(pluginConfigurationArgumentCaptor.getValue().isExternalPluginsEnabled()) .isEqualTo(false); @@ -86,7 +86,7 @@ public void shouldParsePluginsExternalEnabledOptionWhenFalse() { @Test public void shouldParsePluginsExternalEnabledOptionWhenTrue() { parseCommand("--Xplugins-external-enabled=true"); - verify(mockBesuPluginContext).registerPlugins(pluginConfigurationArgumentCaptor.capture()); + verify(mockBesuPluginContext).initialize(pluginConfigurationArgumentCaptor.capture()); assertThat(pluginConfigurationArgumentCaptor.getValue().isExternalPluginsEnabled()) .isEqualTo(true); @@ -98,7 +98,7 @@ public void shouldParsePluginsExternalEnabledOptionWhenTrue() { @Test public void shouldEnablePluginsExternalByDefault() { parseCommand(); - verify(mockBesuPluginContext).registerPlugins(pluginConfigurationArgumentCaptor.capture()); + verify(mockBesuPluginContext).initialize(pluginConfigurationArgumentCaptor.capture()); assertThat(pluginConfigurationArgumentCaptor.getValue().isExternalPluginsEnabled()) .isEqualTo(true); @@ -109,10 +109,31 @@ public void shouldEnablePluginsExternalByDefault() { @Test public void shouldFailWhenPluginsIsDisabledAndPluginsExplicitlyRequested() { parseCommand("--Xplugins-external-enabled=false", "--plugins", "pluginA"); - verify(mockBesuPluginContext).registerPlugins(pluginConfigurationArgumentCaptor.capture()); + verify(mockBesuPluginContext).initialize(pluginConfigurationArgumentCaptor.capture()); assertThat(commandOutput.toString(UTF_8)).isEmpty(); assertThat(commandErrorOutput.toString(UTF_8)) .contains("--plugins option can only be used when --Xplugins-external-enabled is true"); } + + @Test + public void shouldHaveHaltOnErrorFalseByDefault() { + parseCommand(); + verify(mockBesuPluginContext).initialize(pluginConfigurationArgumentCaptor.capture()); + assertThat(pluginConfigurationArgumentCaptor.getValue().isHaltOnPluginError()).isEqualTo(false); + + assertThat(commandOutput.toString(UTF_8)).isEmpty(); + assertThat(commandErrorOutput.toString(UTF_8)).isEmpty(); + } + + @Test + public void shouldUseHaltOnErrorWhenTrue() { + parseCommand("--halt-on-plugin-error=true"); + verify(mockBesuPluginContext).initialize(pluginConfigurationArgumentCaptor.capture()); + + assertThat(pluginConfigurationArgumentCaptor.getValue().isHaltOnPluginError()).isEqualTo(true); + + assertThat(commandOutput.toString(UTF_8)).isEmpty(); + assertThat(commandErrorOutput.toString(UTF_8)).isEmpty(); + } } From a743376614b1044fb19bb6e378afb24681b1df62 Mon Sep 17 00:00:00 2001 From: Gabriel-Trintinalia Date: Mon, 23 Sep 2024 14:28:34 +1000 Subject: [PATCH 03/12] Add option to halt besu during plugin lifecycle Signed-off-by: Gabriel-Trintinalia --- .../hyperledger/besu/services/BesuPluginContextImpl.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/besu/src/main/java/org/hyperledger/besu/services/BesuPluginContextImpl.java b/besu/src/main/java/org/hyperledger/besu/services/BesuPluginContextImpl.java index 617bf21a643..b3fa146fd41 100644 --- a/besu/src/main/java/org/hyperledger/besu/services/BesuPluginContextImpl.java +++ b/besu/src/main/java/org/hyperledger/besu/services/BesuPluginContextImpl.java @@ -244,8 +244,8 @@ public void beforeExternalServices() { "Error calling `beforeExternalServices` on plugin of type {}, start will not be called.", plugin.getClass().getName(), e); + pluginsIterator.remove(); } - pluginsIterator.remove(); } } @@ -278,8 +278,8 @@ public void startPlugins() { "Error starting plugin of type {}, stop will not be called.", plugin.getClass().getName(), e); + pluginsIterator.remove(); } - pluginsIterator.remove(); } } @@ -312,8 +312,8 @@ public void afterExternalServicesMainLoop() { + plugin.getClass().getName() + ", stop will not be called.", e); + pluginsIterator.remove(); } - pluginsIterator.remove(); } } } From 5026a827932bf75c3ce539c4c6c643ee97cec10a Mon Sep 17 00:00:00 2001 From: Gabriel-Trintinalia Date: Mon, 23 Sep 2024 16:34:39 +1000 Subject: [PATCH 04/12] Add option to halt besu during plugin lifecycle Signed-off-by: Gabriel-Trintinalia --- .../cli/options/stable/PluginsConfigurationOptions.java | 2 +- .../hyperledger/besu/services/BesuPluginContextImpl.java | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/besu/src/main/java/org/hyperledger/besu/cli/options/stable/PluginsConfigurationOptions.java b/besu/src/main/java/org/hyperledger/besu/cli/options/stable/PluginsConfigurationOptions.java index eca40ffb527..6443e36e410 100644 --- a/besu/src/main/java/org/hyperledger/besu/cli/options/stable/PluginsConfigurationOptions.java +++ b/besu/src/main/java/org/hyperledger/besu/cli/options/stable/PluginsConfigurationOptions.java @@ -44,7 +44,7 @@ public class PluginsConfigurationOptions implements CLIOptions