Add early abort in case of misconfigured plugins #4418
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We were reporting the failure immediately but still continuing with the startup. This could happen if an important plugin ends up in a race with another plugin (important or not) for a contended resource (CLI option or RPC method name). We would eventually notice that we were supposed to abort, but at that point we already processed a couple of blocks, loaded the entire state, etc. This just aborts early with a sane error message. Changelog-Added: plugin: If there is a misconfiguration with important plugins we now abort early with a more descriptive error message. Reported-by: PsySc0rpi0n Reported-by: Ján Sáreník <@jsarenik>
cdecker
changed the title
|
Thank you @cdecker! Currently it hangs which is pretty good because it would not infinitely restart on UPDATE: NACK 7bafc57 (see below) |
|
What do others think? Would you prefer a hanging |
jsarenik
suggested changes
Mar 9, 2021
| * effort in starting up. | ||
| */ | ||
| if (ld->exit_code) | ||
| fatal("Could not initialize the plugins, see above for details."); |
There was a problem hiding this comment.
@cdecker seemingly I never get here
There was a problem hiding this comment.
Apologies, I thought that important flag is set in plugin details automatically. All good now. Thank you.
|
That's strange, it doesn't hang when I test it locally. Hanging is definitely the wrong thing to do here. |
|
Notice that we don't exit if a non-important plugin dies. Please use |
|
@cdecker Thanks for info. ACK 7bafc57 |
jsarenik
approved these changes
Mar 10, 2021
| * effort in starting up. | ||
| */ | ||
| if (ld->exit_code) | ||
| fatal("Could not initialize the plugins, see above for details."); |
There was a problem hiding this comment.
Apologies, I thought that important flag is set in plugin details automatically. All good now. Thank you.
niftynei
approved these changes
Mar 10, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are ways to misconfigure plugins that will cause us to shut
down. These include having an important plugin fight for a method name
with another plugin, e.g., by loading the same plugin twice. Until now
we would happily continue with the startup sequence only to later
realize that we were supposed to shutdown, but by then we'd have
buried the error message in the logs.
This PR just adds a check for misconfigurations before we even start
the normal
io_loop, avoiding expensive initializations and nothiding the log entries about the cause. This works if we're just using
--helpas well, and produces a much clearer picture of what is goingon.
I also added the name of the plugin that was clashing with RPC methods.
Fixes #4415
Reported-by: PsySc0rpi0n
Reported-by: Ján Sáreník <@jsarenik>