core: Opaque: Clarify mutable/immutable Opaques, add ByteBuffer support

Currently, Opaque is merely a wrapper around byte[], which adds
hashCode/equals functionality, preventing mutations, so we can use it as
a key in a HashMap.

When we process a byte stream (from a ByteBuffer), we may want to look
up values from a HashMap. However, we would still need to create new
byte[]-backed Opaque objects for the lookup. This creates unnecessary
allocations.

Improve the Opaque interface to clarify that by itself, Opaques can be
mutable. Provide a #toImmutableOpaque() method which is used when
storing values in a Map.

Adjust FileTracker to use immutable Opaques when storing in a Map.

Provide a mutable Opaque implementation backed by a ByteBuffer, and
improve equals() for OpaqueImpl.

Add a small test case to ensure sanity.

Related: #149
Signed-off-by: Christian Kohlschütter <[email protected]>

Author: kohlschuetter

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

@@ -22,14 +22,19 @@
 import java.nio.ByteBuffer;
 import java.util.Arrays;
 import java.util.Base64;
+import java.util.Objects;
 
 /**
- * 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.
+ * Describes something that can be used as a key for {@link java.util.HashMap} and that can be converted to a
+ * {@code byte[]} and a Base64 string representation.
+ * <p>
+ * Note that {@link Opaque}s that are <em>stored</em> in {@link java.util.HashMap} need to be immutable. Call
+ * {@link #toImmutableOpaque()} when necessary (e.g., when using {@link java.util.HashMap#put(Object, Object)},
+ * {@link java.util.HashMap#computeIfAbsent(Object, java.util.function.Function)}, etc.
  */
 public interface Opaque {
     /**
-     * Returns an {@link Opaque} instance based on a copy of the given bytes.
+     * Returns an immutable {@link Opaque} instance based on a copy of the given bytes.
      * 
      * @param bytes The bytes.
      * @return The {@link Opaque} instance.
@@ -39,7 +44,8 @@ static Opaque forBytes(byte[] bytes) {
     }
 
     /**
-     * Returns an {@link Opaque} instance based on a copy of the {@code length} bytes from the given {@link ByteBuffer}.
+     * Returns an immutable {@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.
@@ -52,6 +58,23 @@ static Opaque forBytes(ByteBuffer buf, int length) {
         return new OpaqueImpl(bytes);
     }
 
+    /**
+     * Returns a <em>mutable</em> {@link Opaque} instance backed on the byte contents of the given {@link ByteBuffer},
+     * for the given number of bytes starting from the given absolute index.
+     * <p>
+     * Note that the returned {@link Opaque} is typically not suitable for <em>storing</em> in a
+     * {@link java.util.HashMap}, but merely for lookups. Call {@link #toImmutableOpaque()} when necessary.
+     * 
+     * @param buf The buffer backing the {@link Opaque}.
+     * @param index The absolute index to start from.
+     * @param length The number of bytes.
+     * @return The {@link Opaque} instance.
+     * @see #toImmutableOpaque()
+     */
+    static Opaque forMutableByteBuffer(ByteBuffer buf, int index, int length) {
+        return new OpaqueBufferImpl(buf, index, length);
+    }
+
     /**
      * Default implementation for {@link #hashCode()}.
      * 
@@ -102,6 +125,13 @@ static boolean defaultEquals(Opaque obj, Object other) {
      */
     String toBase64();
 
+    /**
+     * Returns an immutable {@link Opaque}, which may be the instance itself if it is already immutable.
+     * 
+     * @return An immutable opaque.
+     */
+    Opaque toImmutableOpaque();
+
     /**
      * Writes the bytes of this {@link Opaque} to the given {@link ByteBuffer}.
      * 
@@ -177,6 +207,19 @@ public boolean equals(Object o) {
 
             if (o instanceof OpaqueImpl) {
                 return Arrays.equals(_opaque, ((OpaqueImpl) o)._opaque);
+            } else if (o instanceof OpaqueBufferImpl) {
+                OpaqueBufferImpl other = (OpaqueBufferImpl) o;
+                if (other.numBytes() != _opaque.length) {
+                    return false;
+                }
+                ByteBuffer otherBuf = other.buf;
+                int otherIndex = other.index;
+                for (int i = 0, n = _opaque.length, oi = otherIndex; i < n; i++, oi++) {
+                    if (_opaque[i] != otherBuf.get(oi)) {
+                        return false;
+                    }
+                }
+                return true;
             } else {
                 return Arrays.equals(_opaque, ((Opaque) o).toBytes());
             }
@@ -196,5 +239,95 @@ public String toString() {
         public int numBytes() {
             return _opaque.length;
         }
+
+        @Override
+        public Opaque toImmutableOpaque() {
+            return this;
+        }
+    }
+
+    final class OpaqueBufferImpl implements Opaque {
+        private final ByteBuffer buf;
+        private final int index;
+        private final int length;
+
+        private OpaqueBufferImpl(ByteBuffer buf, int index, int length) {
+            this.buf = Objects.requireNonNull(buf);
+            this.index = index;
+            this.length = length;
+        }
+
+        @Override
+        public byte[] toBytes() {
+            byte[] bytes = new byte[length];
+            buf.get(index, bytes);
+            return bytes;
+        }
+
+        @Override
+        public int numBytes() {
+            return length;
+        }
+
+        @Override
+        public String toBase64() {
+            return Base64.getEncoder().withoutPadding().encodeToString(toBytes());
+        }
+
+        @Override
+        public Opaque toImmutableOpaque() {
+            return Opaque.forBytes(toBytes());
+        }
+
+        @Override
+        public int hashCode() {
+            int result = 1;
+            for (int i = index, n = index + length; i < n; i++) {
+                byte element = buf.get(i);
+                result = 31 * result + element;
+            }
+
+            return result;
+        }
+
+        @Override
+        public boolean equals(Object o) {
+            if (o == this) {
+                return true;
+            }
+            if (!(o instanceof Opaque)) {
+                return false;
+            }
+            if (length != ((Opaque) o).numBytes()) {
+                return false;
+            }
+
+            if (o instanceof OpaqueImpl) {
+                byte[] otherBytes = ((OpaqueImpl) o)._opaque;
+                for (int i = index, n = index + length, oi = 0; i < n; i++, oi++) {
+                    if (buf.get(i) != otherBytes[oi]) {
+                        return false;
+                    }
+                }
+                return true;
+            } else if (o instanceof OpaqueBufferImpl) {
+                OpaqueBufferImpl other = (OpaqueBufferImpl) o;
+                ByteBuffer otherBuf = other.buf;
+                int otherIndex = other.index;
+                for (int i = index, n = index + length, oi = otherIndex; i < n; i++, oi++) {
+                    if (buf.get(i) != otherBuf.get(oi)) {
+                        return false;
+                    }
+                }
+                return true;
+            } else {
+                return toImmutableOpaque().equals(o);
+            }
+        }
+
+        @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 = inode.getFileIdKey();
+        final Opaque fileId = inode.getFileIdKey().toImmutableOpaque();
         Lock lock = filesLock.get(fileId);
         lock.lock();
         try {

core/src/test/java/org/dcache/nfs/util/OpaqueTest.java

@@ -0,0 +1,40 @@
+package org.dcache.nfs.util;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+
+import java.nio.ByteBuffer;
+
+import org.junit.Test;
+
+public class OpaqueTest {
+    @Test
+    public void testMutable() {
+        ByteBuffer buf = ByteBuffer.allocate(64);
+        buf.putInt(3, 0xAABBCCDD);
+        buf.putInt(7, 0xEEFF0011);
+
+        Opaque bufOpaque = Opaque.forMutableByteBuffer(buf, 3, 4);
+        Opaque bytesOpaque = Opaque.forBytes(new byte[] {(byte) 0xAA, (byte) 0xBB, (byte) 0xCC, (byte) 0xDD});
+
+        assertEquals(bufOpaque.toBase64(), bytesOpaque.toBase64());
+        assertEquals(bufOpaque, bytesOpaque);
+        assertEquals(bytesOpaque, bufOpaque);
+        assertEquals(bytesOpaque.hashCode(), bufOpaque.hashCode());
+
+        // unrelated changes should not affect equality
+        buf.put(2, (byte) 0x7f);
+        buf.putInt(7, 0);
+        assertEquals(bufOpaque.toBase64(), bytesOpaque.toBase64());
+        assertEquals(bufOpaque, bytesOpaque);
+        assertEquals(bytesOpaque, bufOpaque);
+        assertEquals(bytesOpaque.hashCode(), bufOpaque.hashCode());
+
+        // change contents of mutable buffer
+        buf.put(6, (byte) 0x12);
+        assertNotEquals(bufOpaque.toBase64(), bytesOpaque.toBase64());
+        assertNotEquals(bufOpaque, bytesOpaque);
+        assertNotEquals(bytesOpaque, bufOpaque);
+        assertNotEquals(bytesOpaque.hashCode(), bufOpaque.hashCode());
+    }
+}