feat: refresh and wait #418
Conversation
We'd like to be able to refresh then _wait_ for the refresh to finish in the testground DHT tests. That way, we can: 1. Start and disable _auto_ refresh. 2. Bootstrap. 3. Refresh a couple of times till we're stable. 4. Wait to _stop_ refreshing. 5. Disconnect from and forget about all peers _not_ in our routing tables. 6. Run the actual tests without interference from the bootstrapping logic.
Stebalien
force-pushed
the
feat/refresh-and-wait
branch
from
04fd9ef
to
0be0cbc
Compare
| collectWaiting: | ||
| for { | ||
| select { | ||
| case res := <-dht.triggerRtRefresh: |
There was a problem hiding this comment.
Is this second select here to handle the case where multiple callers might ask for a refresh "simultaneously" ?
There was a problem hiding this comment.
Yes. I'll document that.
| @@ -70,7 +70,7 @@ type IpfsDHT struct { | |||
| autoRefresh bool | |||
| rtRefreshQueryTimeout time.Duration | |||
| rtRefreshPeriod time.Duration | |||
| triggerRtRefresh chan struct{} | |||
| triggerRtRefresh chan chan<- error | |||
There was a problem hiding this comment.
shouldn't this be just chan error?
| @@ -167,7 +167,7 @@ func makeDHT(ctx context.Context, h host.Host, dstore ds.Batching, protocols []p | |||
| routingTable: rt, | |||
| protocols: protocols, | |||
| bucketSize: bucketSize, | |||
| triggerRtRefresh: make(chan struct{}), | |||
| triggerRtRefresh: make(chan chan<- error), | |||
There was a problem hiding this comment.
This one is slightly stricter.
|
So, it turns out I don't absolutely need this unless we're also trying to test our bootstrapping logic (as I can just connect to all peers and call it a day). |
dht_bootstrap.go
Outdated
| @@ -148,7 +183,25 @@ func (dht *IpfsDHT) Bootstrap(_ context.Context) error { | |||
| // RefreshRoutingTable tells the DHT to refresh it's routing tables. | |||
| func (dht *IpfsDHT) RefreshRoutingTable() { | |||
There was a problem hiding this comment.
A simpler API would be to make RefreshRoutingTable() always return the channel making it non-buffered, and in the send to channel above, skip and close the channel if no one's receiving (select + default branch). Such an API change wouldn't be breaking.
There was a problem hiding this comment.
SGTM. However:
- It's technically breaking but only if someone defined an interface for this.
- We have to write to the channel even if nobody's listening. Otherwise, we could return before the caller starts listening.
| } | ||
| err := dht.doRefresh(ctx) | ||
| for _, w := range waiting { | ||
| w <- err |
There was a problem hiding this comment.
(well, if we're returning a channel, we should probably close it)
Stebalien
force-pushed
the
feat/refresh-and-wait
branch
from
09f6660
to
50a9858
Compare
We'd like to be able to refresh then wait for the refresh to finish in the testground DHT tests. That way, we can: