diff --git a/core/src/main/java/org/dcache/nfs/util/Opaque.java b/core/src/main/java/org/dcache/nfs/util/Opaque.java index 119d4fdd..bec1ddba 100644 --- a/core/src/main/java/org/dcache/nfs/util/Opaque.java +++ b/core/src/main/java/org/dcache/nfs/util/Opaque.java @@ -22,24 +22,43 @@ 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. + *

+ * Note that {@link Opaque}s that are stored 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. */ static Opaque forBytes(byte[] bytes) { - return new OpaqueImpl(bytes.clone()); + return new OpaqueImmutableImpl(bytes.clone()); } /** - * Returns an {@link Opaque} instance based on a copy of the {@code length} bytes from the given {@link ByteBuffer}. + * Returns an mutable {@link Opaque} instance based on the given byte array. + *

+ * Note that the returned {@link Opaque} is typically not suitable for storing in a + * {@link java.util.HashMap}, but merely for lookups. Call {@link #toImmutableOpaque()} when necessary. + * + * @param bytes The bytes. + * @return The {@link Opaque} instance. + */ + static Opaque forMutableByteArray(byte[] bytes) { + return new OpaqueImpl(bytes); + } + + /** + * 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. @@ -49,7 +68,24 @@ static Opaque forBytes(ByteBuffer buf, int length) { byte[] bytes = new byte[length]; buf.get(bytes); - return new OpaqueImpl(bytes); + return new OpaqueImmutableImpl(bytes); + } + + /** + * Returns a mutable {@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. + *

+ * Note that the returned {@link Opaque} is typically not suitable for storing 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); } /** @@ -102,6 +138,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}. * @@ -131,11 +174,10 @@ default void putBytes(ByteBuffer buf) { @Override boolean equals(Object o); - final class OpaqueImpl implements Opaque { - private final byte[] _opaque; - private String base64 = null; + class OpaqueImpl implements Opaque { + final byte[] _opaque; - private OpaqueImpl(byte[] opaque) { + OpaqueImpl(byte[] opaque) { _opaque = opaque; } @@ -145,21 +187,22 @@ public byte[] toBytes() { } @Override - public String toBase64() { - if (base64 == null) { - base64 = Base64.getEncoder().withoutPadding().encodeToString(_opaque); - } - return base64; + public int hashCode() { + return Arrays.hashCode(_opaque); } @Override - public void putBytes(ByteBuffer buf) { - buf.put(_opaque); + public String toBase64() { + return toBase64Impl(); + } + + protected String toBase64Impl() { + return Base64.getEncoder().withoutPadding().encodeToString(_opaque); } @Override - public int hashCode() { - return Arrays.hashCode(_opaque); + public void putBytes(ByteBuffer buf) { + buf.put(_opaque); } @Override @@ -173,6 +216,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()); } @@ -192,5 +248,125 @@ public String toString() { public int numBytes() { return _opaque.length; } + + @Override + public Opaque toImmutableOpaque() { + return Opaque.forBytes(_opaque); + } + } + + final class OpaqueImmutableImpl extends OpaqueImpl { + private String base64 = null; + private int hashCode; + + protected OpaqueImmutableImpl(byte[] opaque) { + super(opaque); + } + + @Override + public int hashCode() { + if (hashCode == 0) { + hashCode = Arrays.hashCode(_opaque); + } + return hashCode; + } + + @Override + public String toBase64() { + if (base64 == null) { + base64 = toBase64Impl(); + } + return base64; + } + + @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() + "]"; + } } } diff --git a/core/src/main/java/org/dcache/nfs/v4/FileTracker.java b/core/src/main/java/org/dcache/nfs/v4/FileTracker.java index e6e5991c..1ff0b9a4 100644 --- a/core/src/main/java/org/dcache/nfs/v4/FileTracker.java +++ b/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 { diff --git a/core/src/main/java/org/dcache/nfs/v4/nlm/SimpleLm.java b/core/src/main/java/org/dcache/nfs/v4/nlm/SimpleLm.java index 0d1aa92d..71b1b676 100644 --- a/core/src/main/java/org/dcache/nfs/v4/nlm/SimpleLm.java +++ b/core/src/main/java/org/dcache/nfs/v4/nlm/SimpleLm.java @@ -58,36 +58,32 @@ public SimpleLm(int concurrency) { /** * Exclusive lock on objects locks. */ - private final ConcurrentHashMap> locks = new ConcurrentHashMap<>(); + private final ConcurrentHashMap> locks = new ConcurrentHashMap<>(); @Override protected Lock getObjectLock(Opaque objId) { - String key = toKey(objId); - return objLock.get(key); + return objLock.get(objId); } @Override protected Collection getActiveLocks(Opaque objId) { - String key = toKey(objId); - return locks.getOrDefault(key, Collections.emptyList()); + return locks.getOrDefault(objId, Collections.emptyList()); } @Override protected void add(Opaque objId, NlmLock lock) { - String key = toKey(objId); - Collection l = locks.computeIfAbsent(key, k -> new ArrayList<>()); + Collection l = locks.computeIfAbsent(objId.toImmutableOpaque(), k -> new ArrayList<>()); l.add(lock); } @Override protected boolean remove(Opaque objId, NlmLock lock) { - String key = toKey(objId); - Collection l = locks.get(key); + Collection l = locks.get(objId); boolean isRemoved = false; if (l != null) { isRemoved = l.remove(lock); if (l.isEmpty()) { - locks.remove(key); + locks.remove(objId); } } return isRemoved; @@ -95,25 +91,18 @@ protected boolean remove(Opaque objId, NlmLock lock) { @Override protected void addAll(Opaque objId, Collection locks) { - String key = toKey(objId); - Collection l = this.locks.computeIfAbsent(key, k -> new ArrayList<>()); + Collection l = this.locks.computeIfAbsent(objId.toImmutableOpaque(), k -> new ArrayList<>()); l.addAll(locks); } @Override protected void removeAll(Opaque objId, Collection locks) { - String key = toKey(objId); - Collection l = this.locks.get(key); + Collection l = this.locks.get(objId); if (l != null) { l.removeAll(locks); if (l.isEmpty()) { - this.locks.remove(key); + this.locks.remove(objId); } } } - - private final String toKey(Opaque objId) { - return objId.toBase64(); - } - } diff --git a/core/src/test/java/org/dcache/nfs/util/OpaqueTest.java b/core/src/test/java/org/dcache/nfs/util/OpaqueTest.java new file mode 100644 index 00000000..0d329121 --- /dev/null +++ b/core/src/test/java/org/dcache/nfs/util/OpaqueTest.java @@ -0,0 +1,75 @@ +package org.dcache.nfs.util; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNotSame; +import static org.junit.Assert.assertSame; + +import java.nio.ByteBuffer; + +import org.junit.Test; + +public class OpaqueTest { + @Test + public void testMutableByteBuffer() { + 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()); + } + + @Test + public void testMutableByteArray() throws Exception { + byte[] buf = new byte[] {(byte) 0x01, (byte) 0x02, (byte) 0x03, (byte) 0x04}; + Opaque bufOpaque = Opaque.forMutableByteArray(buf); + Opaque bytesOpaque = Opaque.forBytes(new byte[] {(byte) 0x01, (byte) 0x02, (byte) 0x03, (byte) 0x04}); + + assertEquals(bufOpaque.toBase64(), bytesOpaque.toBase64()); + assertEquals(bufOpaque, bytesOpaque); + assertEquals(bytesOpaque, bufOpaque); + assertEquals(bytesOpaque.hashCode(), bufOpaque.hashCode()); + + // change contents of mutable buffer + buf[3] = (byte) 0xDD; + assertNotEquals(bufOpaque.toBase64(), bytesOpaque.toBase64()); + assertNotEquals(bufOpaque, bytesOpaque); + assertNotEquals(bytesOpaque, bufOpaque); + assertNotEquals(bytesOpaque.hashCode(), bufOpaque.hashCode()); + } + + @Test + public void testToImmutable() throws Exception { + Opaque mutable = Opaque.forMutableByteBuffer(ByteBuffer.wrap(new byte[] { + (byte) 0x01, (byte) 0x02, (byte) 0x03, (byte) 0x04}), 0, 4); + Opaque mutableToImmutable = mutable.toImmutableOpaque(); + assertEquals(mutable, mutableToImmutable); + assertNotSame(mutable, mutableToImmutable); + + Opaque immutable = Opaque.forBytes(new byte[] {(byte) 0xAA, (byte) 0xBB, (byte) 0xCC, (byte) 0xDD}); + Opaque immutableToImmutable = immutable.toImmutableOpaque(); + assertEquals(immutable, immutableToImmutable); + assertSame(immutable, immutableToImmutable); + } +}