Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sidecar: inform remote side about cancellation #304

Merged
merged 3 commits into from
Nov 2, 2021
Merged

Conversation

guggero
Copy link
Member

@guggero guggero commented Oct 27, 2021

Fixes #295.
Fixes #294.

This PR fixes the issue that when one side of a sidecar channel offering cancels the ticket, the other side isn't informed about it. This leads to follow-up errors on restarts and subsequent ticket submissions.

We solve this by sending a message to the other side if we are the cancelling party.
Because we send the message over the mailbox, we cannot remove it in that case.

This commit fixes an instance of a "break" keyword that didn't have the
desired effect (breaking out of the outer for loop).
This didn't lead to an endless loop just because of the break in the
error case a few lines above where we would leave the loop in case of an
invalid state transition.
auto_sidecar.go Show resolved Hide resolved
auto_sidecar.go Show resolved Hide resolved
To avoid the other side waiting for an update for a ticket that will
never come if it is canceled, we inform them instead.
To avoid running into an error during automatic bid order submission
when offering a sidecar channel, we need to make the --min_chan_amt flag
of the order mandatory.
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice solution! Did an initial pass, need to do another to ensure I've wrapped my head around how this handles some of the disconnect/retransmission edge cases.

auto_sidecar.go Show resolved Hide resolved
return
}

// In every other case we can just remote the mailbox:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remote -> remove

log.Errorf("unable to send cancel "+
"msg to recipient: %v", err)
return
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should fully tear down the goroutine here since at this point all negotiation has ceased. Same for the opposite role as well.

@Roasbeef
Copy link
Member

Roasbeef commented Nov 2, 2021

Gonna land this so we can proceed with some of the dependent merges, ideally can address those lingering commits in a follow up change

@Roasbeef Roasbeef merged commit f566f3a into master Nov 2, 2021
@guggero guggero deleted the cancel-sidecar branch November 3, 2021 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants