Pass context.Context to GetUtxo #2770
Conversation
This will make sure a long-running rescan can be canceled in case Neutrino is the backend.
This will make sure a long-running rescan can be canceled in the case the notifier is shutting down.
halseth
force-pushed
the
getutxo-context
branch
from
6e12c97
to
c25e14d
Compare
| @@ -972,10 +973,26 @@ func (l *LightningWallet) handleFundingCounterPartySigs(msg *addCounterPartySigs | |||
| ) | |||
| if err != nil { | |||
| } | |||
|
|
|||
| ctx, cancel := context.WithCancel(context.Background()) | |||
There was a problem hiding this comment.
since our goal is simply to break on shutdown, we should only need to make one context.Context for the whole LightningWallet. the ctx and cancel can be tracked as member variables, and cancel only needs to invoked when Stopis called. similar comment applies for router
There was a problem hiding this comment.
We could, but that seems like a big deviation from our normal pattern? I like sticking with the simple cancel chan tbh.
There was a problem hiding this comment.
what's the normal pattern? having one context for all concurrent wallet/router GetUtxo requests seems more in line with how we would otherwise pass the object's quit channel, like in #2716?
There was a problem hiding this comment.
I think having a context.Background makes sense for the struct itself. We can then use specific deadlines/cancels by creating a context specific for that call, like we do with the integration tests.
There was a problem hiding this comment.
I like sticking with the simple cancel chan tbh.
Do you mean quit chans or cancel() funcs?
I think having a context.Background makes sense for the struct itself. We can then use specific deadlines/cancels by creating a context specific for that call, like we do with the integration tests.
Agreed, though it's not necessary to create a cancellable context for each call when our only need is to cancel them all simultaneously. If in the future we want to be able to cancel/timeout individual calls, then creating a subcontext would be appropriate. In the meantime it is extra goroutine overhead
atm i'm kind of in favor of #2716 for simplicity, it matches the existing patterns w/in the codebase, and accomplishes the same task w/ less overhead
|
Closing since #2716 was merged |
Alternative to #2716