Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Force besu to stop on plugin initialization errors #7662

Conversation

Gabriel-Trintinalia
Copy link
Contributor

@Gabriel-Trintinalia Gabriel-Trintinalia commented Sep 23, 2024

PR Description

This PR changes the initialization behaviour of plugins in the BesuPluginContextImpl class by introducing a pluginContinueOnError flag. By default, Besu will not allow plugins to fail during registration and other startup lifecycle stages.

Changes Made

pluginContinueOnError Flag:

  • Introduced the --plugin-continue-on-error CLI option (default to false).
  • Added a pluginContinueOnError flag to the PluginConfiguration class.

Lifecycle Methods:

  • Updated the registerPlugins, beforeExternalServices, startPlugins, and afterExternalServicesMainLoop methods to check the pluginContinueOnError flag. If a plugin fails and the flag is set to true, Besu will log the error and continue running. If the flag is set to false, Besu will throw a RuntimeException and halt.

Usage Example

besu --plugin-continue-on-error=true

In this example, if any plugin fails during registration, before external services, startup, or after external services post main loop, Besu will log the error and continue running. If the --plugin-continue-on-error option is set to false (default), Besu will throw a RuntimeException and halt gracefully, ensuring that plugins cannot fail registration.

Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Signed-off-by: Gabriel-Trintinalia <[email protected]>
@Gabriel-Trintinalia Gabriel-Trintinalia marked this pull request as ready for review September 23, 2024 08:16
Copy link
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just add a CHANGELOG entry and doc-change-required label.

Then I am thinking, that even if it will be a breaking changes, to set the default to true, at least to me it seems the normal behavior to fail in case a plugin has an issue. WDYT?

@Gabriel-Trintinalia Gabriel-Trintinalia added the doc-change-required Indicates an issue or PR that requires doc to be updated label Sep 24, 2024
@Gabriel-Trintinalia
Copy link
Contributor Author

Gabriel-Trintinalia commented Sep 24, 2024

LGTM, just add a CHANGELOG entry and doc-change-required label.

Then I am thinking, that even if it will be a breaking changes, to set the default to true, at least to me it seems the normal behavior to fail in case a plugin has an issue. WDYT?

I agree with stopping the startup when the plugin fails by default. That is probably the right behaviour in the majority of setups.

@Gabriel-Trintinalia Gabriel-Trintinalia changed the title Add --halt-on-plugin-error CLI Option to ensure successful plugins registration Force besu to stop on plugin initialization errors Sep 24, 2024
Signed-off-by: Gabriel-Trintinalia <[email protected]>
Copy link
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a last rename

Signed-off-by: Gabriel-Trintinalia <[email protected]>
@Gabriel-Trintinalia Gabriel-Trintinalia enabled auto-merge (squash) September 24, 2024 10:14
@Gabriel-Trintinalia Gabriel-Trintinalia merged commit e0518c6 into hyperledger:main Sep 24, 2024
43 checks passed
Wolmin pushed a commit to lukso-network/network-besu that referenced this pull request Sep 27, 2024
This was referenced Oct 18, 2024
@alexandratran alexandratran removed the doc-change-required Indicates an issue or PR that requires doc to be updated label Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants