nfs: fix ABBA dead lock during dead client cleanup

Motivation:
In situation where one thread tries to clean an expired client and an other
thread checks client validity a dead lock may happen:

  T1 -> take lock on open states in FileTracker#addOpen
  T2 -> take lock on client object in NFS4Client#updateLeaseTime
  T2 -> waits for lock on open states in FileTracker#removeOpen
  T1 -> wait for lock on client objectin NFS4Client#isLeaseValid

Modification:
use volatile filed to make NFS4Client#isLeaseValid non blocking. Remove draining
of states out of error path in NFS4Client#updateLeaseTime as draining done
in dead-client cleanup phase. Add test to ensure that state disposal is
called on client disposal.

Result:
deadlock is resolved.

Fixes: #56
Acked-by: Albert Rossi
Target: master, 0.17, 0.16, 0.15

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

@@ -122,7 +122,11 @@ public class NFS4Client {
      * sessions associated with the client
      */
     private final Map<sessionid4, NFSv41Session> _sessions = new HashMap<>();
-    private long _cl_time = System.currentTimeMillis();        // time of last lease renewal
+
+    /**
+     * The timestamp in millis of the last lease renewal.
+     */
+    private volatile long _lastLeaseUpdate = System.currentTimeMillis();
 
     /**
      * Open Owners associated with client.
@@ -254,8 +258,8 @@ public synchronized void setConfirmed() {
         _isConfirmed = true;
     }
 
-    public synchronized boolean isLeaseValid() {
-        return (System.currentTimeMillis() - _cl_time) < _leaseTime;
+    public boolean isLeaseValid() {
+        return (System.currentTimeMillis() - _lastLeaseUpdate) < _leaseTime;
     }
 
     /**
@@ -264,23 +268,22 @@ public synchronized boolean isLeaseValid() {
      * @throws ExpiredException if difference between current time and last
      * lease more than max_lease_time
      */
-    public synchronized void updateLeaseTime() throws ChimeraNFSException {
+    public void updateLeaseTime() throws ChimeraNFSException {
 
         long curentTime = System.currentTimeMillis();
-        long delta = curentTime - _cl_time;
+        long delta = curentTime - _lastLeaseUpdate;
         if (delta > _leaseTime) {
-            drainStates();
             throw new ExpiredException("lease time expired: (" + delta +"): " + BaseEncoding.base16().lowerCase().encode(_ownerId) +
                     " (" + _clientId + ").");
         }
-        _cl_time = curentTime;
+        _lastLeaseUpdate = curentTime;
     }
 
     /**
      * sets client lease time with current time
      */
-    public synchronized void refreshLeaseTime() {
-        _cl_time = System.currentTimeMillis();
+    public void refreshLeaseTime() {
+        _lastLeaseUpdate = System.currentTimeMillis();
     }
 
     /**
@@ -468,7 +471,7 @@ public void detachState(NFS4State state) {
         _clientStates.remove(state.stateid());
     }
 
-    private void drainStates() {
+    private synchronized void drainStates() {
         Iterator<NFS4State> i = _clientStates.values().iterator();
         while (i.hasNext()) {
             NFS4State state = i.next();

core/src/test/java/org/dcache/nfs/v4/NFS4ClientTest.java

@@ -21,6 +21,7 @@
 
 import java.net.UnknownHostException;
 import java.nio.charset.StandardCharsets;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 import org.dcache.nfs.ChimeraNFSException;
 import org.dcache.nfs.nfsstat;
@@ -150,5 +151,16 @@ public void testCreateSessionWrongSequence() throws ChimeraNFSException {
         nfsClient.createSession(2, 0, 0, 0, 1);
     }
 
+    @Test
+    public void testClientDisposeCleansState() throws ChimeraNFSException {
+        AtomicBoolean isDisposed = new AtomicBoolean(false);
+
+        NFS4State state = nfsClient.createState(owner);
+        state.addDisposeListener(s -> isDisposed.set(true));
+
+        nfsClient.tryDispose();
+        assertTrue("client state is not disposed", isDisposed.get());
+        assertFalse("client claims to have a state after dispose", nfsClient.hasState());
+    }
 }