discovery: thread context through in preparation for passing to graph DB #9680
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. 🏷️ Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
(soz for pre-mature review request . will re-request once CI is green ) |
In preparation for starting to thread a single parent context through LND, we update the main `server.Start` method to take a context so that it can later pass it to any subsytem's Start method it calls. We also pass the context to `newServer` since it makes some calls that will eventually reach the DB (for example the graph db).
Pass the parent LND context to the gossiper, let it derive a child context that gets cancelled on Stop. Pass the context through to any methods that will eventually thread it through to any graph DB calls. One `context.TODO()` is added here - this will be removed in the next commit. NOTE: for any internal methods that the context gets passed to, if those methods already listen on the gossiper's `quit` channel, then then don't need to also listen on the passed context's Done() channel because the quit channel is closed at the same time that the context is cancelled.
And remove a context.TODO() that was added in the previous commit.
| @@ -555,6 +553,10 @@ type AuthenticatedGossiper struct { | |||
| vb *ValidationBarrier | |||
|
|
|||
| sync.Mutex | |||
|
|
|||
| cancel fn.Option[context.CancelFunc] | |||
There was a problem hiding this comment.
Q: I don't think we want to allow this to be optional?
There was a problem hiding this comment.
the reason we need to make it optional is cause it is something we set at Start time when we derive it from a parent context. Only things that are set in the constructor can we for sure know are set at Stop time. It needs to be safe to call Stop even if Start has not run.
There was a problem hiding this comment.
It needs to be safe to call Stop even if Start has not run.
Why is that? If it's not started why do we even call Stop?
There was a problem hiding this comment.
because of the changes made in this PR from last year: 653e2f3
PR context
This is part of the last step required to complete #9494. We want all calls to the graph DB (and hence, ChannelGraph) to take a context since later, this context will be passed through to any remote graph RPC calls as well as any DB calls to a SQL graph backend.
This PR specifically:
We start in the
discoverypackage since the gossiper and the syncers make calls to the graph. So we prepare accordingly by threading LND's parent context to the gossiper and threading it through to where we will use it later.Branch strategy:
This series of PRs is basically a big code refactor. So once 19 is shipped, we can merge any work that has been merged into
elle-graphand from then on we can just merge into master.