staticaddr: fix deposit recovery deadlock #954
Conversation
hieblmi
force-pushed
the
fix-deadlock
branch
3 times, most recently
from
eb832fd to
7bd246e
Compare
hieblmi
force-pushed
the
fix-deadlock
branch
3 times, most recently
from
de3c60e to
8285bc7
Compare
| if err != nil { | ||
| log.Errorf("Error sending OnStart event: %v", err) | ||
| } | ||
| go func(fsm *FSM) { |
There was a problem hiding this comment.
I wonder what the tradeoff is between having this in a go func or more asynchronous fsm actions? from what I can tell in effect it is the same?
There was a problem hiding this comment.
But with most recent projects I tend to force tha actions to be asynchronous as this means we can go events only vs listening for channels everywhere where we need to interact with a blocking action
starius
left a comment
starius
left a comment
There was a problem hiding this comment.
LGTM! 🌴
One proposal to prevent this issue from silently recurring.
| log.Errorf("Error sending OnStart event: %v", | ||
| err) | ||
| } | ||
| }(fsm) |
There was a problem hiding this comment.
I propose to add a unit test that would fail with the dead-lock before this change and to make sure this goroutine doesn't leak in that test.
There was a problem hiding this comment.
Given that is is a pattern we use frequently I only left a unit-test todo here.
3 participants
Fixes a deadlock in the deposit recovery that arises if deposits expired while the client was offline.
Since
recoverDepositsblocks onfsm.SendEvent(ctx, OnRecover, nil)the main run loop never gets a chance to read fromm.finalizedDepositChan.