[MPP] "slow settle" for TrackPayment #4142
Conversation
Similar to what is done for SettleAttempt.
In case settling an attempt doesn't lead to the payment moving to the success status (if there are more attempts still in flight), we don't send the success event yet. We wait until all shards are done, and the paymetnt move to the final success state. For failures we do the same, we don't notify about failures before we are certain the payment has moved completely to the Failed state.
| // can happen if the payment had no more attempts in flight at this | ||
| // point, and recording this payment failure took the payment to a | ||
| // final failure state. | ||
| p.notifyFinalEvent(payment) |
There was a problem hiding this comment.
If we don't notify here, because there are still htlcs in-flight, is this Fail method guarantueed to be called again? (does every failed htlc eventually trigger a call to this method?)
There was a problem hiding this comment.
No. That's why we also call it in FailAttemp above
| // should notify all subscribers of the final payment status. This can | ||
| // happen if the payment already had a settled attempt or terminal | ||
| // failure recorded, and this attempt was the last one in-flight. | ||
| p.notifyFinalEvent(payment) |
There was a problem hiding this comment.
Isn't this causing a duplicate event?
There was a problem hiding this comment.
Idea is that only the first event that take the payment to Settle/failed state will produce the notification. The second one will find the payment also in this state, but by that time the subscribers list will be empty (and cannot be filled again since we only add to it for in-flight payments)
There was a problem hiding this comment.
And there is no race condition possible there?
There was a problem hiding this comment.
I think the first one that gets the lock on the subscribers will empty the list, and at that point it is impossible to add more to the list, so I think it is okay.
If we merge #3970 first I can add some more testcases here for MPP scenarios.
There was a problem hiding this comment.
I was actually thinking about a race between subscribe and notify, because the payment update in FailAttempt isn't inside the subscriber lock.
I think the bottom line of this discussion is that the concurrent behavior isn't very clear. There is potentially a duplicate notification, but it is never broadcast because the subscriber lock makes sure the list is already empty.
Also control tower notification needs to be updated anyway to sends updates for non-final state transitions.
Isn't a single PR on top of #3970 that fixes both things a better idea?
There was a problem hiding this comment.
in order to be totally consistent i think, yes, there would need to be a lock around the db operation as well. this is preexisting tho, no?
There was a problem hiding this comment.
This is the direction that I am thinking of atm: master...joostjager:notify-htlc-arrival-2
Similar to what is done currently for
ListPayments, we only report theSuccess/Failedstate of a payment when there is no longer any payment shards in flight.