pyln: Add safe fallback results for hooks #4031
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
cdecker
force-pushed
the
htlc-accepted-crash
branch
from
71bf992
to
8d4ddc3
Compare
cdecker
force-pushed
the
htlc-accepted-crash
branch
from
8d4ddc3
to
b7e3829
Compare
niftynei
reviewed
Sep 9, 2020
| r'Hook handler for htlc_accepted failed with an exception.' | ||
| ) | ||
|
|
||
| with pytest.raises(RpcError, match=r'failed: WIRE_TEMPORARY_NODE_FAILURE'): |
There was a problem hiding this comment.
should we also test for one of the 'non-recoverable' hooks, db_write or commitment_revocation
There was a problem hiding this comment.
I'll look into it, the semantics are a bit different though (db_write hangs indefinitely for example).
Hooks do not tolerate failures at all. If we return a JSON-RPC error to a hook call the only thing the main daemon can really do is to crash. This commit adds a mapping of error to a safe fallback result, including a warning to the node operator that this should be addressed in the plugin. The warning is reported as a `**BROKEN**` message, and should therefore fail any testing done on the plugin. Changelog-Fixed: pyln: Fixed HTLCs hanging indefinitely if the hook function raises an exception. A safe fallback result is now returned instead.
cdecker
force-pushed
the
htlc-accepted-crash
branch
from
b7e3829
to
7f0db34
Compare
Good catch, fixed up :-) |
|
Ack 7f0db34 |
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.
Hooks do not tolerate failures at all. If we return a JSON-RPC error to a hook
call the only thing the main daemon can really do is to crash. This commit
adds a mapping of error to a safe fallback result, including a warning to the
node operator that this should be addressed in the plugin. The warning is
reported as a
**BROKEN**message, and should therefore fail any testing doneon the plugin.
The alternatives would have been
lightningd. But I don't like relaxing the requirements on this. It feels magic enough as it is, but prevents us from accidentally hanging due to a plugin.Fixes #3748