Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LocalCache weight eviction does not work when maxSegmentWeight is >= int.MAX_VALUE #1761

Closed
gissuebot opened this issue Oct 31, 2014 · 4 comments
Assignees
Labels
Milestone

Comments

@gissuebot
Copy link

Original issue created by DanGLow on 2014-05-22 at 05:18 PM


Hi,

This bug was originally discovered here: elastic/elasticsearch#6268, but I have traced it down into an issue with an overflowing int and long comparison in LocalCache.java eviction logic. The following is the eviction code at https://github.com/google/guava/blob/master/guava/src/com/google/common/cache/LocalCache.java#L2665

@GuardedBy("Segment.this")
void evictEntries() {
  if (!map.evictsBySize()) {
    return;
  }

  drainRecencyQueue();
  while (totalWeight > maxSegmentWeight) {
    ReferenceEntry<K, V> e = getNextEvictable();
    if (!removeEntry(e, e.getHash(), RemovalCause.SIZE)) {
      throw new AssertionError();
    }
  }
}

'totalWeight' is defined as an int, while 'maxSegmentWeight' is a long. If 'maxSegmentWeight' is set to any value int.MAX_VALUE or greater, then this check

  while (totalWeight > maxSegmentWeight) {

will never be true, since 'totalWeight' will overflow, and the eviction code will never run.

@gissuebot
Copy link
Author

Original comment posted by kak@google.com on 2014-05-22 at 06:25 PM


Wow, good catch...seems like it should be relatively straight forward to change totalWeight to a long, right?


Status: Accepted
Owner: lowasser@google.com
Labels: Type-Defect, Package-Cache, Milestone-Release18
CC: fry@google.com

@gissuebot
Copy link
Author

gissuebot commented Nov 1, 2014

Original comment posted by adrien.g...@elasticsearch.com on 2014-05-27 at 10:18 AM


This looks like the appropriate fix to me indeed!

@gissuebot
Copy link
Author

Original comment posted by lowasser@google.com on 2014-05-27 at 12:05 PM


Fix submitted internally; should be mirrored out shortly.

@gissuebot
Copy link
Author

Original comment posted by lowasser@google.com on 2014-05-27 at 12:05 PM


(No comment entered for this change.)


Status: Fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants