fix over-wait in WaitPub #53
Conversation
ghost
assigned 
|
This could use a couple of good tests but it should fix #38. |
There was a problem hiding this comment.
In general this LGTM and it does fix #38 with some nice improvements, particularly it makes much more sense to have the last-published logic concentrated inside Run and not distributed throughout different methods.
Ideally (and I'm talking out of zero knowledge of the Go concurrent model) I would prefer to centralize as much logic as possible inside a single for/select pair (maybe offloading some logic to other methods) instead of having for-select-for-select (only for the sake of making the code easier to follow).
repub.go
Outdated
| return | ||
| case toPublish = <-rp.update: | ||
| if lastPublished.Equals(toPublish) { | ||
| break publishWaitLoop |
There was a problem hiding this comment.
Why do we not reset the timer in this case? Even if it's a repeated value my interpretation of the short timer is that it tracks activity (even dull activity), like a temporal locality, if someone updated the value recently there's a good chance it will update it again in the near future.
There was a problem hiding this comment.
In this case, it'll go back to the top which will reset the timer.
| @@ -68,19 +67,23 @@ func (rp *Republisher) Close() error { | |||
| // channel. Multiple consecutive updates may extend the time period before | |||
| // the next publish occurs in order to more efficiently batch updates. | |||
| func (rp *Republisher) Update(c cid.Cid) { | |||
| // concurrent publish and we can safely let that | ||
| // concurrent publish win. | ||
| } | ||
| case rp.update <- c: | ||
| } | ||
| } | ||
|
|
||
| // Run contains the core logic of the `Republisher`. It calls the user-defined |
There was a problem hiding this comment.
I think Run's logic has changed enough to deserve an update in its documentation.
| // no more updates before it expires (restarted every time | ||
| // the `valueHasBeenUpdated` is signaled). | ||
| quick.Stop() | ||
| select { |
There was a problem hiding this comment.
nit (and may even be wrong): Stop's documentation seems to be suggesting the simpler way:
// if !t.Stop() {
// <-t.C
// }
There was a problem hiding this comment.
Unfortunately, that will deadlock if we loop without ever starting the timer. See: golang/go#18553.
Really, I just avoid that idiom so I never have to think about this. I've been bitten too many times.
repub.go
Outdated
| case <-quick: | ||
| case <-longer: | ||
| case valueHasBeenPublished = <-rp.immediatePublish: | ||
| case toPublish := <-rp.update: |
There was a problem hiding this comment.
Could you add more comments to this case please? I'm having trouble following the logic.
repub.go
Outdated
|
|
||
| case <-quick.C: | ||
| case <-longer.C: | ||
| case waiter := <-rp.immediatePublish: |
There was a problem hiding this comment.
So with this change WaitPub now does no longer trigger an immediate publish (immediate in the sense of not waiting for the timers) right? In that case could we rename the immediatePublish please?
Also, is this immediate publish feature currently expected by the users? Without clear documentation I guess it's hard to say, I'm appealing to your general knowledge of go-ipfs and IPNS in particular, the question being: is someone impacted by having to actually wait the short timeout every time it updates? (e.g., is someone doing many Updates/WaitPubs in a row to be impacted by it?)
There was a problem hiding this comment.
🙄
I just wasn't thinking...
Stebalien
force-pushed
the
fix/38
branch
2 times, most recently
from
6b60060
to
30f0d7d
Compare
Before, WaitPub could wait forever if a value was published at the same time as the call to WaitPub. This patch also avoids republishing the same value multiple times and allows setting an initial value without reaching in and modifying internal state. fixes #38
|
You were right, a single loop is much simpler. I ended up adding a second loop anyways to retry publishing (instead of just moving on) but this should be easier to understand. |
|
For some reason this PR seems to makes mfs fail to flush the root node when running commands locally (without the daemon), see ipfs/kubo#5925 (comment) |
Before, WaitPub could wait forever if a value was published at the same time as the call to WaitPub.
This patch also avoids republishing the same value multiple times and allows setting an initial value without reaching in and modifying internal state.
fixes #38