Mark unrecoverable errors so they don't spawn #412
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
Mark unrecoverable sender errors in storage so that they no longer spawn isolates. Spawning isolates and failing them immediately uses more resources than necessary. This is therefore a performance improvement for the stability of the app. In the medium term we should switch on errors from payjoin-flutter, but those errors are not yet available to switch on. This will reduce the amount of error definition and handling we have to manually implement, but since this is a potential immediate performance issue I'm addressing it straight away.
DanGould
force-pushed
the
handle-unrecoverable-pj
branch
from
1cf3a0b to
fd2fc86
Compare
This way they don't respawn if they're never going to succeed.
DanGould
changed the title
kumulynja
reviewed
Jan 3, 2025
lib/_pkg/payjoin/manager.dart
Outdated
| @@ -281,6 +301,9 @@ class PayjoinManager { | |||
|
|
|||
| return completer.future; | |||
| } catch (e) { | |||
| if (e is UnrecoverableError) { | |||
There was a problem hiding this comment.
@DanGould I think this never gets here and should be moved to the receive port listener where the UnrecoverableError is send to from the isolate. So I guess instead of the message is Err on line 283, check for and handle the UnrecoverableError there.
There was a problem hiding this comment.
I made this change as well as one where if errors in the switch statement crop up we mark the receiver payjoin session as unrecoverable which I detail in the commit message. Let me know what you think.
Receivers need to handle both switch errors or isolate errors. This change takes the error handling out of the spawn try-catch, which would never receive them and moves it into places where those errors would propagate. Note that some of the switch errors like checkIsOwned might crop up because of a wallet being improperly loaded, but for the time being we don't have the granularity to distinguish between them.
|
Thanks @DanGould! Thanks @kumulynja and @bitcoin-b for the review! |
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.
Mark unrecoverable errors in storage so that they no longer spawn isolates.
Spawning isolates and failing them immediately uses more resources than necessary. This is therefore a performance improvement for the stability of the app.
In the medium term we should switch on errors from payjoin-flutter, but those errors are not yet available to switch on. This will reduce the amount of error definition and handling we have to manually implement, but since this is a potential immediate performance issue I'm addressing it straight away.