MapMaker expiration not behaving as expected

[GoogleCodeExporter]

I checked out trunk and written some tests using the new functionality.  In 
both expire after access and expire after write, the eviction listener is not 
being hit where I expect.

Here are the two tests I have which are failing:

    @Test
    public void testMapMakerExpireAfterAccess() throws InterruptedException {
        final AtomicBoolean stopped = new AtomicBoolean(false);
        final ConcurrentMap<String, Double> map = new MapMaker().expireAfterAccess(1, TimeUnit.SECONDS).evictionListener(new MapEvictionListener<String, Double>() {
            @Override
            public void onEviction(String key, Double value) {
                System.out.println("Eviction listener hit for " + key);
                stopped.set(true);
            }
        }).makeMap();

        new Thread(new Runnable() {

            @Override
            public void run() {
                while (!stopped.get()) {
                    // make key 1 and key 2 tick
                    map.put("key1", Math.random());
                    map.put("key2", Math.random());
                    // get key2 so its acccess time is updated
                    map.get("key2");
                    try {
                        Thread.sleep(100L);
                    } catch (InterruptedException e) {
                        // no action
                    }
                }
            }
        }).start();
        Thread.sleep(2000L);
        // expect key2 to be present because of the thread accessing it, but key1 has only been written not read so it should expire
        assertEquals(1, map.size());
        assertTrue(map.containsKey("key2"));
        assertFalse(map.containsKey("key1"));
    }

    @Test
    public void testMapMakerExpiresAfterWrite() throws InterruptedException {
        final AtomicBoolean stopped = new AtomicBoolean(false);
        final ConcurrentMap<String, Double> map = new MapMaker().expireAfterWrite(1, TimeUnit.SECONDS).evictionListener(new MapEvictionListener<String, Double>() {
            @Override
            public void onEviction(String key, Double value) {
                System.out.println("Eviction listener hit for " + key);
            }
        }).makeMap();
        map.put("key1", Math.random());
        map.put("key2", Math.random());
        new Thread(new Runnable() {

            @Override
            public void run() {
                while (!stopped.get()) {
                    map.get("key2");
                    try {
                        Thread.sleep(100L);
                    } catch (InterruptedException e) {
                        // no action
                    }
                }
            }
        }).start();
        Thread.sleep(2000L);
        // nothing has been written for 2 seconds, so the get calls on the other thread should have caused the items to be evicted
        stopped.set(true);
        assertEquals(0, map.size());
    }

Original issue reported on code.google.com by keith.jo...@glebepark.com on 19 Nov 2010 at 4:20

[GoogleCodeExporter]

According to the documentation of evictionListener(): "A map built by this map 
maker will invoke the supplied listener after it evicts an entry, whether it 
does so due to timed expiration, exceeding the maximum size, or discovering 
that the key or value has been reclaimed by the garbage collector. It will 
invoke the listener synchronously, during invocations of any of that map's 
public methods (even read-only methods)."

While not 100% clear, I think the second sentence does leave the door open for 
calls to _some_ public methods not to cause expiration/calls to 
expirationListener.  Looking at the implementation of size(), it appears that 
it does not cause expiration.  This would explain your tests failing on the 
assertEqual calls.

It looks like we probably need to either (a) update the documentation to be 
more clear that expiration might not happen on some public method calls; or (b) 
update size to process expirations.

Bob, as author of this class, would you mind taking a look at this please?

Original comment by boppenh...@google.com on 20 Nov 2010 at 6:24

[GoogleCodeExporter]

I would expect the eviction listener to be invoked on the thread that is 
calling get() before the size assertion is made.

Original comment by keith.jo...@glebepark.com on 22 Nov 2010 at 8:54

[GoogleCodeExporter]

It looks like get's code deosn't cause expiration either.  I would assume that 
this was a conscious decision since expired elements are explicitly ignored in 
trying to find the right entry.  I'm leaning more towards this being a 
documentation issue, however, I am sure Bob will have some more insight.

Original comment by boppenh...@google.com on 22 Nov 2010 at 9:36

[GoogleCodeExporter]

It seems size() in general does not work correctly with evictions.  See the 
test below which I have run against trunk as of now.  I think this is more than 
a documentation issue, size() is not returning a value that is consistent with 
the state of the map.

@Test
    public void testMapMakerExpireAfterWrite() throws InterruptedException {
        final ConcurrentMap<String, String> map = new MapMaker().expireAfterWrite(1, TimeUnit.SECONDS).evictionListener(new MapEvictionListener<String, String>() {
            @Override
            public void onEviction(String key, String value) {
                System.out.println("Eviction listener hit for " + key);
            }
        }).makeMap();
        map.put("k1", "v1");
        map.put("k2", "v2");
        Thread.sleep(2000L);
        map.put("k3", "v3");
        System.out.println(map.size()); // prints 3
        System.out.println(map); // prints {k3=v3}
        for (String k : map.keySet()) {
            // loop runs once and prints k3 - v3
            System.out.println(k + " - " + map.get(k));
        }
        System.out.println(map.size()); // prints 3
        assertEquals(1, map.size()); // fails
    }

Original comment by keith.jo...@glebepark.com on 21 Dec 2010 at 5:01

[GoogleCodeExporter]

Timed expiration works rather strangely. Expired entries are not removed from 
the map, they are merely hidden from certain API methods. They are still 
referenced by the map and therefore cannot be garbage-collected. I think it's a 
bug.

Original comment by michal.b...@gmail.com on 30 Dec 2010 at 11:27

[GoogleCodeExporter]

Original comment by kevinb@google.com on 12 Jan 2011 at 11:12

• Added labels: Type-Defect

[GoogleCodeExporter]

Everything mentioned in this bug is by design. Though I'm willing to consider 
alternatives. And yes, we do need to improve the documentation.

Specifically, expiration occurs on a delayed basis, and only inside of methods 
which already lock (e.g. only during writes). This was a performance 
optimization to avoid slowing down read operations for the sake of early 
expiration.

Do you feel it would be strongly preferable to perform expiration more 
aggressively?

Note that the delayed expiration is only observable by calling the size method 
or observing the eviction listener; expired but not fully-cleaned up (and 
notified) entries will never be visible within the map.

Original comment by yrfselrahc@gmail.com on 14 Jan 2011 at 5:05

[GoogleCodeExporter]

Given what you have said, should the line map.put("k3", "v3") in comment 4 
above cause eviction?  It is a call to put not size, so I think it should.

Original comment by keith.jo...@glebepark.com on 17 Jan 2011 at 12:26

[GoogleCodeExporter]

It will cause eviction in the segment that element is in (assuming that segment 
is full).

Original comment by yrfselrahc@gmail.com on 18 Jan 2011 at 4:43

[GoogleCodeExporter]

Original comment by yrfselrahc@gmail.com on 19 Jan 2011 at 10:41

• Changed state: WontFix

[GoogleCodeExporter]

Issue 552 has been merged into this issue.

Original comment by boppenh...@google.com on 21 Feb 2011 at 10:13

[GoogleCodeExporter]

My use case in issue 552 came from what I thought would be a fairly mainstream 
use: it's a computing map, the only operation I ever need to call on the map is 
get(), if the value isn't there it creates it, and I use SoftReferences for the 
values so they'll be gc'd if necessary.  I added an EvictionListener for 
logging to see when entries where being evicted -- but it was never called.

The map construction in my actual code looks like this:

    private final Map<Integer, UserIndex> _map = new MapMaker()
        .concurrencyLevel(4)
        .softValues()
        .evictionListener(
            new MapEvictionListener<Integer, UserIndex>() {
                @Override
                public void onEviction(Integer userId, UserIndex userIndex) {
                    logger.info("Evicted index for {}, {} users indexed",
                        userId, _map.size());
                }
            })
        .makeComputingMap(
            new Function<Integer, UserIndex>() {
                @Override
                public UserIndex apply(Integer userId) {
                    logger.info("Added index for {}, {} users indexed",
                        userId, _map.size());
                    return new UserIndex(userId);
                }
            });

So you're telling me that in order for my EvictionListener to be called, I have 
to gratuitously call some method other than get(), more specifically a method 
that does a write, even though get() already does some kind of write if the 
value doesn't exist?

The first thing that comes to mind is something like this:

    UserIndex index = _map.get(userId);
    _map.put(userId, index);  // Try to get the EvictionListener to fire.

which seems stupid.  Is there a better, recommended way to get my 
EvictionListener called in my use case?

Original comment by tom%tomm...@gtempaccount.com on 22 Feb 2011 at 4:47

[GoogleCodeExporter]

See my comment in issue 552.

Original comment by fry@google.com on 24 Feb 2011 at 2:27

[GoogleCodeExporter]

Thank you!

Original comment by tom%tomm...@gtempaccount.com on 24 Feb 2011 at 7:06

[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:15

• Added labels: MigratedToGitHub

[GoogleCodeExporter]

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

• Added labels: Restrict-AddIssueComment-Commit