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,178 @@
  */
 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;
-
-    private final byte[] _opaque;
-
-    public Opaque(byte[] opaque) {
-        _opaque = opaque;
+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.
+     */
+    static Opaque forBytes(byte[] bytes) {
+        return new OpaqueImpl(bytes.clone());
     }
 
-    public byte[] getOpaque() {
-        return _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.
+     */
+    static Opaque forBytes(ByteBuffer buf, int length) {
+        byte[] bytes = new byte[length];
+        buf.get(bytes);
+
+        return new OpaqueImpl(bytes);
     }
 
-    @Override
-    public int hashCode() {
-        return Arrays.hashCode(_opaque);
+    /**
+     * Default implementation for {@link #hashCode()}.
+     * 
+     * @param obj The instance object.
+     * @return The hash code.
+     * @see #hashCode()
+     */
+    static int defaultHashCode(Opaque obj) {
+        return Arrays.hashCode(obj.toBytes());
     }
 
-    @Override
-    public boolean equals(Object o) {
-        if (o == this) {
+    /**
+     * Default implementation for {@link #equals(Object)}.
+     * 
+     * @param obj The instance object.
+     * @param other The other object.
+     * @return {@code true} if equal.
+     * @see #equals(Object)
+     */
+    static boolean defaultEquals(Opaque obj, Object other) {
+        if (other == obj) {
             return true;
         }
-        if (!(o instanceof Opaque)) {
+        if (!(other instanceof Opaque)) {
             return false;
         }
+        return Arrays.equals(obj.toBytes(), ((Opaque) other).toBytes());
+    }
 
-        return Arrays.equals(_opaque, ((Opaque) o)._opaque);
+    /**
+     * Returns a byte-representation of this opaque object.
+     * 
+     * @return A new array.
+     */
+    byte[] toBytes();
+
+    /**
+     * Returns the number of bytes in this opaque object;
+     * 
+     * @return The number of bytes;
+     */
+    int numBytes();
+
+    /**
+     * Returns a Base64 string representing this opaque object.
+     * 
+     * @return A Base64 string.
+     */
+    String toBase64();
+
+    /**
+     * Writes the bytes of this {@link Opaque} to the given {@link ByteBuffer}.
+     * 
+     * @param buf The target buffer.
+     */
+    default void putBytes(ByteBuffer buf) {
+        buf.put(toBytes());
     }
 
+    /**
+     * Returns the hashCode based on the byte-representation of this instance.
+     * <p>
+     * This method must behave like {@link #defaultHashCode(Opaque)}, but may be optimized.
+     * 
+     * @return The hashCode.
+     */
+    @Override
+    int hashCode();
+
+    /**
+     * Compares this object to another one.
+     * <p>
+     * This method must behave like {@link #defaultEquals(Opaque, Object)}, but may be optimized.
+     * 
+     * @return {@code true} if both objects are equal.
+     */
     @Override
-    public String toString() {
-        StringBuilder sb = new StringBuilder();
-        sb.append('[').append(BaseEncoding.base16().lowerCase().encode(_opaque)).append(']');
-        return sb.toString();
+    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();
+        }
+
+        @Override
+        public String toBase64() {
+            if (base64 == null) {
+                base64 = Base64.getEncoder().withoutPadding().encodeToString(_opaque);
+            }
+            return base64;
+        }
+
+        @Override
+        public void putBytes(ByteBuffer buf) {
+            buf.put(_opaque);
+        }
+
+        @Override
+        public int hashCode() {
+            return Arrays.hashCode(_opaque);
+        }
+
+        @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() + "]";
+        }
+
+        @Override
+        public int numBytes() {
+            return _opaque.length;
+        }
     }
 }

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
             }