CacheBuilder#removalListener(RemovalListener) lacks information about deferred invoking RemovalListener

[GoogleCodeExporter]

[http://stackoverflow.com/questions/21986551/guava-cachebuilder-doesnt-call-remo
val-listener In this Stackoverflow question] user pointed out that 
RemovalListener isn't called on removal entry. That behavior is not explicitely 
documented, as CacheBuilder#removalListener(RemovalListener) documentation says:

> Specifies a listener instance that caches should notify each time an entry
> is removed for any reason. Each cache created by *this builder will invoke
> this listener as part of the routine maintenance described in the class
> documentation above*.

The "documentation above" says:

> Caches built with CacheBuilder do not perform cleanup and evict values
> "automatically," or instantly after a value expires, or anything of the sort.
> Instead, it performs small amounts of maintenance during write operations,
> *or during occasional read operations if writes are rare*.

I think user can expect RemovalListener to be called on actual removal.

Internally, the cleanup is (not) made by postReadCleanup():

    /**
     * Performs routine cleanup following a read. Normally cleanup happens during writes. If cleanup
     * is not observed after a sufficient number of reads, try cleaning up from the read thread.
     */
    void postReadCleanup() {
      if ((readCount.incrementAndGet() & DRAIN_THRESHOLD) == 0) {
        cleanUp();
      }
    }

so "occasionally" means every 64 reads, if no write or cleanup occured 
meanwhile. This is some kind of optimization, so I'd like to request either:

 - adding a clarification in CacheBuilder#removalListener(RemovalListener),
 - changing postReadCleanup() to just do cleanUp() (it should probably be benchmarked),
 - adding something like CacheBuilder#doNotDeferRemovalListener() which would bypass DRAIN_THRESHOLD check in postReadCleanup().

I've checked LoadingCache tests and it looks like the third option (which is a 
win-win IMO) would not break any tests, while second option breaks 
testDrainRecencyQueueOn{Read,Write}.

Original issue reported on code.google.com by xaerx...@gmail.com on 24 Feb 2014 at 2:58

[GoogleCodeExporter]

I'm not sure I follow the issue here.  Are you expecting the entry to be passed 
to the RemovalListener immediately after being removed from the table, i.e. no 
longer accessible to queries?

Or are you expressing concern over _why_ the decision was made to only cleanup 
"occasionally" on reads?

Original comment by lowas...@google.com on 24 Feb 2014 at 6:19

[GoogleCodeExporter]

After checking the example and reading source and docs again, it seems that 
it's a documented behavior and changes in code I proposed wouldn't be suitable 
for Guava's (Loading)Cache not-heavyweight design.

However, phrase "Specifies a listener instance that caches should notify *each 
time an entry is removed* for any reason." isn't 100% accurate to me, because 
notification doesn't occur *at once*, but as part of "routine described in 
docs", so whole sentence should be IMO rephrased. I don't know if you feel the 
same - if not, don't hesitate to close this issue.

Original comment by xaerx...@gmail.com on 18 Apr 2014 at 11:53

[GoogleCodeExporter]

This issue has been migrated to GitHub.

It can be found at https://github.com/google/guava/issues/<id>

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:09

• Added labels: MigratedToGitHub

[GoogleCodeExporter]

Original comment by cgdecker@google.com on 1 Nov 2014 at 4:17

• Changed state: Migrated

[GoogleCodeExporter]

Original comment by cgdecker@google.com on 3 Nov 2014 at 9:07

• Added labels: Restrict-AddIssueComment-Commit