-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
routing: launch fetchFundingTx in goroutine so router can exit #8151
Conversation
@@ -1796,6 +1796,36 @@ func (r *ChannelRouter) processUpdate(msg interface{}, | |||
return nil | |||
} | |||
|
|||
// fetchFundingTxWrapper is a wrapper around fetchFundingTx, except that it | |||
// will exit if the router has stopped. | |||
func (r *ChannelRouter) fetchFundingTxWrapper(chanID *lnwire.ShortChannelID) ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love to see this generalized. The pattern here is that you have some sync function that you'd like to make async and multiplexed over an alternative quit signal.
func RunAsyncWithCancel(func()(A, error), <-chan struct{}, error) (A, error) { ... }
then call with
RunAsyncWithCancel(func() { return r.fetchFundingTx(&channelID) }, r.quit, ErrRouterShuttingDown)
You can place this in the new fn
package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed that this typechecks and compiles:
func RunAsyncWithCancel[A any](
f func() (A, error),
quitSig <-chan struct{},
quitErr error,
) (*A, error) {
resChan := make(chan A, 1)
errChan := make(chan error, 1)
go func() {
res, err := f()
if err != nil {
errChan <- err
} else {
resChan <- res
}
}()
select {
case res := <- resChan:
return &res, nil
case err := <- errChan:
return nil, err
case <-quitSig:
return nil, quitErr
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea, but I think it's better left to another PR. I like to keep my PRs very minimal. In another PR, we could replace call-sites in one sweep (I think the itests use this pattern a lot)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a project management mechanism for scheduling these sorts of things? If not, it will never get prioritized and we won't be able to reap the benefits of having improved abstractions. If so, what do we need to do to get it into the queue? Or do I just need to do it when I have a spare moment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can make a PR and I'll review it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor change I'd suggest but isn't required. I'd like to start extracting common patterns out and making them broadly available so we don't have to keep writing them over and over again. I think this is a good candidate here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ✅
I like the change suggested by @ProofOfKeags too 👍
@Crypt-iQ, remember to re-request review from reviewers when ready |
This commit introduces a wrapper function fetchFundingTxWrapper which calls fetchFundingTx in a goroutine. This is to avoid an issue with pruned nodes where the router is attempting to stop, but the prunedBlockDispatcher is waiting to connect to peers that can serve the block. This can cause the shutdown process to hang until we connect to a peer that can send us the block.
This commit introduces a wrapper function
fetchFundingTxWrapper
which callsfetchFundingTx
in a goroutine. This is to avoid an issue with pruned nodes where the router is attempting to stop, but theprunedBlockDispatcher
is waiting to connect to peers that can serve the block. This can cause the shutdown process to hang until we connect to a peer that can send us the block. Marking as draft for now until I can test this. I opted for this approach rather than passing in aquit
channel toGetBlock
.See #7928 (comment) for more info