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

Cache should not expire entites when Duration value is 0 (zero) #8148

Closed
alparslanavci opened this issue May 11, 2016 · 7 comments

Comments

Projects
None yet
3 participants
@alparslanavci
Copy link

commented May 11, 2016

According to the JCache API javadoc, when the durationAmount parameter in javax.cache.expiry.Duration constructor is set to 0 (zero), it means cache entites should live eternal. Here is the javadoc:

  /**
   * Constructs a duration.
   *
   * @param timeUnit       the unit of time to specify time in. The minimum time unit is milliseconds.
   * @param durationAmount how long, in the specified units, the cache entries should live. 0 means eternal.
   * @throws NullPointerException     if timeUnit is null
   * @throws IllegalArgumentException if durationAmount is less than 0 or a TimeUnit less than milliseconds is specified
   */
  public Duration(TimeUnit timeUnit, long durationAmount) {

However the entity directly expires when it is set to zero. See the test below. It fails when executed:

    @Test
    public void valueShouldNotBeExpiredWhenDurationIsZero() {
        final int CREATED_EXPIRY_TIME_IN_MSEC = 0;

        Duration duration = new Duration(TimeUnit.MILLISECONDS, CREATED_EXPIRY_TIME_IN_MSEC);
        CacheConfig<Integer, String> cacheConfig = new CacheConfig<Integer, String>();
        cacheConfig.setExpiryPolicyFactory(CreatedExpiryPolicy.factoryOf(duration));

        final Cache<Integer, String> cache = createCache(cacheConfig);
        cache.put(1, "value");

        assertTrueEventually(new AssertTask() {
            @Override
            public void run() throws Exception {
                assertNotNull(cache.get(1));
            }
        });
    }

@alparslanavci alparslanavci changed the title Cache should not expire values when Duration value is 0 (zero) Cache should not expire entites when Duration value is 0 (zero) May 11, 2016

@serkan-ozal serkan-ozal self-assigned this May 11, 2016

@serkan-ozal serkan-ozal added this to the 3.7 milestone May 11, 2016

@serkan-ozal

This comment has been minimized.

Copy link
Contributor

commented May 20, 2016

I have looked at the implementation of javax.cache.expiry.Duration and noticed the expiration time can be interpreted as eternal if and only if time unit is null and duration time is 0. So if duration time is 0 but time unit is not null (for example MILLISECONDS), it is not accepted as eternal.

Regarding to the JCache TCK tests, Duration ZERO = new Duration(SECONDS, 0) means expiry immediately (not eternal because in here time unit is not null but SECONDS).

So in fact there was no issue in our implementation. But yes, behaviour of duration time when it is 0 is not obviously clear from javadoc of jcache.

@jerrinot

This comment has been minimized.

Copy link
Contributor

commented May 20, 2016

@serkan-ozal: this is a good realization! Should we raise a ticket in the JSR-107 project?

@serkan-ozal

This comment has been minimized.

Copy link
Contributor

commented May 20, 2016

@jerrinot To me javadoc is not clear about 0 duration time. But maybe it is clear for others :)

@jerrinot

This comment has been minimized.

Copy link
Contributor

commented May 20, 2016

I had another look and I'm interpreting it differently now:

There is indeed a null check in the Duration class: https://github.com/jsr107/jsr107spec/blob/916b3df8f45e5341748e9355ef7254bfccfb1b13/src/main/java/javax/cache/expiry/Duration.java#L108

I believe the check is there only because they do not want to throw NPE in the case the durationAmount is set to 0. Because timeUnit is irrelevant in this case anyway.

JavaDoc says: "How long, in the specified units, the cache entries should live. 0 means eternal." I read it as: "If duration is set to 0 then it's eternal. Full Stop." timeUnit is not relevant in this case.

However there is nothing indicating: "It's eternal if and only if timeUnit is set to null"
I skimmed the actual spec (not just JavaDoc), but I could not find any details there.

The bottom line is: I agree with @alparslanavci that our behaviour is buggy and we should interpret 0 as eternal duration. Even when timeUnit is non-null.

EDIT:
I had yet another look. There is also this check: https://github.com/jsr107/jsr107spec/blob/916b3df8f45e5341748e9355ef7254bfccfb1b13/src/main/java/javax/cache/expiry/Duration.java#L180 This suggests @serkan-ozal is right.

@serkan-ozal

This comment has been minimized.

Copy link
Contributor

commented May 20, 2016

@jerrinot See the Duration.ZERO. It is used by TCK tests and it is not interpreted as eternal

@jerrinot

This comment has been minimized.

Copy link
Contributor

commented May 20, 2016

@jerrinot

This comment has been minimized.

Copy link
Contributor

commented May 20, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.