Use Opaque for locking instead of byte[], improve reuse of base64 keys

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

@@ -19,49 +19,99 @@
  */
 package org.dcache.nfs.util;
 
-import java.io.Serializable;
+import java.nio.ByteBuffer;
 import java.util.Arrays;
-
-import com.google.common.io.BaseEncoding;
+import java.util.Base64;
 
 /**
- * A helper class for opaque data manipulations. Enabled opaque date to be used as a key in {@link java.util.Collection}
+ * Describes something that can be used as a key in {@link java.util.Map} and that can be converted to a {@code byte[]}
+ * and a Base64 string representation.
  */
-public class Opaque implements Serializable {
-
-    private static final long serialVersionUID = 1532238396149112674L;
+public interface Opaque {
+    /**
+     * Returns an {@link Opaque} instance based on a copy of the given bytes.
+     * 
+     * @param bytes The bytes.
+     * @return The {@link Opaque} instance.
+     */
+    public static Opaque forBytes(byte[] bytes) {
+        return new OpaqueImpl(bytes.clone());
+    }
 
-    private final byte[] _opaque;
+    /**
+     * Returns an {@link Opaque} instance based on a copy of the {@code length} bytes from the given {@link ByteBuffer}.
+     * 
+     * @param buf The buffer.
+     * @param length The number of bytes.
+     * @return The {@link Opaque} instance.
+     */
+    public static Opaque forBytes(ByteBuffer buf, int length) {
+        byte[] bytes = new byte[length];
+        buf.get(bytes);
 
-    public Opaque(byte[] opaque) {
-        _opaque = opaque;
+        return new OpaqueImpl(bytes);
     }
 
-    public byte[] getOpaque() {
-        return _opaque;
-    }
+    byte[] toBytes();
+
+    String toBase64();
 
     @Override
-    public int hashCode() {
-        return Arrays.hashCode(_opaque);
-    }
+    int hashCode();
 
     @Override
-    public boolean equals(Object o) {
-        if (o == this) {
-            return true;
+    boolean equals(Object o);
+
+    final class OpaqueImpl implements Opaque {
+        private final byte[] _opaque;
+        private String base64 = null;
+
+        private OpaqueImpl(byte[] opaque) {
+            _opaque = opaque;
+        }
+
+        @Override
+        public byte[] toBytes() {
+            return _opaque.clone();
         }
-        if (!(o instanceof Opaque)) {
-            return false;
+
+        @Override
+        public String toBase64() {
+            if (base64 == null) {
+                base64 = Base64.getEncoder().withoutPadding().encodeToString(_opaque);
+            }
+            return base64;
         }
 
-        return Arrays.equals(_opaque, ((Opaque) o)._opaque);
-    }
+        @Override
+        public int hashCode() {
+            return Arrays.hashCode(_opaque);
+        }
 
-    @Override
-    public String toString() {
-        StringBuilder sb = new StringBuilder();
-        sb.append('[').append(BaseEncoding.base16().lowerCase().encode(_opaque)).append(']');
-        return sb.toString();
+        @Override
+        public boolean equals(Object o) {
+            if (o == this) {
+                return true;
+            }
+            if (!(o instanceof Opaque)) {
+                return false;
+            }
+
+            if (o instanceof OpaqueImpl) {
+                return Arrays.equals(_opaque, ((OpaqueImpl) o)._opaque);
+            } else {
+                return Arrays.equals(_opaque, ((Opaque) o).toBytes());
+            }
+        }
+
+        /**
+         * Returns a (potentially non-stable) debug string.
+         * 
+         * @see #toBase64()
+         */
+        @Override
+        public String toString() {
+            return super.toString() + "[" + toBase64() + "]";
+        }
     }
 }

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

@@ -242,7 +242,7 @@ public OpenRecord addOpen(NFS4Client client, StateOwner owner, Inode inode, int
         // client explicitly requested write delegation
         boolean wantWriteDelegation = (shareAccess & nfs4_prot.OPEN4_SHARE_ACCESS_WANT_WRITE_DELEG) != 0;
 
-        Opaque fileId = new Opaque(inode.getFileId());
+        Opaque fileId = inode.getFileIdKey();
         Lock lock = filesLock.get(fileId);
         lock.lock();
         try {
@@ -376,7 +376,7 @@ public OpenRecord addOpen(NFS4Client client, StateOwner owner, Inode inode, int
     public stateid4 downgradeOpen(NFS4Client client, stateid4 stateid, Inode inode, int shareAccess, int shareDeny)
             throws ChimeraNFSException {
 
-        Opaque fileId = new Opaque(inode.getFileId());
+        Opaque fileId = inode.getFileIdKey();
         Lock lock = filesLock.get(fileId);
         lock.lock();
         try {
@@ -426,7 +426,7 @@ public stateid4 downgradeOpen(NFS4Client client, stateid4 stateid, Inode inode,
     public void delegationReturn(NFS4Client client, stateid4 stateid, Inode inode)
             throws ChimeraNFSException {
 
-        Opaque fileId = new Opaque(inode.getFileId());
+        Opaque fileId = inode.getFileIdKey();
         Lock lock = filesLock.get(fileId);
         lock.lock();
         try {
@@ -464,7 +464,7 @@ public void delegationReturn(NFS4Client client, stateid4 stateid, Inode inode)
     public int getShareAccess(NFS4Client client, Inode inode, stateid4 stateid)
             throws ChimeraNFSException {
 
-        Opaque fileId = new Opaque(inode.getFileId());
+        Opaque fileId = inode.getFileIdKey();
         Lock lock = filesLock.get(fileId);
         lock.lock();
         try {
@@ -527,7 +527,7 @@ public int getShareAccess(NFS4Client client, Inode inode, stateid4 stateid)
      */
     void removeOpen(Inode inode, stateid4 stateid) {
 
-        Opaque fileId = new Opaque(inode.getFileId());
+        Opaque fileId = inode.getFileIdKey();
         Lock lock = filesLock.get(fileId);
         lock.lock();
         try {
@@ -565,7 +565,7 @@ void removeOpen(Inode inode, stateid4 stateid) {
     public Map<Inode, Collection<NFS4Client>> getOpenFiles() {
         return files.entrySet().stream()
                 .collect(Collectors.toMap(
-                        e -> Inode.forFile(e.getKey().getOpaque()),
+                        e -> Inode.forFileIdKey(e.getKey()),
                         e -> e.getValue().stream().map(OpenState::getClient).collect(Collectors.toSet())));
     }
 
@@ -578,7 +578,7 @@ public Map<Inode, Collection<NFS4Client>> getOpenFiles() {
     public Map<Inode, Collection<NFS4Client>> getDelegations() {
         return delegations.entrySet().stream()
                 .collect(Collectors.toMap(
-                        e -> Inode.forFile(e.getKey().getOpaque()),
+                        e -> Inode.forFileIdKey(e.getKey()),
                         e -> e.getValue().stream().map(DelegationState::client).collect(Collectors.toSet())));
     }
 }

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

@@ -628,7 +628,7 @@ public boolean isCallbackNeede() {
     public synchronized StateOwner getOrCreateOwner(byte[] owner, seqid4 seq) throws BadSeqidException {
         StateOwner stateOwner;
         if (_minorVersion == 0) {
-            Opaque k = new Opaque(owner);
+            Opaque k = Opaque.forBytes(owner);
             stateOwner = _owners.get(k);
             if (stateOwner == null) {
                 state_owner4 so = new state_owner4();
@@ -655,7 +655,7 @@ public synchronized StateOwner getOrCreateOwner(byte[] owner, seqid4 seq) throws
      * @param owner client unique state owner
      */
     public synchronized void releaseOwner(byte[] owner) throws StaleClientidException {
-        Opaque k = new Opaque(owner);
+        Opaque k = Opaque.forBytes(owner);
         StateOwner stateOwner = _owners.remove(k);
         if (stateOwner == null) {
             throw new StaleClientidException();

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

@@ -118,11 +118,11 @@ public void process(CompoundContext context, nfs_resop4 result) throws ChimeraNF
 
             NlmLock lock = new NlmLock(lockOwner, _args.oplock.locktype, _args.oplock.offset.value,
                     _args.oplock.length.value);
-            context.getLm().lock(inode.getFileId(), lock);
+            context.getLm().lock(inode.getLockKey(), lock);
 
             // ensure, that on close locks will be released
             lock_state.addDisposeListener(s -> {
-                context.getLm().unlockIfExists(inode.getFileId(), lock);
+                context.getLm().unlockIfExists(inode.getLockKey(), lock);
             });
 
             // FIXME: we might run into race condition, thus updating sedid must be fenced!

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

@@ -85,7 +85,7 @@ public void process(CompoundContext context, nfs_resop4 result) throws IOExcepti
 
             NlmLock lock = new NlmLock(lockOwner, _args.oplockt.locktype, _args.oplockt.offset.value,
                     _args.oplockt.length.value);
-            context.getLm().test(inode.getFileId(), lock);
+            context.getLm().test(inode.getLockKey(), lock);
 
             result.oplockt.status = nfsstat.NFS_OK;
 

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

@@ -76,7 +76,7 @@ public void process(CompoundContext context, nfs_resop4 result) throws ChimeraNF
             NlmLock lock = new NlmLock(lockOwner, _args.oplocku.locktype, _args.oplocku.offset.value,
                     _args.oplocku.length.value);
             try {
-                context.getLm().unlock(inode.getFileId(), lock);
+                context.getLm().unlock(inode.getLockKey(), lock);
             } catch (LockRangeUnavailabeException e) {
                 // posix locks allows unlocking of not locked regions
             }

core/src/main/java/org/dcache/nfs/v4/nlm/AbstractLockManager.java

@@ -26,6 +26,7 @@
 import java.util.concurrent.locks.Lock;
 import java.util.stream.Collectors;
 
+import org.dcache.nfs.util.Opaque;
 import org.dcache.nfs.v4.xdr.nfs4_prot;
 
 /**
@@ -42,23 +43,23 @@ public abstract class AbstractLockManager implements LockManager {
      * @param objId object id.
      * @return exclusive lock.
      */
-    abstract protected Lock getObjectLock(byte[] objId);
+    protected abstract Lock getObjectLock(Opaque objId);
 
     /**
      * Get collection of currently used active locks on the object.
      *
      * @param objId object id.
      * @return collection of active locks.
      */
-    abstract protected Collection<NlmLock> getActiveLocks(byte[] objId);
+    protected abstract Collection<NlmLock> getActiveLocks(Opaque objId);
 
     /**
      * Add {@code lock} to an object.
      *
      * @param objId object id.
      * @param lock lock to add.
      */
-    abstract protected void add(byte[] objId, NlmLock lock);
+    protected abstract void add(Opaque objId, NlmLock lock);
 
     /**
      * Remove a lock for the given object.
@@ -67,26 +68,26 @@ public abstract class AbstractLockManager implements LockManager {
      * @param lock lock to remove.
      * @return true, if specified lock was removed.
      */
-    abstract protected boolean remove(byte[] objId, NlmLock lock);
+    protected abstract boolean remove(Opaque objId, NlmLock lock);
 
     /**
      * Add all locks from a given collection of locks
      *
-     * @param objId
-     * @param locks
+     * @param objId object id.
+     * @param locks locks to add.
      */
-    abstract protected void addAll(byte[] objId, Collection<NlmLock> locks);
+    protected abstract void addAll(Opaque objId, Collection<NlmLock> locks);
 
     /**
      * Remove all locks specified by {@code locks} associated with the given object.
      *
      * @param objId object id.
      * @param locks collections of locks to remove.
      */
-    abstract protected void removeAll(byte[] objId, Collection<NlmLock> locks);
+    protected abstract void removeAll(Opaque objId, Collection<NlmLock> locks);
 
     @Override
-    public void lock(byte[] objId, NlmLock lock) throws LockException {
+    public void lock(Opaque objId, NlmLock lock) throws LockException {
         Lock dlmLock = getObjectLock(objId);
         dlmLock.lock();
         try {
@@ -123,7 +124,7 @@ public void lock(byte[] objId, NlmLock lock) throws LockException {
     }
 
     @Override
-    public void unlock(byte[] objId, NlmLock lock) throws LockException {
+    public void unlock(Opaque objId, NlmLock lock) throws LockException {
         Lock dlmLock = getObjectLock(objId);
         dlmLock.lock();
         try {
@@ -162,7 +163,7 @@ public void unlock(byte[] objId, NlmLock lock) throws LockException {
     }
 
     @Override
-    public void test(byte[] objId, NlmLock lock) throws LockException {
+    public void test(Opaque objId, NlmLock lock) throws LockException {
         Lock dlmLock = getObjectLock(objId);
         dlmLock.lock();
         try {
@@ -178,7 +179,7 @@ public void test(byte[] objId, NlmLock lock) throws LockException {
     }
 
     @Override
-    public void unlockIfExists(byte[] objId, NlmLock lock) {
+    public void unlockIfExists(Opaque objId, NlmLock lock) {
         Lock dlmLock = getObjectLock(objId);
         dlmLock.lock();
         try {
@@ -187,5 +188,4 @@ public void unlockIfExists(byte[] objId, NlmLock lock) {
             dlmLock.unlock();
         }
     }
-
 }

core/src/main/java/org/dcache/nfs/v4/nlm/LockManager.java

@@ -19,6 +19,8 @@
  */
 package org.dcache.nfs.v4.nlm;
 
+import org.dcache.nfs.util.Opaque;
+
 /**
  */
 public interface LockManager {
@@ -31,7 +33,7 @@ public interface LockManager {
      * @throws LockDeniedException if a conflicting lock is detected.
      * @throws LockException if locking fails.
      */
-    void lock(byte[] objId, NlmLock lock) throws LockException;
+    void lock(Opaque objId, NlmLock lock) throws LockException;
 
     /**
      * Unlock byte range of an {@code objId}.
@@ -41,7 +43,7 @@ public interface LockManager {
      * @throws LockRangeUnavailabeException if no matching lock found.
      * @throws LockException if locking fails.
      */
-    void unlock(byte[] objId, NlmLock lock) throws LockException;
+    void unlock(Opaque objId, NlmLock lock) throws LockException;
 
     /**
      * Test byte range lock existence for an {@code objId}. Same as {@link #lock}, except that a new lock is not
@@ -52,13 +54,13 @@ public interface LockManager {
      * @throws LockDeniedException if a conflicting lock is detected.
      * @throws LockException if locking fails.
      */
-    void test(byte[] objId, NlmLock lock) throws LockException;
+    void test(Opaque objId, NlmLock lock) throws LockException;
 
     /**
-     * Like {@link #unlock(byte[], org.dcache.nfs.v4.nlm.NlmLock)}, but does not fail if lock does not exists.
+     * Like {@link #unlock(Opaque, org.dcache.nfs.v4.nlm.NlmLock)}, but does not fail if lock does not exists.
      *
      * @param objId
      * @param lock
      */
-    void unlockIfExists(byte[] objId, NlmLock lock);
+    void unlockIfExists(Opaque objId, NlmLock lock);
 }