nfs4: update state handler to use java.time.Clock

Motivation:
The use of System.currentMillis() make it hard to implement time based
tests. Thus use of java.time.Clock provides a more flexible time source
that can instrumented during testing, if needed.

Modification:
Update NFSv4StateHandler and NFS4Client to use java.time.Clock. Update
Cache and CacheElement to use Duration.

Result:
non-functional API change that provide self documenting API with better
testing capabilities.

Acked-by: Lea Morschel
Acked-by: Paul Millar
Target: master

Author: kofemann

API-changes.md

@@ -1,5 +1,10 @@
 # Changes to NFS4J public API
 
+## 0.24
+
+- update org.dcache.nfs.util.Cache and org.dcache.nfs.v4.NFSv4StateHandler to use java.time.Duration instead of a _long in millis_ to describe various amounts of time.
+- update org.dcache.nfs.v4.{NFSv4StateHandler,NFS4Client} to use java.time.Clock as a time source
+
 ## 0.23
 
 - dropped Stat#get/setFileId methods

benchmarks/src/main/java/org/dcache/nfs/benchmarks/CacheBenchmark.java

@@ -1,5 +1,7 @@
 package org.dcache.nfs.benchmarks;
 
+import java.time.Duration;
+import java.time.Instant;
 import org.dcache.nfs.util.Cache;
 import org.dcache.nfs.util.NopCacheEventListener;
 import org.openjdk.jmh.annotations.Benchmark;
@@ -28,8 +30,8 @@ public static class CacheHolder {
 
         @Setup
         public void setUp() {
-            cache = new Cache<>("test cache", 64, Integer.MAX_VALUE,
-                    Integer.MAX_VALUE,
+            cache = new Cache<>("test cache", 64, Duration.ofSeconds(Long.MAX_VALUE),
+                    Duration.ofSeconds(Long.MAX_VALUE),
                     new NopCacheEventListener());
             cache.put("foo", "bar");
         }
@@ -42,7 +44,7 @@ public Cache<String, String> getCache() {
     @Benchmark
     @Threads(16)
     @Warmup(iterations = 5, time = 100, timeUnit = TimeUnit.MILLISECONDS)
-    public long cachePutRemoveBenchmark(CacheHolder cacheHolder) {
+    public Instant cachePutRemoveBenchmark(CacheHolder cacheHolder) {
 
         final var cache = cacheHolder.getCache();
         var key = "key";

core/src/main/java/org/dcache/nfs/util/Cache.java

@@ -20,13 +20,15 @@
 package org.dcache.nfs.util;
 
 import java.time.Clock;
+import java.time.Duration;
+import java.time.Instant;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.MissingResourceException;
-import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.concurrent.locks.StampedLock;
 
 import org.slf4j.Logger;
@@ -40,8 +42,8 @@
  *
  * Typical usage is:
  * <pre>
- *     Cache&lt;String, String&gt; cache  = new Cache&lt;&gt;("test cache", 10, TimeUnit.HOURS.toMillis(1),
- *           TimeUnit.MINUTES.toMillis(5));
+ *     Cache&lt;String, String&gt; cache  = new Cache&lt;&gt;("test cache", 10, Duration.ofHours(1),
+ *           Duration.ofMinutes(2);
  *
  *     cache.put("key", "value");
  *     String value = cache.get("key");
@@ -65,17 +67,16 @@ public class Cache<K, V> {
     private final String _name;
 
     /**
-     * Maximum allowed time, in milliseconds, that an object is allowed to be cached.
+     * Maximum amount of time that an object is allowed to be cached.
      * After expiration of this time cache entry invalidated.
      */
-
-    private final long _defaultEntryMaxLifeTime;
+    private final Duration _defaultEntryMaxLifeTime;
 
     /**
-     * Time in milliseconds since last use of the object. After expiration of this
+     * Time amount since last use of the object. After expiration of this
      * time cache entry is invalidated.
      */
-    private final long _defaultEntryIdleTime;
+    private final Duration _defaultEntryIdleTime;
 
     /**
      * Maximum number of entries in cache.
@@ -105,18 +106,18 @@ public class Cache<K, V> {
     /**
      * Last cleanup time
      */
-    private final AtomicLong _lastClean = new AtomicLong(System.currentTimeMillis());
+    private final AtomicReference<Instant> _lastClean;
 
     /**
      * Create new cache instance with default {@link CacheEventListener} and
      * default cleanup period.
      *
      * @param name Unique id for this cache.
      * @param size maximal number of elements.
-     * @param entryLifeTime maximal time in milliseconds.
-     * @param entryIdleTime maximal idle time in milliseconds.
+     * @param entryLifeTime maximal time that an entry allowed to stay in the cache after creation.
+     * @param entryIdleTime maximal time that an entry allowed to stay in the cache after last access.
      */
-    public Cache(String name, int size, long entryLifeTime, long entryIdleTime) {
+    public Cache(String name, int size, Duration entryLifeTime, Duration entryIdleTime) {
         this(name, size, entryLifeTime, entryIdleTime,
                 new NopCacheEventListener<K, V>());
     }
@@ -126,11 +127,11 @@ public Cache(String name, int size, long entryLifeTime, long entryIdleTime) {
      *
      * @param name Unique id for this cache.
      * @param size maximal number of elements.
-     * @param entryLifeTime maximal time in milliseconds.
-     * @param entryIdleTime maximal idle time in milliseconds.
+     * @param entryLifeTime maximal time that an entry allowed to stay in the cache after creation.
+     * @param entryIdleTime maximal time that an entry allowed to stay in the cache after last access.
      * @param eventListener {@link CacheEventListener}
      */
-    public Cache(final String name, int size, long entryLifeTime, long entryIdleTime,
+    public Cache(final String name, int size, Duration entryLifeTime, Duration entryIdleTime,
             CacheEventListener<K, V> eventListener) {
         this(name, size, entryLifeTime, entryIdleTime, eventListener, Clock.systemDefaultZone());
     }
@@ -140,16 +141,16 @@ public Cache(final String name, int size, long entryLifeTime, long entryIdleTime
      *
      * @param name Unique id for this cache.
      * @param size maximal number of elements.
-     * @param entryLifeTime maximal time in milliseconds.
-     * @param entryIdleTime maximal idle time in milliseconds.
+     * @param entryLifeTime maximal time that an entry allowed to stay in the cache after creation.
+     * @param entryIdleTime maximal time that an entry allowed to stay in the cache after last access.
      * @param eventListener {@link CacheEventListener}
      * @param clock {@link Clock} to use
      * <code>timeValue</code> parameter.
      */
-    public Cache(final String name, int size, long entryLifeTime, long entryIdleTime,
+    public Cache(final String name, int size, Duration entryLifeTime, Duration entryIdleTime,
             CacheEventListener<K, V> eventListener, Clock clock) {
 
-        checkArgument(entryLifeTime >= entryIdleTime, "Entry life time cant be smaller that idle time");
+        checkArgument(entryLifeTime.compareTo(entryIdleTime) >= 0, "Entry life time cant be smaller that idle time");
 
         _name = name;
         _size = size;
@@ -159,6 +160,7 @@ public Cache(final String name, int size, long entryLifeTime, long entryIdleTime
         _eventListener = eventListener;
         _mxBean = new CacheMXBeanImpl<>(this);
         _timeSource = clock;
+        _lastClean = new AtomicReference<>(_timeSource.instant());
     }
 
     /**
@@ -186,12 +188,12 @@ public void put(K k, V v) {
      *
      * @param k key associated with the value.
      * @param v value associated with key.
-     * @param entryMaxLifeTime maximal life time in milliseconds.
-     * @param entryIdleTime maximal idle time in milliseconds.
+     * @param entryMaxLifeTime maximal time that an entry allowed to stay in the cache after creation.
+     * @param entryIdleTime maximal time that an entry allowed to stay in the cache after last access.
      *
      * @throws MissingResourceException if Cache limit is reached.
      */
-    public void put(K k, V v, long entryMaxLifeTime, long entryIdleTime) {
+    public void put(K k, V v, Duration entryMaxLifeTime, Duration entryIdleTime) {
         _log.debug("Adding new cache entry: key = [{}], value = [{}]", k, v);
 
         long stamp = _accessLock.writeLock();
@@ -229,8 +231,7 @@ public V get(K k) {
                 return null;
             }
 
-            long now = _timeSource.millis();
-            valid = element.validAt(now);
+            valid = element.validAt(_timeSource.instant());
             v = element.getObject();
 
             if ( !valid ) {
@@ -278,7 +279,7 @@ public V remove(K k) {
         try {
             CacheElement<V> element = _storage.remove(k);
             if( element == null ) return null;
-            valid = element.validAt(_timeSource.millis());
+            valid = element.validAt(_timeSource.instant());
             v = element.getObject();
         } finally {
             _accessLock.unlock(stamp);
@@ -310,18 +311,18 @@ int size() {
     /**
      * Get maximal idle time until entry become unavailable.
      *
-     * @return time in milliseconds.
+     * @return default amount of an entry's maximal idle time.
      */
-    public long getEntryIdleTime() {
+    public Duration getEntryIdleTime() {
         return _defaultEntryIdleTime;
     }
 
     /**
      * Get maximal total time until entry become unavailable.
      *
-     * @return time in milliseconds.
+     * @return default amount of an entry's live time.
      */
-    public long getEntryLiveTime() {
+    public Duration getEntryLiveTime() {
         return _defaultEntryMaxLifeTime;
     }
 
@@ -348,7 +349,7 @@ public void cleanUp() {
 
         long stamp = _accessLock.writeLock();
         try {
-            long now = _timeSource.millis();
+            Instant now = _timeSource.instant();
             Iterator<Map.Entry<K, CacheElement<V>>> entries = _storage.entrySet().iterator();
             while (entries.hasNext()) {
                 Map.Entry<K, CacheElement<V>> entry = entries.next();
@@ -386,7 +387,7 @@ public List<CacheElement<V>> entries() {
         return entries;
     }
 
-    public long lastClean() {
+    public Instant lastClean() {
         return _lastClean.get();
     }
 }

core/src/main/java/org/dcache/nfs/util/CacheElement.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009 - 2020 Deutsches Elektronen-Synchroton,
+ * Copyright (c) 2009 - 2022 Deutsches Elektronen-Synchroton,
  * Member of the Helmholtz Association, (DESY), HAMBURG, GERMANY
  *
  * This library is free software; you can redistribute it and/or modify
@@ -20,7 +20,8 @@
 package org.dcache.nfs.util;
 
 import java.time.Clock;
-import java.util.Date;
+import java.time.Duration;
+import java.time.Instant;
 
 /**
  * CacheElement wrapper.
@@ -31,33 +32,33 @@
 public class CacheElement<V> {
 
     /**
-     * Maximum allowed time, in milliseconds, that an object is allowed to be cached.
+     * Maximum amount of time that the cache entry is allowed to be cached.
      * After expiration of this time cache entry invalidated.
      */
-    private final long _maxLifeTime;
+    private final Duration _maxLifeTime;
     /**
-     * Time in milliseconds since last use of the object. After expiration of this
+     * Maximum amount of time that the cache entry is allowed to be cached since last use. After expiration of this
      * time cache entry is invalidated.
      */
-    private final long _idleTime;
+    private final Duration _idleTime;
     /**
      * Element creation time.
      */
-    private final long _creationTime;
+    private final Instant _creationTime;
     /**
      * Elements last access time.
      */
-    private long _lastAccessTime;
+    private Instant _lastAccessTime;
     /**
      * internal object.
      */
     private final V _inner;
 
     private final Clock _clock;
 
-    CacheElement(V inner, Clock clock, long maxLifeTime, long idleTime) {
+    CacheElement(V inner, Clock clock, Duration maxLifeTime, Duration idleTime) {
         _clock = clock;
-        _creationTime = _clock.millis();
+        _creationTime = _clock.instant();
         _lastAccessTime = _creationTime;
         _inner = inner;
         _maxLifeTime = maxLifeTime;
@@ -71,7 +72,7 @@ public class CacheElement<V> {
      * @return internal object.
      */
     public V getObject() {
-        _lastAccessTime = _clock.millis();
+        _lastAccessTime = _clock.instant();
         return _inner;
     }
 
@@ -86,21 +87,21 @@ public V peekObject() {
     }
 
     /**
-     * Check the entry's validity at the specified time.
-     *
-     * @param time in milliseconds since 1 of January 1970.
+     * Check the entry's validity at the specified point in time.
      *
+     * @param instant point in time at which entry validity is checked.
      * @return true if entry still valid and false otherwise.
      */
-    public boolean validAt(long time) {
-        return time - _lastAccessTime < _idleTime && time - _creationTime < _maxLifeTime;
+    public boolean validAt(Instant instant) {
+        return Duration.between(_lastAccessTime, instant).compareTo(_idleTime) <= 0 &&
+              Duration.between(_creationTime, instant).compareTo(_maxLifeTime) <= 0;
     }
 
     @Override
     public String toString() {
-        long now = _clock.millis();
-        return String.format("Element: [%s], created: %s, last access: %s, life time %d, idle: %d, max idle: %d",
-            _inner.toString(), new Date( _creationTime), new Date(_lastAccessTime),
-            _maxLifeTime, now - _lastAccessTime, _idleTime);
+        Instant now = _clock.instant();
+        return String.format("Element: [%s], created: %s, last access: %s, life time %s, idle: %s, max idle: %s",
+            _inner.toString(), _creationTime, _lastAccessTime,
+            _maxLifeTime, Duration.between(_lastAccessTime, now), _idleTime);
     }
 }

core/src/main/java/org/dcache/nfs/util/CacheMXBeanImpl.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009 - 2020 Deutsches Elektronen-Synchroton,
+ * Copyright (c) 2009 - 2022 Deutsches Elektronen-Synchroton,
  * Member of the Helmholtz Association, (DESY), HAMBURG, GERMANY
  *
  * This library is free software; you can redistribute it and/or modify
@@ -19,6 +19,8 @@
  */
 package org.dcache.nfs.util;
 import java.lang.management.ManagementFactory;
+import java.time.Duration;
+import java.time.Instant;
 import java.util.List;
 import javax.management.*;
 import org.slf4j.Logger;
@@ -75,17 +77,17 @@ public int getSize() {
 
     @Override
     public long getEntryIdleTime() {
-        return _cache.getEntryIdleTime();
+        return _cache.getEntryIdleTime().toMillis();
     }
 
     @Override
     public long getEntryLiveTime() {
-        return _cache.getEntryLiveTime();
+        return _cache.getEntryLiveTime().toMillis();
     }
 
     @Override
     public long getLastClean() {
-        return System.currentTimeMillis() - _cache.lastClean();
+        return Duration.between(Instant.now(), _cache.lastClean()).toMillis();
     }
 }
 

core/src/main/java/org/dcache/nfs/v4/DefaultClientCache.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2020 iterate GmbH
+ * Copyright (c) 2020 - 2022 iterate GmbH
  *
  * This library is free software; you can redistribute it and/or modify
  * it under the terms of the GNU Library General Public License as
@@ -18,18 +18,19 @@
  */
 package org.dcache.nfs.v4;
 
+import java.time.Duration;
+import java.time.Instant;
 import org.dcache.nfs.util.Cache;
 import org.dcache.nfs.util.CacheElement;
 import org.dcache.nfs.util.CacheEventListener;
 import org.dcache.nfs.v4.xdr.clientid4;
 
-import java.util.concurrent.TimeUnit;
 import java.util.stream.Stream;
 
 public class DefaultClientCache extends Cache<clientid4, NFS4Client> implements ClientCache {
-    public DefaultClientCache(int leaseTime, CacheEventListener<clientid4, NFS4Client> eventListener) {
-        super("NFSv41 clients", 5000, Long.MAX_VALUE,
-                TimeUnit.SECONDS.toMillis(leaseTime * 2),
+    public DefaultClientCache(Duration leaseTime, CacheEventListener<clientid4, NFS4Client> eventListener) {
+        super("NFSv41 clients", 5000, Duration.ofSeconds(Long.MAX_VALUE),
+                leaseTime.multipliedBy(2),
                 eventListener);
     }