-
Notifications
You must be signed in to change notification settings - Fork 2.2k
discovery+autopilot: revert passing contexts to Start
methods
#9704
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
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 (
|
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 think we wanna rely on the cancel/done signal from the ctx
but not quit
?
The purpose of adding the ctx
to some of the public methods is to allow the caller to abort the call. Currently, each call only listens to the quit
signal to abort, and we now add a ctx
, it can either be canceled externally or internally. And when it's internal, it's functioning the same as calling close(quit)
, which makes the quit
channel redundant.
Also asked gemini and it's interesting that it suggests us to make this usage an exception - that we should put the ctx
on the struct albeit it being anti-pattern.
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! ✅
7438f22
into
lightningnetwork:elle-graph
Our pattern of having
Start/Stop
for each subsystem very much depends onStop
being responsible for proper clean-up of the subsystem - if we pass in a parent context toStart
and it gets cancelled -Stop
might not get to properly clean itself up.ctx
threading is about making synchronous calls "stoppable". So it should only be passed to aStart
method ifStart
itself is making synchronous calls before returning. IfStart
spins off asynchronous goroutines, those should be separate from the context &Stop
should be responsible for cleaning those up.So im reverting two small changes from previous PR here. Luckily, if we follow the above approach, the scope for the remainder of the "context" thread PRs gets reduced significantly .