-
Notifications
You must be signed in to change notification settings - Fork 226
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
Bootstrap immediately after calls to BootstrapWithConfig #170
Conversation
dht_bootstrap.go
Outdated
@@ -77,7 +77,24 @@ func (dht *IpfsDHT) BootstrapWithConfig(cfg BootstrapConfig) (goprocess.Process, | |||
return nil, fmt.Errorf("invalid number of queries: %d", cfg.Queries) | |||
} | |||
|
|||
proc := periodicproc.Tick(cfg.Period, dht.bootstrapWorker(cfg)) | |||
tickch := make(chan struct{}, 1) | |||
proc := periodicproc.OnSignal(tickch, dht.bootstrapWorker(cfg)) |
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.
So, OnSignal is useful when you already have a channel but, in this case, I'd just fire off your own goroutine:
dht.Process().Go(func(p goprocess.Process) {
for {
select {
...
case <-p.Closing():
return
}
}
})
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.
ah, totally!
@Stebalien that change is up! |
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 need to stop forgetting about half-written reviews...
It should be possible to just write something like:
for {
select {
case <-time.After(cfg.Period):
<-p.Go(dht.bootstrapWorker(cfg)).Closed()
case <-p.Closing():
return
}
}
goprocess will tell the worker when to quit. Does that sound right?
@Stebalien so the details behind the slightly more complex logic are: the implementation of goprocess ticker functions is such that the process will not be executed again if the previous instantiation of it has not terminated, effectively skipping one of our intervals. i sought to keep that behavior consistent, though if it's not particularly necessary for us i'm happy to remove it! in c++ i'd probably just handle this with an |
Hm. That's kind of an odd way to do it... The code above will just avoid starting the timer until the previous operation has finished (i.e., it will cause the system to sleep |
I'd just go with the simple code. I'm not aware of any good reason we do what we do with ticker. |
done! |
dht_bootstrap.go
Outdated
proc := periodicproc.Tick(cfg.Period, dht.bootstrapWorker(cfg)) | ||
proc := dht.Process().Go(func(p goprocess.Process) { | ||
for { | ||
select { |
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.
This'll need a <-p.Go(dht.bootstrapWorker(cfg)).Closed()
at the top (to actually fix the bug 😄). Sorry about that.
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.
hahahaha wasn't even thinking about that. yup.
smh hahaha. done |
closes #163. need to test locally just to be sure.