[WIP] - Rework how Stop works #61
Conversation
Stop no longer relies on closing a channel. Rather, it uses the control channel to send a stop message. Further, Stop is now delayed by a configurable duration to minimize the chance that concurrent operations are impacted by Stop. Using the cache after Stop executes should no longer panic. However, it can block indefinitely (because the worker is no longer reading from various channels). This seems much worse to me. To compensate, the control channel is now buffered (configurable) and control writes are now guarded with a select/default->noop. The promote call was already guarded by this. Further, all channels are now buffered.
| @@ -161,39 +174,65 @@ func (c *Cache) Delete(key string) bool { | |||
| return false | |||
| } | |||
|
|
|||
| // Clears the cache | |||
| // Clears the cache. | |||
| // This is a control command. | |||
There was a problem hiding this comment.
Might be good, in this and similar doc comments, to say something like "See Configuration.ControlBuffer" because that seems to be where "control command" is defined.
| func (c *Cache) SetMaxSize(size int64) { | ||
| c.control <- setMaxSize{size} | ||
| done := make(chan struct{}) | ||
| c.control <- setMaxSize{size: size, done: done} |
There was a problem hiding this comment.
Is it intentional that this one is still using a blocking send whereas the others have been changed to use a non-blocking send?
| close(c.promotables) | ||
| <-c.control | ||
| go func() { | ||
| time.Sleep(c.stopDelay) |
There was a problem hiding this comment.
I'm not sure what's gained by this delay. Your comment on the other issue (which would probably be good to copy into the description of this PR) said it's "to let concurrent work finish", but what would happen if the work did not finish? That is, what is the bad consequence of still having some stuff in the other channels when the worker notices the stop message in the control channel and shuts down? It will drain those channels anyway, so this doesn't seem to be about reducing the chance of leaving some values sitting around in the channels.
As far as I can tell, the worst that would happen is that it wouldn't update the LRU status of some recently updated items. And since LRU status won't be getting updated from this point on anyway, I would think it'd be adequate to say that calling Stop() implies expiration/eviction behavior is now undefined.
Maybe in place of the deleted text in the comment above, it could say something like "After Stop is called, Get and Set operations will not cause panics, but the state of the cache data is no longer guaranteed to be consistent and should not be relied on, and control operations will have no effect.
Work on improving how safe Stop (and other parts of the code) are:
#59