From 099b2f4de0f06a3c109a88168ac23b081f13706d Mon Sep 17 00:00:00 2001 From: Armin Schrenk Date: Thu, 16 Mar 2023 11:03:18 +0100 Subject: [PATCH 01/24] **not compile clean** first draft of refactoring chunk cache: * differentiate between active and stale chunks * only recycle buffers of stale chunks --- .../cryptofs/ch/CleartextFileChannel.java | 9 ++- .../org/cryptomator/cryptofs/fh/Chunk.java | 13 ++-- .../cryptomator/cryptofs/fh/ChunkCache.java | 73 ++++++++++++++----- .../cryptomator/cryptofs/fh/ChunkLoader.java | 4 +- .../cryptofs/ch/CleartextFileChannelTest.java | 16 ++-- .../cryptofs/fh/ChunkCacheTest.java | 48 +++++++++--- 6 files changed, 115 insertions(+), 48 deletions(-) diff --git a/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java b/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java index 436dde78..c276392d 100644 --- a/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java +++ b/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java @@ -4,8 +4,8 @@ import org.cryptomator.cryptofs.CryptoFileSystemStats; import org.cryptomator.cryptofs.EffectiveOpenOptions; import org.cryptomator.cryptofs.fh.BufferPool; -import org.cryptomator.cryptofs.fh.ChunkCache; import org.cryptomator.cryptofs.fh.Chunk; +import org.cryptomator.cryptofs.fh.ChunkCache; import org.cryptomator.cryptofs.fh.ExceptionsDuringWrite; import org.cryptomator.cryptofs.fh.OpenFileModifiedDate; import org.cryptomator.cryptofs.fh.OpenFileSize; @@ -106,7 +106,8 @@ protected synchronized int readLocked(ByteBuffer dst, long position) throws IOEx long pos = position + read; long chunkIndex = pos / payloadSize; // floor by int-truncation int offsetInChunk = (int) (pos % payloadSize); // known to fit in int, because payloadSize is int - ByteBuffer data = chunkCache.get(chunkIndex).data().duplicate().position(offsetInChunk); + var chunk = chunkCache.acquireChunk(chunkIndex); + ByteBuffer data = chunk.data().duplicate().position(offsetInChunk); int len = min(dst.remaining(), data.remaining()); // known to fit in int, because second argument is int dst.put(data.limit(data.position() + len)); read += len; @@ -161,7 +162,7 @@ private long writeLockedInternal(ByteSource src, long position) throws IOExcepti * We don't actually need to read the current data into the cache. * It would suffice if store the written data and do reading when storing the chunk. */ - Chunk chunk = chunkCache.get(chunkIndex); + Chunk chunk = chunkCache.acquireChunk(chunkIndex); chunk.data().limit(Math.max(chunk.data().limit(), offsetInChunk + len)); // increase limit (if needed) src.copyTo(chunk.data().duplicate().position(offsetInChunk)); // work on duplicate using correct offset chunk.dirty().set(true); @@ -196,7 +197,7 @@ protected void truncateLocked(long newSize) throws IOException { long indexOfLastChunk = (newSize + cleartextChunkSize - 1) / cleartextChunkSize - 1; int sizeOfIncompleteChunk = (int) (newSize % cleartextChunkSize); // known to fit in int, because cleartextChunkSize is int if (sizeOfIncompleteChunk > 0) { - var chunk = chunkCache.get(indexOfLastChunk); + var chunk = chunkCache.acquireChunk(indexOfLastChunk); chunk.data().limit(sizeOfIncompleteChunk); chunk.dirty().set(true); } diff --git a/src/main/java/org/cryptomator/cryptofs/fh/Chunk.java b/src/main/java/org/cryptomator/cryptofs/fh/Chunk.java index b2af8c70..992d2df6 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/Chunk.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/Chunk.java @@ -10,8 +10,7 @@ import java.nio.ByteBuffer; import java.util.concurrent.atomic.AtomicBoolean; - -import static java.lang.String.format; +import java.util.concurrent.atomic.AtomicInteger; /** * A chunk of plaintext data. It has these rules: @@ -23,14 +22,18 @@ *
  • When no longer used, the cleartext ByteBuffer may be recycled
    • * */ -public record Chunk(ByteBuffer data, AtomicBoolean dirty) { +public record Chunk(ByteBuffer data, AtomicBoolean dirty, AtomicInteger currentAccesses, Runnable onClose) implements AutoCloseable { - public Chunk(ByteBuffer data, boolean dirty) { - this(data, new AtomicBoolean(dirty)); + public Chunk(ByteBuffer data, boolean dirty, Runnable onClose) { + this(data, new AtomicBoolean(dirty), new AtomicInteger(0), onClose); } public boolean isDirty() { return dirty.get(); } + @Override + public void close() throws Exception { + onClose.run(); + } } diff --git a/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java b/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java index 215b58a6..06b94cb9 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java @@ -2,6 +2,7 @@ import com.google.common.cache.Cache; import com.google.common.cache.CacheBuilder; +import com.google.common.cache.RemovalCause; import com.google.common.cache.RemovalNotification; import org.cryptomator.cryptofs.CryptoFileSystemStats; import org.cryptomator.cryptolib.api.AuthenticationFailedException; @@ -9,7 +10,8 @@ import javax.inject.Inject; import java.io.IOException; import java.io.UncheckedIOException; -import java.util.concurrent.ExecutionException; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; @OpenFileScoped public class ChunkCache { @@ -20,7 +22,8 @@ public class ChunkCache { private final ChunkSaver chunkSaver; private final CryptoFileSystemStats stats; private final BufferPool bufferPool; - private final Cache chunks; + private final Cache staleChunks; + private final ConcurrentMap activeChunks; @Inject public ChunkCache(ChunkLoader chunkLoader, ChunkSaver chunkSaver, CryptoFileSystemStats stats, BufferPool bufferPool) { @@ -28,46 +31,78 @@ public ChunkCache(ChunkLoader chunkLoader, ChunkSaver chunkSaver, CryptoFileSyst this.chunkSaver = chunkSaver; this.stats = stats; this.bufferPool = bufferPool; - this.chunks = CacheBuilder.newBuilder() // + this.staleChunks = CacheBuilder.newBuilder() // .maximumSize(MAX_CACHED_CLEARTEXT_CHUNKS) // - .removalListener(this::removeChunk) // + .removalListener(this::evictChunk) // .build(); + this.activeChunks = new ConcurrentHashMap<>(); } - private Chunk loadChunk(long chunkIndex) throws IOException { + private Chunk loadChunk(long chunkIndex) throws AuthenticationFailedException, UncheckedIOException { stats.addChunkCacheMiss(); try { - return chunkLoader.load(chunkIndex); - } catch (AuthenticationFailedException e) { - // TODO provide means to pass an AuthenticationFailedException handler using an OpenOption - throw new IOException("Unauthentic ciphertext in chunk " + chunkIndex, e); + return new Chunk(chunkLoader.load(chunkIndex), false, () -> releaseChunk(chunkIndex)); + } catch (IOException e) { + throw new UncheckedIOException(e); } } - private void removeChunk(RemovalNotification removal) { + private synchronized void evictChunk(RemovalNotification removal) { try { - chunkSaver.save(removal.getKey(), removal.getValue()); - bufferPool.recycle(removal.getValue().data()); + if (removal.getCause() != RemovalCause.EXPLICIT) { + chunkSaver.save(removal.getKey(), removal.getValue()); + bufferPool.recycle(removal.getValue().data()); + } } catch (IOException e) { throw new UncheckedIOException(e); } } - public Chunk get(long chunkIndex) throws IOException { + public void releaseChunk(long chunkIndex) { + activeChunks.compute(chunkIndex, (index, chunk) -> { + assert chunk != null; + if (chunk.currentAccesses().decrementAndGet() == 0) { + staleChunks.put(index, chunk); + return null; //chunk is stale, remove from active + } else { + return chunk; //keep active + } + }); + } + + public Chunk acquireChunk(long chunkIndex) throws IOException { try { stats.addChunkCacheAccess(); - return chunks.get(chunkIndex, () -> loadChunk(chunkIndex)); - } catch (ExecutionException e) { - assert e.getCause() instanceof IOException; // the only checked exception thrown by #loadChunk(long) - throw (IOException) e.getCause(); + return activeChunks.compute(chunkIndex, this::internalCompute); + } catch (UncheckedIOException | AuthenticationFailedException e) { + throw new IOException(e); + } + } + + + private Chunk internalCompute(Long index, Chunk activeChunk) throws AuthenticationFailedException, UncheckedIOException { + Chunk result = activeChunk; + if (result == null) { + //check stale and put into active + result = staleChunks.getIfPresent(index); + staleChunks.invalidate(index); } + if (result == null) { + //load chunk + result = loadChunk(index); + } + + assert result != null; + result.currentAccesses().incrementAndGet(); + return result; } public void set(long chunkIndex, Chunk data) { - chunks.put(chunkIndex, data); + staleChunks.put(chunkIndex, data); } + //TODO: needs to be synchronized with the activeChunk map? public void invalidateAll() { - chunks.invalidateAll(); + staleChunks.invalidateAll(); } } diff --git a/src/main/java/org/cryptomator/cryptofs/fh/ChunkLoader.java b/src/main/java/org/cryptomator/cryptofs/fh/ChunkLoader.java index 3524e95a..2802a8e5 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/ChunkLoader.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/ChunkLoader.java @@ -26,7 +26,7 @@ public ChunkLoader(Cryptor cryptor, ChunkIO ciphertext, FileHeaderHolder headerH this.bufferPool = bufferPool; } - public Chunk load(Long chunkIndex) throws IOException, AuthenticationFailedException { + public ByteBuffer load(Long chunkIndex) throws IOException, AuthenticationFailedException { stats.addChunkCacheMiss(); int chunkSize = cryptor.fileContentCryptor().ciphertextChunkSize(); long ciphertextPos = chunkIndex * chunkSize + cryptor.fileHeaderCryptor().headerSize(); @@ -42,7 +42,7 @@ public Chunk load(Long chunkIndex) throws IOException, AuthenticationFailedExcep cleartextBuf.flip(); stats.addBytesDecrypted(cleartextBuf.remaining()); } - return new Chunk(cleartextBuf, false); + return cleartextBuf; } finally { bufferPool.recycle(ciphertextBuf); } diff --git a/src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java b/src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java index a2dfb966..edb52d67 100644 --- a/src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java +++ b/src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java @@ -76,7 +76,7 @@ public class CleartextFileChannelTest { public void setUp() throws IOException { when(cryptor.fileHeaderCryptor()).thenReturn(fileHeaderCryptor); when(cryptor.fileContentCryptor()).thenReturn(fileContentCryptor); - when(chunkCache.get(Mockito.anyLong())).then(invocation -> new Chunk(ByteBuffer.allocate(100), false)); + when(chunkCache.acquireChunk(Mockito.anyLong())).then(invocation -> new Chunk(ByteBuffer.allocate(100), false)); when(bufferPool.getCleartextBuffer()).thenAnswer(invocation -> ByteBuffer.allocate(100)); when(fileHeaderCryptor.headerSize()).thenReturn(50); when(fileContentCryptor.cleartextChunkSize()).thenReturn(100); @@ -354,10 +354,10 @@ public void testReadFromMultipleChunks() throws IOException { inTest.read(buf, 5_000_000_000l); InOrder inOrder = Mockito.inOrder(chunkCache, chunkCache, chunkCache, chunkCache); - inOrder.verify(chunkCache).get(0l); // A - inOrder.verify(chunkCache).get(1l); // B - inOrder.verify(chunkCache).get(2l); // B - inOrder.verify(chunkCache).get(50_000_000l); // C + inOrder.verify(chunkCache).acquireChunk(0l); // A + inOrder.verify(chunkCache).acquireChunk(1l); // B + inOrder.verify(chunkCache).acquireChunk(2l); // B + inOrder.verify(chunkCache).acquireChunk(50_000_000l); // C inOrder.verifyNoMoreInteractions(); } @@ -407,7 +407,7 @@ public void testWriteToMultipleChunks() throws IOException { inTest.write(buf3, 5000); InOrder inOrder = Mockito.inOrder(chunkCache, chunkCache, chunkCache); - inOrder.verify(chunkCache).get(0l); // A + inOrder.verify(chunkCache).acquireChunk(0l); // A inOrder.verify(chunkCache).set(Mockito.eq(1l), Mockito.any()); // B inOrder.verify(chunkCache).set(Mockito.eq(50l), Mockito.any()); // C inOrder.verifyNoMoreInteractions(); @@ -437,9 +437,9 @@ public void testWriteIncrementsPositionByAmountWritten() throws IOException { Assertions.assertEquals(110, written); Assertions.assertEquals(205, inTest.size()); - verify(chunkCache).get(0l); + verify(chunkCache).acquireChunk(0l); verify(chunkCache).set(Mockito.eq(1l), Mockito.any()); - verify(chunkCache).get(2l); + verify(chunkCache).acquireChunk(2l); } @Test diff --git a/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java b/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java index f62dd41b..23894d21 100644 --- a/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java +++ b/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java @@ -8,6 +8,7 @@ import java.io.IOException; import java.nio.ByteBuffer; +import java.util.concurrent.atomic.AtomicInteger; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; @@ -22,13 +23,28 @@ public class ChunkCacheTest { private final BufferPool bufferPool = mock(BufferPool.class); private final ChunkCache inTest = new ChunkCache(chunkLoader, chunkSaver, stats, bufferPool); + @Test + public void testGetIncreasesAccessCounter() throws IOException { + long index = 42L; + Chunk chunk = mock(Chunk.class); + AtomicInteger cnt = new AtomicInteger(0); + when(chunk.currentAccesses()).thenReturn(cnt); + when(chunkLoader.load(index)).thenReturn(chunk); + + var result = inTest.acquireChunk(index); + + Assertions.assertSame(chunk, result); + Assertions.assertEquals(1, cnt.get()); + } + @Test public void testGetInvokesLoaderIfEntryNotInCache() throws IOException, AuthenticationFailedException { long index = 42L; Chunk chunk = mock(Chunk.class); + when(chunk.currentAccesses()).thenReturn(new AtomicInteger()); when(chunkLoader.load(index)).thenReturn(chunk); - Assertions.assertSame(chunk, inTest.get(index)); + Assertions.assertSame(chunk, inTest.acquireChunk(index)); verify(stats).addChunkCacheAccess(); } @@ -36,10 +52,11 @@ public void testGetInvokesLoaderIfEntryNotInCache() throws IOException, Authenti public void testGetDoesNotInvokeLoaderIfEntryInCacheFromPreviousGet() throws IOException, AuthenticationFailedException { long index = 42L; Chunk chunk = mock(Chunk.class); + when(chunk.currentAccesses()).thenReturn(new AtomicInteger()); when(chunkLoader.load(index)).thenReturn(chunk); - inTest.get(index); + inTest.acquireChunk(index); - Assertions.assertSame(chunk, inTest.get(index)); + Assertions.assertSame(chunk, inTest.acquireChunk(index)); verify(stats, Mockito.times(2)).addChunkCacheAccess(); verify(chunkLoader).load(index); } @@ -48,24 +65,31 @@ public void testGetDoesNotInvokeLoaderIfEntryInCacheFromPreviousGet() throws IOE public void testGetDoesNotInvokeLoaderIfEntryInCacheFromPreviousSet() throws IOException { long index = 42L; Chunk chunk = mock(Chunk.class); + when(chunk.currentAccesses()).thenReturn(new AtomicInteger()); inTest.set(index, chunk); - Assertions.assertSame(chunk, inTest.get(index)); + Assertions.assertSame(chunk, inTest.acquireChunk(index)); verify(stats).addChunkCacheAccess(); } + @Test + public void testGetDoesNotRecycleBuffer + @Test public void testGetInvokesSaverIfMaxEntriesInCacheAreReachedAndAnEntryNotInCacheIsRequested() throws IOException, AuthenticationFailedException { long firstIndex = 42L; long indexNotInCache = 40L; Chunk chunk = mock(Chunk.class); + when(chunk.currentAccesses()).thenReturn(new AtomicInteger()); inTest.set(firstIndex, chunk); for (int i = 1; i < ChunkCache.MAX_CACHED_CLEARTEXT_CHUNKS; i++) { inTest.set(firstIndex + i, mock(Chunk.class)); } - when(chunkLoader.load(indexNotInCache)).thenReturn(mock(Chunk.class)); + Chunk notIndexedChunk = mock(Chunk.class); + when(notIndexedChunk.currentAccesses()).thenReturn(new AtomicInteger()); + when(chunkLoader.load(indexNotInCache)).thenReturn(notIndexedChunk); - inTest.get(indexNotInCache); + inTest.acquireChunk(indexNotInCache); verify(stats).addChunkCacheAccess(); verify(chunkSaver).save(firstIndex, chunk); @@ -78,6 +102,7 @@ public void testGetInvokesSaverIfMaxEntriesInCacheAreReachedAndAnEntryNotInCache long firstIndex = 42L; long indexNotInCache = 40L; Chunk chunk = mock(Chunk.class); + when(chunk.currentAccesses()).thenReturn(new AtomicInteger()); inTest.set(firstIndex, chunk); for (int i = 1; i < ChunkCache.MAX_CACHED_CLEARTEXT_CHUNKS; i++) { inTest.set(firstIndex + i, mock(Chunk.class)); @@ -95,6 +120,7 @@ public void testGetInvokesSaverIfMaxEntriesInCacheAreReachedAndAnEntryInCacheIsS // TODO markuskreusch: this behaviour isn't actually needed, maybe we can somehow prevent saving in such situations? long firstIndex = 42L; Chunk chunk = mock(Chunk.class); + when(chunk.currentAccesses()).thenReturn(new AtomicInteger()); inTest.set(firstIndex, chunk); for (int i = 1; i < ChunkCache.MAX_CACHED_CLEARTEXT_CHUNKS; i++) { inTest.set(firstIndex + i, mock(Chunk.class)); @@ -114,7 +140,7 @@ public void testGetRethrowsAuthenticationFailedExceptionFromLoader() throws IOEx when(chunkLoader.load(index)).thenThrow(authenticationFailedException); IOException e = Assertions.assertThrows(IOException.class, () -> { - inTest.get(index); + inTest.acquireChunk(index); }); Assertions.assertSame(authenticationFailedException, e.getCause()); } @@ -126,7 +152,7 @@ public void testGetThrowsUncheckedExceptionFromLoader() throws IOException, Auth when(chunkLoader.load(index)).thenThrow(uncheckedException); RuntimeException e = Assertions.assertThrows(RuntimeException.class, () -> { - inTest.get(index); + inTest.acquireChunk(index); }); Assertions.assertSame(uncheckedException, e.getCause()); } @@ -139,8 +165,10 @@ public void testInvalidateAllInvokesSaverForAllEntriesInCache() throws IOExcepti Chunk chunk2 = mock(Chunk.class); when(chunk1.data()).thenReturn(ByteBuffer.allocate(42)); when(chunk2.data()).thenReturn(ByteBuffer.allocate(23)); + when(chunk1.currentAccesses()).thenReturn(new AtomicInteger()); + when(chunk2.currentAccesses()).thenReturn(new AtomicInteger()); when(chunkLoader.load(index)).thenReturn(chunk1); - inTest.get(index); + inTest.acquireChunk(index); inTest.set(index2, chunk2); inTest.invalidateAll(); @@ -158,7 +186,7 @@ public void testGetRethrowsIOExceptionFromLoader() throws IOException, Authentic when(chunkLoader.load(index)).thenThrow(ioException); IOException e = Assertions.assertThrows(IOException.class, () -> { - inTest.get(index); + inTest.acquireChunk(index); }); Assertions.assertSame(ioException, e); } From 9a1ece7620e24b6915c9e1f8a927b4f7e9578c9a Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Thu, 16 Mar 2023 13:53:25 +0100 Subject: [PATCH 02/24] ChunkCache refactoring continues --- pom.xml | 16 ++ src/main/java/module-info.java | 2 +- .../cryptofs/ch/CleartextFileChannel.java | 33 ++-- .../org/cryptomator/cryptofs/fh/Chunk.java | 2 +- .../cryptomator/cryptofs/fh/ChunkCache.java | 128 +++++++++---- .../cryptomator/cryptofs/fh/ChunkLoader.java | 2 +- .../cryptofs/fh/OpenCryptoFile.java | 2 +- ...toFileChannelWriteReadIntegrationTest.java | 147 +++++++++++++++ .../cryptofs/ch/CleartextFileChannelTest.java | 26 +-- .../cryptofs/fh/ChunkCacheTest.java | 171 +++++++----------- .../cryptofs/fh/ChunkLoaderTest.java | 26 +-- .../cryptofs/fh/ChunkSaverTest.java | 8 +- .../cryptomator/cryptofs/fh/ChunkTest.java | 20 +- .../cryptofs/fh/OpenCryptoFileTest.java | 2 +- 14 files changed, 375 insertions(+), 210 deletions(-) diff --git a/pom.xml b/pom.xml index 4a539e05..949e3af9 100644 --- a/pom.xml +++ b/pom.xml @@ -22,6 +22,7 @@ 4.3.0 2.44.2 31.1-jre + 3.1.4 2.0.3 @@ -81,6 +82,21 @@ guava ${guava.version} + + com.github.ben-manes.caffeine + caffeine + ${caffeine.version} + + + org.checkerframework + checker-qual + + + com.google.errorprone + error_prone_annotations + + + org.slf4j slf4j-api diff --git a/src/main/java/module-info.java b/src/main/java/module-info.java index 97e5da36..5cbfb094 100644 --- a/src/main/java/module-info.java +++ b/src/main/java/module-info.java @@ -1,5 +1,4 @@ import org.cryptomator.cryptofs.CryptoFileSystemProvider; -import org.cryptomator.cryptofs.common.CiphertextFileType; import org.cryptomator.cryptofs.health.api.HealthCheck; import org.cryptomator.cryptofs.health.dirid.DirIdCheck; import org.cryptomator.cryptofs.health.shortened.ShortenedNamesCheck; @@ -10,6 +9,7 @@ module org.cryptomator.cryptofs { requires transitive org.cryptomator.cryptolib; requires com.google.common; + requires com.github.benmanes.caffeine; requires org.slf4j; requires dagger; requires com.auth0.jwt; diff --git a/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java b/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java index c276392d..b5d28d9b 100644 --- a/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java +++ b/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java @@ -106,11 +106,12 @@ protected synchronized int readLocked(ByteBuffer dst, long position) throws IOEx long pos = position + read; long chunkIndex = pos / payloadSize; // floor by int-truncation int offsetInChunk = (int) (pos % payloadSize); // known to fit in int, because payloadSize is int - var chunk = chunkCache.acquireChunk(chunkIndex); - ByteBuffer data = chunk.data().duplicate().position(offsetInChunk); - int len = min(dst.remaining(), data.remaining()); // known to fit in int, because second argument is int - dst.put(data.limit(data.position() + len)); - read += len; + try (var chunk = chunkCache.getChunk(chunkIndex)) { + ByteBuffer data = chunk.data().duplicate().position(offsetInChunk); + int len = min(dst.remaining(), data.remaining()); // known to fit in int, because second argument is int + dst.put(data.limit(data.position() + len)); + read += len; + } } dst.limit(origLimit); stats.addBytesRead(read); @@ -154,18 +155,18 @@ private long writeLockedInternal(ByteSource src, long position) throws IOExcepti ByteBuffer cleartextChunkData = bufferPool.getCleartextBuffer(); src.copyTo(cleartextChunkData); cleartextChunkData.flip(); - Chunk chunk = new Chunk(cleartextChunkData, true); - chunkCache.set(chunkIndex, chunk); + chunkCache.putChunk(chunkIndex, cleartextChunkData).close(); } else { /* * TODO performance: * We don't actually need to read the current data into the cache. * It would suffice if store the written data and do reading when storing the chunk. */ - Chunk chunk = chunkCache.acquireChunk(chunkIndex); - chunk.data().limit(Math.max(chunk.data().limit(), offsetInChunk + len)); // increase limit (if needed) - src.copyTo(chunk.data().duplicate().position(offsetInChunk)); // work on duplicate using correct offset - chunk.dirty().set(true); + try (Chunk chunk = chunkCache.getChunk(chunkIndex)) { + chunk.data().limit(Math.max(chunk.data().limit(), offsetInChunk + len)); // increase limit (if needed) + src.copyTo(chunk.data().duplicate().position(offsetInChunk)); // work on duplicate using correct offset + chunk.dirty().set(true); + } } written += len; } @@ -197,11 +198,13 @@ protected void truncateLocked(long newSize) throws IOException { long indexOfLastChunk = (newSize + cleartextChunkSize - 1) / cleartextChunkSize - 1; int sizeOfIncompleteChunk = (int) (newSize % cleartextChunkSize); // known to fit in int, because cleartextChunkSize is int if (sizeOfIncompleteChunk > 0) { - var chunk = chunkCache.acquireChunk(indexOfLastChunk); - chunk.data().limit(sizeOfIncompleteChunk); - chunk.dirty().set(true); + try (var chunk = chunkCache.getChunk(indexOfLastChunk)) { + chunk.data().limit(sizeOfIncompleteChunk); + chunk.dirty().set(true); + } } long ciphertextFileSize = cryptor.fileHeaderCryptor().headerSize() + cryptor.fileContentCryptor().ciphertextSize(newSize); + chunkCache.flush(); chunkCache.invalidateAll(); // make sure no chunks _after_ newSize exist that would otherwise be written during the next cache eviction ciphertextFileChannel.truncate(ciphertextFileSize); position = min(newSize, position); @@ -231,7 +234,7 @@ private void forceInternal(boolean metaData) throws IOException { private void flush() throws IOException { if (isWritable()) { writeHeaderIfNeeded(); - chunkCache.invalidateAll(); // TODO performance: write chunks but keep them cached + chunkCache.flush(); exceptionsDuringWrite.throwIfPresent(); } } diff --git a/src/main/java/org/cryptomator/cryptofs/fh/Chunk.java b/src/main/java/org/cryptomator/cryptofs/fh/Chunk.java index 992d2df6..7cfbf443 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/Chunk.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/Chunk.java @@ -33,7 +33,7 @@ public boolean isDirty() { } @Override - public void close() throws Exception { + public void close() { onClose.run(); } } diff --git a/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java b/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java index 06b94cb9..5ae902b1 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java @@ -1,15 +1,17 @@ package org.cryptomator.cryptofs.fh; -import com.google.common.cache.Cache; -import com.google.common.cache.CacheBuilder; -import com.google.common.cache.RemovalCause; -import com.google.common.cache.RemovalNotification; +import com.github.benmanes.caffeine.cache.Cache; +import com.github.benmanes.caffeine.cache.Caffeine; + +import com.github.benmanes.caffeine.cache.RemovalCause; +import com.google.common.base.Preconditions; import org.cryptomator.cryptofs.CryptoFileSystemStats; import org.cryptomator.cryptolib.api.AuthenticationFailedException; import javax.inject.Inject; import java.io.IOException; import java.io.UncheckedIOException; +import java.nio.ByteBuffer; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; @@ -31,56 +33,52 @@ public ChunkCache(ChunkLoader chunkLoader, ChunkSaver chunkSaver, CryptoFileSyst this.chunkSaver = chunkSaver; this.stats = stats; this.bufferPool = bufferPool; - this.staleChunks = CacheBuilder.newBuilder() // + this.staleChunks = Caffeine.newBuilder() // .maximumSize(MAX_CACHED_CLEARTEXT_CHUNKS) // - .removalListener(this::evictChunk) // + .evictionListener(this::evictStaleChunk) // .build(); this.activeChunks = new ConcurrentHashMap<>(); } - private Chunk loadChunk(long chunkIndex) throws AuthenticationFailedException, UncheckedIOException { - stats.addChunkCacheMiss(); - try { - return new Chunk(chunkLoader.load(chunkIndex), false, () -> releaseChunk(chunkIndex)); - } catch (IOException e) { - throw new UncheckedIOException(e); - } - } - - private synchronized void evictChunk(RemovalNotification removal) { - try { - if (removal.getCause() != RemovalCause.EXPLICIT) { - chunkSaver.save(removal.getKey(), removal.getValue()); - bufferPool.recycle(removal.getValue().data()); - } - } catch (IOException e) { - throw new UncheckedIOException(e); - } - } - - public void releaseChunk(long chunkIndex) { - activeChunks.compute(chunkIndex, (index, chunk) -> { - assert chunk != null; - if (chunk.currentAccesses().decrementAndGet() == 0) { - staleChunks.put(index, chunk); - return null; //chunk is stale, remove from active + /** + * Overwrites data at the given index + * + * @param chunkIndex Which chunk to overwrite + * @param chunkData The cleartext data + * @return The chunk + * @throws IllegalArgumentException If {@code chunkData}'s remaining bytes is not equal to the number of bytes fitting into a chunk + */ + public Chunk putChunk(long chunkIndex, ByteBuffer chunkData) throws IllegalArgumentException { + return activeChunks.compute(chunkIndex, (index, chunk) -> { + if (chunk == null) { + return new Chunk(chunkData, true, () -> releaseChunk(chunkIndex)); } else { - return chunk; //keep active + var dst = chunk.data().duplicate().clear(); + Preconditions.checkArgument(chunkData.remaining() == dst.remaining()); + dst.put(chunkData); + chunk.dirty().set(true); + return chunk; } }); } - public Chunk acquireChunk(long chunkIndex) throws IOException { + /** + * Returns the chunk for the given index, potentially loading the chunk into cache + * + * @param chunkIndex Which chunk to load + * @return The chunk + * @throws IOException If reading or decrypting the chunk failed + */ + public Chunk getChunk(long chunkIndex) throws IOException { try { stats.addChunkCacheAccess(); - return activeChunks.compute(chunkIndex, this::internalCompute); + return activeChunks.compute(chunkIndex, this::acquireInternal); } catch (UncheckedIOException | AuthenticationFailedException e) { throw new IOException(e); } } - - private Chunk internalCompute(Long index, Chunk activeChunk) throws AuthenticationFailedException, UncheckedIOException { + private Chunk acquireInternal(Long index, Chunk activeChunk) throws AuthenticationFailedException, UncheckedIOException { Chunk result = activeChunk; if (result == null) { //check stale and put into active @@ -97,12 +95,64 @@ private Chunk internalCompute(Long index, Chunk activeChunk) throws Authenticati return result; } - public void set(long chunkIndex, Chunk data) { - staleChunks.put(chunkIndex, data); + private Chunk loadChunk(long chunkIndex) throws AuthenticationFailedException, UncheckedIOException { + stats.addChunkCacheMiss(); + try { + return new Chunk(chunkLoader.load(chunkIndex), false, () -> releaseChunk(chunkIndex)); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + } + + @SuppressWarnings("resource") + private void releaseChunk(long chunkIndex) { + activeChunks.compute(chunkIndex, (index, chunk) -> { + assert chunk != null; + if (chunk.currentAccesses().decrementAndGet() == 0) { + staleChunks.put(index, chunk); + return null; //chunk is stale, remove from active + } else { + return chunk; //keep active + } + }); } + /** + * Flushes cached data (but keeps them cached). + * @see #invalidateAll() + */ //TODO: needs to be synchronized with the activeChunk map? + public void flush() { + activeChunks.replaceAll((index, chunk) -> { + saveChunk(index, chunk); + chunk.dirty().set(false); + return chunk; // keep + }); + // what happens if this thread is currently "here" and other thread just moved one item from stale to active? + staleChunks.asMap().replaceAll((index, chunk) -> { + saveChunk(index, chunk); + return chunk; // keep + }); + } + + /** + * Removes not currently used chunks from cache. + */ public void invalidateAll() { staleChunks.invalidateAll(); } + + private void evictStaleChunk(Long index, Chunk chunk, RemovalCause removalCause) { + assert removalCause != RemovalCause.EXPLICIT; // as per spec of Caffeine#evictionListener(RemovalListener) + saveChunk(index, chunk); + bufferPool.recycle(chunk.data()); + } + + private void saveChunk(long index, Chunk chunk) throws UncheckedIOException { + try { + chunkSaver.save(index, chunk); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + } } diff --git a/src/main/java/org/cryptomator/cryptofs/fh/ChunkLoader.java b/src/main/java/org/cryptomator/cryptofs/fh/ChunkLoader.java index 2802a8e5..87a55ee6 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/ChunkLoader.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/ChunkLoader.java @@ -30,7 +30,7 @@ public ByteBuffer load(Long chunkIndex) throws IOException, AuthenticationFailed stats.addChunkCacheMiss(); int chunkSize = cryptor.fileContentCryptor().ciphertextChunkSize(); long ciphertextPos = chunkIndex * chunkSize + cryptor.fileHeaderCryptor().headerSize(); - ByteBuffer ciphertextBuf = bufferPool.getCiphertextBuffer(); + ByteBuffer ciphertextBuf = ByteBuffer.allocate(cryptor.fileContentCryptor().ciphertextChunkSize()); // bufferPool.getCiphertextBuffer(); ByteBuffer cleartextBuf = bufferPool.getCleartextBuffer(); try { int read = ciphertext.read(ciphertextBuf, ciphertextPos); diff --git a/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java b/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java index 24d03020..0651e1c2 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java @@ -69,7 +69,7 @@ public synchronized FileChannel newFileChannel(EffectiveOpenOptions options, Fil Path path = currentFilePath.get(); if (options.truncateExisting()) { - chunkCache.invalidateAll(); + chunkCache.flush(); } FileChannel ciphertextFileChannel = null; diff --git a/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java b/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java index cfc773a1..0e4e18a6 100644 --- a/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java +++ b/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java @@ -18,6 +18,7 @@ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.RepeatedTest; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.TestInstance; import org.junit.jupiter.api.condition.EnabledOnOs; @@ -30,6 +31,7 @@ import org.mockito.Mockito; import java.io.IOException; +import java.lang.invoke.MethodHandles; import java.net.URI; import java.nio.ByteBuffer; import java.nio.channels.Channels; @@ -42,7 +44,15 @@ import java.nio.file.attribute.FileTime; import java.time.Instant; import java.time.temporal.ChronoUnit; +import java.util.Arrays; import java.util.List; +import java.util.Random; +import java.util.concurrent.BrokenBarrierException; +import java.util.concurrent.CyclicBarrier; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.stream.IntStream; import java.util.stream.Stream; import static java.lang.Math.min; @@ -463,6 +473,143 @@ public void testAppend(int dataSize) throws IOException { } } + @RepeatedTest(100) + public void testConcurrentRead() throws IOException, InterruptedException { + // prepare test data: + var content = new byte[10_000_000]; + for (int i = 0; i < 100_000; i++) { + byte b = (byte) (i % 255); + Arrays.fill(content, i * 100, (i + 1) * 100, b); + } + + try (var ch = FileChannel.open(file, CREATE, WRITE, TRUNCATE_EXISTING)) { + ch.write(ByteBuffer.wrap(content)); + } + + // read concurrently from same file channel: + var numThreads = 100; + var rnd = new Random(42L); + var results = new int[numThreads]; + var resultsHandle = MethodHandles.arrayElementVarHandle(int[].class); + Arrays.fill(results, 42); + var executor = Executors.newCachedThreadPool(); + try (var ch = FileChannel.open(file, READ)) { + for (int i = 0; i < numThreads; i++) { + int t = i; + int num = rnd.nextInt(50_000); + int pos = rnd.nextInt(400_000); + executor.submit(() -> { + ByteBuffer buf = ByteBuffer.allocate(num); + try { + System.out.println("thread " + t + " reading " + pos + " - " + (pos + num)); + int read = ch.read(buf, pos); + if (read != num) { + resultsHandle.setVolatile(results, t, -1); // ERROR invalid number of bytes + } else if (Arrays.equals(content, pos, pos + num, buf.array(), 0, read)) { + resultsHandle.setVolatile(results, t, 0); // SUCCESS + } else { + resultsHandle.setVolatile(results, t, -2); // ERROR invalid content + } + } catch (IOException e) { + e.printStackTrace(); + resultsHandle.setVolatile(results, t, -3); // ERROR I/O error + } + }); + } + executor.shutdown(); + boolean allTasksFinished = executor.awaitTermination(10, TimeUnit.SECONDS); + Assertions.assertTrue(allTasksFinished); + } + + Assertions.assertAll(IntStream.range(0, numThreads).mapToObj(t -> { + return () -> Assertions.assertEquals(0, resultsHandle.getVolatile(results, t), "thread " + t + " unsuccessful"); + })); + } + + @RepeatedTest(10) + @SuppressWarnings("PointlessArithmeticExpression") + public void testConcurrentChunkReuse() throws IOException, InterruptedException, BrokenBarrierException { + // prepare 16 chunks of test data: + var content = new byte[16 * 32 * 1024]; + for (int i = 0; i < 16; i++) { + Arrays.fill(content, i * 32 * 1024, (i+1) * 32 * 1024, (byte) (i * 0x11)); + } + try (var ch = FileChannel.open(file, CREATE, WRITE, TRUNCATE_EXISTING)) { + ch.write(ByteBuffer.wrap(content)); + } + + try (var ch = FileChannel.open(file, READ)) { + for (int i = 0; i < 5; i++) { + int pos1 = i * 3 * 32 * 1024; + int pos2 = (i * 3 + 2) * 32 * 1024; + + // read two chunks in this thread + ByteBuffer buf1 = ByteBuffer.allocate(2 * 32 * 1024); + ch.read(buf1, pos1); + + // read one chunk in other thread: + CyclicBarrier barrier1 = new CyclicBarrier(2); + var t1 = new Thread(() -> { + try { + barrier1.await(); + ByteBuffer buf2 = ByteBuffer.allocate(32 * 1024); + ch.read(buf2, pos2); + } catch (Exception e) { + e.printStackTrace(); + } + }); + t1.start(); + barrier1.await(); + + // read again in this thread + ByteBuffer buf3 = ByteBuffer.allocate(32 * 1024); + ch.read(buf3, pos2); + + // check if buf3 contains expected data + Assertions.assertArrayEquals(Arrays.copyOfRange(content, pos2, pos2 + buf3.capacity()), buf3.array(), "mismatch at pos2=" + pos2); + } + } + } + + + @RepeatedTest(1000) + public void testConcurrentByteBuffers() throws BrokenBarrierException, InterruptedException { + ByteBuffer buf = ByteBuffer.wrap("hello world".getBytes(StandardCharsets.US_ASCII)); + CyclicBarrier barrier1 = new CyclicBarrier(2); + CyclicBarrier barrier2 = new CyclicBarrier(2); + + var t1 = new Thread(() -> { + try { + buf.put("world hello".getBytes(StandardCharsets.US_ASCII)); + barrier2.await(); + } catch (Exception e) { + e.printStackTrace(); + } + }); + t1.start(); + + AtomicBoolean result = new AtomicBoolean(); + + var t2 = new Thread(() -> { + try { + barrier2.await(); + var canSeeT1 = Arrays.equals("world hello".getBytes(StandardCharsets.US_ASCII), buf.array()); + result.set(canSeeT1); + barrier1.await(); + } catch (Exception e) { + e.printStackTrace(); + } + }); + t2.start(); + + barrier1.await(); + + Assertions.assertTrue(result.get()); + + Assertions.assertArrayEquals("world hello".getBytes(StandardCharsets.US_ASCII), buf.array()); + } + + } } diff --git a/src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java b/src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java index edb52d67..6e333510 100644 --- a/src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java +++ b/src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java @@ -76,7 +76,7 @@ public class CleartextFileChannelTest { public void setUp() throws IOException { when(cryptor.fileHeaderCryptor()).thenReturn(fileHeaderCryptor); when(cryptor.fileContentCryptor()).thenReturn(fileContentCryptor); - when(chunkCache.acquireChunk(Mockito.anyLong())).then(invocation -> new Chunk(ByteBuffer.allocate(100), false)); + when(chunkCache.getChunk(Mockito.anyLong())).then(invocation -> new Chunk(ByteBuffer.allocate(100), false, () -> {})); when(bufferPool.getCleartextBuffer()).thenAnswer(invocation -> ByteBuffer.allocate(100)); when(fileHeaderCryptor.headerSize()).thenReturn(50); when(fileContentCryptor.cleartextChunkSize()).thenReturn(100); @@ -176,7 +176,7 @@ public void testForceInvalidatesChunkCacheWhenWritable() throws IOException { inTest.force(false); - verify(chunkCache).invalidateAll(); + verify(chunkCache).flush(); } @Test @@ -189,7 +189,7 @@ public void testForceRethrowsExceptionsDuringWrite() throws IOException { }); Assertions.assertEquals("exception during write", e.getMessage()); - verify(chunkCache).invalidateAll(); + verify(chunkCache).flush(); } @Test @@ -354,10 +354,10 @@ public void testReadFromMultipleChunks() throws IOException { inTest.read(buf, 5_000_000_000l); InOrder inOrder = Mockito.inOrder(chunkCache, chunkCache, chunkCache, chunkCache); - inOrder.verify(chunkCache).acquireChunk(0l); // A - inOrder.verify(chunkCache).acquireChunk(1l); // B - inOrder.verify(chunkCache).acquireChunk(2l); // B - inOrder.verify(chunkCache).acquireChunk(50_000_000l); // C + inOrder.verify(chunkCache).getChunk(0l); // A + inOrder.verify(chunkCache).getChunk(1l); // B + inOrder.verify(chunkCache).getChunk(2l); // B + inOrder.verify(chunkCache).getChunk(50_000_000l); // C inOrder.verifyNoMoreInteractions(); } @@ -407,9 +407,9 @@ public void testWriteToMultipleChunks() throws IOException { inTest.write(buf3, 5000); InOrder inOrder = Mockito.inOrder(chunkCache, chunkCache, chunkCache); - inOrder.verify(chunkCache).acquireChunk(0l); // A - inOrder.verify(chunkCache).set(Mockito.eq(1l), Mockito.any()); // B - inOrder.verify(chunkCache).set(Mockito.eq(50l), Mockito.any()); // C + inOrder.verify(chunkCache).getChunk(0l); // A + inOrder.verify(chunkCache).putChunk(Mockito.eq(1l), Mockito.any()); // B + inOrder.verify(chunkCache).putChunk(Mockito.eq(50l), Mockito.any()); // C inOrder.verifyNoMoreInteractions(); } @@ -437,9 +437,9 @@ public void testWriteIncrementsPositionByAmountWritten() throws IOException { Assertions.assertEquals(110, written); Assertions.assertEquals(205, inTest.size()); - verify(chunkCache).acquireChunk(0l); - verify(chunkCache).set(Mockito.eq(1l), Mockito.any()); - verify(chunkCache).acquireChunk(2l); + verify(chunkCache).getChunk(0l); + verify(chunkCache).putChunk(Mockito.eq(1l), Mockito.any()); + verify(chunkCache).getChunk(2l); } @Test diff --git a/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java b/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java index 23894d21..4301f14b 100644 --- a/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java +++ b/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java @@ -3,6 +3,7 @@ import org.cryptomator.cryptofs.CryptoFileSystemStats; import org.cryptomator.cryptolib.api.AuthenticationFailedException; import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; import org.mockito.Mockito; @@ -24,171 +25,137 @@ public class ChunkCacheTest { private final ChunkCache inTest = new ChunkCache(chunkLoader, chunkSaver, stats, bufferPool); @Test + @DisplayName("getChunk returns chunk with access count == 1") public void testGetIncreasesAccessCounter() throws IOException { long index = 42L; - Chunk chunk = mock(Chunk.class); - AtomicInteger cnt = new AtomicInteger(0); - when(chunk.currentAccesses()).thenReturn(cnt); - when(chunkLoader.load(index)).thenReturn(chunk); + var data = ByteBuffer.allocate(0); + when(chunkLoader.load(index)).thenReturn(data); - var result = inTest.acquireChunk(index); + var result = inTest.getChunk(index); - Assertions.assertSame(chunk, result); - Assertions.assertEquals(1, cnt.get()); + Assertions.assertEquals(1, result.currentAccesses().get()); } @Test + @DisplayName("getChunk invokes chunkLoader.load() on cache miss") public void testGetInvokesLoaderIfEntryNotInCache() throws IOException, AuthenticationFailedException { long index = 42L; - Chunk chunk = mock(Chunk.class); - when(chunk.currentAccesses()).thenReturn(new AtomicInteger()); - when(chunkLoader.load(index)).thenReturn(chunk); + var data = ByteBuffer.allocate(0); + when(chunkLoader.load(index)).thenReturn(data); - Assertions.assertSame(chunk, inTest.acquireChunk(index)); + inTest.getChunk(index); + + verify(chunkLoader).load(index); verify(stats).addChunkCacheAccess(); } @Test + @DisplayName("getChunk does not invoke chunkLoader.load() on cache hit due to getting chunk twice") public void testGetDoesNotInvokeLoaderIfEntryInCacheFromPreviousGet() throws IOException, AuthenticationFailedException { long index = 42L; - Chunk chunk = mock(Chunk.class); - when(chunk.currentAccesses()).thenReturn(new AtomicInteger()); - when(chunkLoader.load(index)).thenReturn(chunk); - inTest.acquireChunk(index); + var data = ByteBuffer.allocate(0); + when(chunkLoader.load(index)).thenReturn(data); + + var chunk = inTest.getChunk(index); + var sameChunk = inTest.getChunk(index); - Assertions.assertSame(chunk, inTest.acquireChunk(index)); + Assertions.assertSame(chunk, sameChunk); + verify(chunkLoader, Mockito.times(1)).load(index); verify(stats, Mockito.times(2)).addChunkCacheAccess(); - verify(chunkLoader).load(index); + verify(stats, Mockito.times(1)).addChunkCacheMiss(); } @Test + @DisplayName("getChunk does not invoke chunkLoader.load() on cache hit due to getting after putting") public void testGetDoesNotInvokeLoaderIfEntryInCacheFromPreviousSet() throws IOException { long index = 42L; - Chunk chunk = mock(Chunk.class); - when(chunk.currentAccesses()).thenReturn(new AtomicInteger()); - inTest.set(index, chunk); + var data = ByteBuffer.allocate(0); + var chunk = inTest.putChunk(index, data); - Assertions.assertSame(chunk, inTest.acquireChunk(index)); + Assertions.assertSame(chunk, inTest.getChunk(index)); verify(stats).addChunkCacheAccess(); } @Test - public void testGetDoesNotRecycleBuffer - - @Test + @DisplayName("getChunk() triggers cache eviction if stale cache contains MAX_CACHED_CLEARTEXT_CHUNKS entries") public void testGetInvokesSaverIfMaxEntriesInCacheAreReachedAndAnEntryNotInCacheIsRequested() throws IOException, AuthenticationFailedException { long firstIndex = 42L; long indexNotInCache = 40L; - Chunk chunk = mock(Chunk.class); - when(chunk.currentAccesses()).thenReturn(new AtomicInteger()); - inTest.set(firstIndex, chunk); + inTest.putChunk(firstIndex, ByteBuffer.allocate(0)).close(); for (int i = 1; i < ChunkCache.MAX_CACHED_CLEARTEXT_CHUNKS; i++) { - inTest.set(firstIndex + i, mock(Chunk.class)); + inTest.putChunk(firstIndex + i, ByteBuffer.allocate(0)).close(); } - Chunk notIndexedChunk = mock(Chunk.class); - when(notIndexedChunk.currentAccesses()).thenReturn(new AtomicInteger()); - when(chunkLoader.load(indexNotInCache)).thenReturn(notIndexedChunk); + when(chunkLoader.load(indexNotInCache)).thenReturn(ByteBuffer.allocate(0)); - inTest.acquireChunk(indexNotInCache); + inTest.getChunk(indexNotInCache); verify(stats).addChunkCacheAccess(); - verify(chunkSaver).save(firstIndex, chunk); - verify(bufferPool).recycle(chunk.data()); - verifyNoMoreInteractions(chunkSaver); - } - - @Test - public void testGetInvokesSaverIfMaxEntriesInCacheAreReachedAndAnEntryNotInCacheIsSet() throws IOException { - long firstIndex = 42L; - long indexNotInCache = 40L; - Chunk chunk = mock(Chunk.class); - when(chunk.currentAccesses()).thenReturn(new AtomicInteger()); - inTest.set(firstIndex, chunk); - for (int i = 1; i < ChunkCache.MAX_CACHED_CLEARTEXT_CHUNKS; i++) { - inTest.set(firstIndex + i, mock(Chunk.class)); - } - - inTest.set(indexNotInCache, mock(Chunk.class)); - - verify(chunkSaver).save(firstIndex, chunk); - verify(bufferPool).recycle(chunk.data()); - verifyNoMoreInteractions(chunkSaver); - } - - @Test - public void testGetInvokesSaverIfMaxEntriesInCacheAreReachedAndAnEntryInCacheIsSet() throws IOException { - // TODO markuskreusch: this behaviour isn't actually needed, maybe we can somehow prevent saving in such situations? - long firstIndex = 42L; - Chunk chunk = mock(Chunk.class); - when(chunk.currentAccesses()).thenReturn(new AtomicInteger()); - inTest.set(firstIndex, chunk); - for (int i = 1; i < ChunkCache.MAX_CACHED_CLEARTEXT_CHUNKS; i++) { - inTest.set(firstIndex + i, mock(Chunk.class)); - } - - inTest.set(firstIndex, mock(Chunk.class)); - - verify(chunkSaver).save(firstIndex, chunk); - verify(bufferPool).recycle(chunk.data()); + verify(chunkSaver).save(Mockito.eq(firstIndex), Mockito.any()); + verify(bufferPool).recycle(Mockito.any()); verifyNoMoreInteractions(chunkSaver); } @Test + @DisplayName("getChunk() propagates AuthenticationFailedException from loader") public void testGetRethrowsAuthenticationFailedExceptionFromLoader() throws IOException, AuthenticationFailedException { - long index = 42L; AuthenticationFailedException authenticationFailedException = new AuthenticationFailedException("Foo"); - when(chunkLoader.load(index)).thenThrow(authenticationFailedException); + when(chunkLoader.load(42L)).thenThrow(authenticationFailedException); IOException e = Assertions.assertThrows(IOException.class, () -> { - inTest.acquireChunk(index); + inTest.getChunk(42L); }); Assertions.assertSame(authenticationFailedException, e.getCause()); } @Test + @DisplayName("getChunk() rethrows RuntimeException from loader") public void testGetThrowsUncheckedExceptionFromLoader() throws IOException, AuthenticationFailedException { - long index = 42L; RuntimeException uncheckedException = new RuntimeException(); - when(chunkLoader.load(index)).thenThrow(uncheckedException); + when(chunkLoader.load(42L)).thenThrow(uncheckedException); + + Assertions.assertThrows(RuntimeException.class, () -> { + inTest.getChunk(42); + }); + } + + @Test + @DisplayName("getChunk() propagates IOException from loader") + public void testGetRethrowsIOExceptionFromLoader() throws IOException, AuthenticationFailedException { + when(chunkLoader.load(42L)).thenThrow(new IOException()); - RuntimeException e = Assertions.assertThrows(RuntimeException.class, () -> { - inTest.acquireChunk(index); + Assertions.assertThrows(IOException.class, () -> { + inTest.getChunk(42L); }); - Assertions.assertSame(uncheckedException, e.getCause()); } @Test - public void testInvalidateAllInvokesSaverForAllEntriesInCache() throws IOException, AuthenticationFailedException { + @DisplayName("flush() saves all active chunks") + public void testFlushInvokesSaverForAllActiveChunks() throws IOException, AuthenticationFailedException { long index = 42L; long index2 = 43L; - Chunk chunk1 = mock(Chunk.class); - Chunk chunk2 = mock(Chunk.class); - when(chunk1.data()).thenReturn(ByteBuffer.allocate(42)); - when(chunk2.data()).thenReturn(ByteBuffer.allocate(23)); - when(chunk1.currentAccesses()).thenReturn(new AtomicInteger()); - when(chunk2.currentAccesses()).thenReturn(new AtomicInteger()); - when(chunkLoader.load(index)).thenReturn(chunk1); - inTest.acquireChunk(index); - inTest.set(index2, chunk2); - - inTest.invalidateAll(); - - verify(chunkSaver).save(index, chunk1); - verify(bufferPool).recycle(chunk1.data()); - verify(chunkSaver).save(index2, chunk2); - verify(bufferPool).recycle(chunk2.data()); + + try (var chunk1 = inTest.putChunk(index, ByteBuffer.allocate(0)); var chunk2 = inTest.putChunk(index2, ByteBuffer.allocate(0))) { + inTest.flush(); + + verify(chunkSaver).save(Mockito.eq(index), Mockito.any()); + verify(chunkSaver).save(Mockito.eq(index2), Mockito.any()); + } } @Test - public void testGetRethrowsIOExceptionFromLoader() throws IOException, AuthenticationFailedException { + @DisplayName("flush() saves all stale chunks") + public void testFlushInvokesSaverForAllStaleChunks() throws IOException, AuthenticationFailedException { long index = 42L; - IOException ioException = new IOException(); - when(chunkLoader.load(index)).thenThrow(ioException); + long index2 = 43L; - IOException e = Assertions.assertThrows(IOException.class, () -> { - inTest.acquireChunk(index); - }); - Assertions.assertSame(ioException, e); + try (var chunk1 = inTest.putChunk(index, ByteBuffer.allocate(0)); var chunk2 = inTest.putChunk(index2, ByteBuffer.allocate(0))) { + // no-op + } + + inTest.flush(); + + verify(chunkSaver).save(Mockito.eq(index), Mockito.any()); + verify(chunkSaver).save(Mockito.eq(index2), Mockito.any()); } } diff --git a/src/test/java/org/cryptomator/cryptofs/fh/ChunkLoaderTest.java b/src/test/java/org/cryptomator/cryptofs/fh/ChunkLoaderTest.java index 8d05b987..15fe5fd3 100644 --- a/src/test/java/org/cryptomator/cryptofs/fh/ChunkLoaderTest.java +++ b/src/test/java/org/cryptomator/cryptofs/fh/ChunkLoaderTest.java @@ -65,12 +65,12 @@ public void testLoadReturnsEmptyChunkAfterEOF() throws IOException, Authenticati long chunkOffset = chunkIndex * CIPHERTEXT_CHUNK_SIZE + HEADER_SIZE; when(chunkIO.read(argThat(hasAtLeastRemaining(CIPHERTEXT_CHUNK_SIZE)), eq(chunkOffset))).thenReturn(-1); - Chunk chunk = inTest.load(chunkIndex); + var data = inTest.load(chunkIndex); verify(stats).addChunkCacheMiss(); verify(bufferPool).recycle(argThat(ByteBufferMatcher.hasCapacity(CIPHERTEXT_CHUNK_SIZE))); - Assertions.assertEquals(0, chunk.data().remaining()); - Assertions.assertEquals(CLEARTEXT_CHUNK_SIZE, chunk.data().capacity()); + Assertions.assertEquals(0, data.remaining()); + Assertions.assertEquals(CLEARTEXT_CHUNK_SIZE, data.capacity()); } @Test @@ -89,14 +89,14 @@ public void testLoadReturnsDecryptedDataInsideFile() throws IOException, Authent Mockito.any(), eq(chunkIndex), eq(header), eq(true) // ); - Chunk chunk = inTest.load(chunkIndex); + var data = inTest.load(chunkIndex); verify(stats).addChunkCacheMiss(); - verify(stats).addBytesDecrypted(chunk.data().remaining()); + verify(stats).addBytesDecrypted(data.remaining()); verify(bufferPool).recycle(argThat(ByteBufferMatcher.hasCapacity(CIPHERTEXT_CHUNK_SIZE))); - assertThat(chunk.data(), contains(decryptedData.get())); - Assertions.assertEquals(CLEARTEXT_CHUNK_SIZE, chunk.data().remaining()); - Assertions.assertEquals(CLEARTEXT_CHUNK_SIZE, chunk.data().capacity()); + assertThat(data, contains(decryptedData.get())); + Assertions.assertEquals(CLEARTEXT_CHUNK_SIZE, data.remaining()); + Assertions.assertEquals(CLEARTEXT_CHUNK_SIZE, data.capacity()); } @Test @@ -115,14 +115,14 @@ public void testLoadReturnsDecrytedDataNearEOF() throws IOException, Authenticat any(ByteBuffer.class), eq(chunkIndex), eq(header), eq(true) // ); - Chunk chunk = inTest.load(chunkIndex); + var data = inTest.load(chunkIndex); verify(stats).addChunkCacheMiss(); - verify(stats).addBytesDecrypted(chunk.data().remaining()); + verify(stats).addBytesDecrypted(data.remaining()); verify(bufferPool).recycle(argThat(ByteBufferMatcher.hasCapacity(CIPHERTEXT_CHUNK_SIZE))); - assertThat(chunk.data(), contains(decryptedData.get())); - Assertions.assertEquals(CLEARTEXT_CHUNK_SIZE - 3, chunk.data().remaining()); - Assertions.assertEquals(CLEARTEXT_CHUNK_SIZE, chunk.data().capacity()); + assertThat(data, contains(decryptedData.get())); + Assertions.assertEquals(CLEARTEXT_CHUNK_SIZE - 3, data.remaining()); + Assertions.assertEquals(CLEARTEXT_CHUNK_SIZE, data.capacity()); } private Answer fillBufferWith(byte value, int amount) { diff --git a/src/test/java/org/cryptomator/cryptofs/fh/ChunkSaverTest.java b/src/test/java/org/cryptomator/cryptofs/fh/ChunkSaverTest.java index e3f9e885..d818c092 100644 --- a/src/test/java/org/cryptomator/cryptofs/fh/ChunkSaverTest.java +++ b/src/test/java/org/cryptomator/cryptofs/fh/ChunkSaverTest.java @@ -59,7 +59,7 @@ public void testChunkInsideFileIsWritten() throws IOException { long expectedPosition = HEADER_SIZE + chunkIndex * CIPHERTEXT_CHUNK_SIZE; Supplier cleartext = () -> repeat(42).times(CLEARTEXT_CHUNK_SIZE).asByteBuffer(); Supplier ciphertext = () -> repeat(50).times(CIPHERTEXT_CHUNK_SIZE).asByteBuffer(); - Chunk chunk = new Chunk(cleartext.get(), true); + Chunk chunk = new Chunk(cleartext.get(), true, () -> {}); doAnswer(invocation -> { ByteBuffer ciphertextBuf = invocation.getArgument(1); ciphertextBuf.put(ciphertext.get()); @@ -79,7 +79,7 @@ public void testChunkContainingEndOfFileIsWrittenTillEndOfFile() throws IOExcept long expectedPosition = HEADER_SIZE + chunkIndex * CIPHERTEXT_CHUNK_SIZE; Supplier cleartext = () -> repeat(42).times(CLEARTEXT_CHUNK_SIZE - 10).asByteBuffer(); Supplier ciphertext = () -> repeat(50).times(CIPHERTEXT_CHUNK_SIZE - 10).asByteBuffer(); - Chunk chunk = new Chunk(cleartext.get(), true); + Chunk chunk = new Chunk(cleartext.get(), true, () -> {}); doAnswer(invocation -> { ByteBuffer ciphertextBuf = invocation.getArgument(1); ciphertextBuf.put(ciphertext.get()); @@ -96,7 +96,7 @@ public void testChunkContainingEndOfFileIsWrittenTillEndOfFile() throws IOExcept @Test public void testChunkThatWasNotWrittenIsNotWritten() throws IOException { Long chunkIndex = 43L; - Chunk chunk = new Chunk(ByteBuffer.allocate(CLEARTEXT_CHUNK_SIZE), false); + Chunk chunk = new Chunk(ByteBuffer.allocate(CLEARTEXT_CHUNK_SIZE), false, () -> {}); inTest.save(chunkIndex, chunk); @@ -112,7 +112,7 @@ public void testIOExceptionsDuringWriteAreAddedToExceptionsDuringWrite() throws long expectedPosition = HEADER_SIZE + chunkIndex * CIPHERTEXT_CHUNK_SIZE; Supplier cleartext = () -> repeat(42).times(CLEARTEXT_CHUNK_SIZE).asByteBuffer(); Supplier ciphertext = () -> repeat(50).times(CIPHERTEXT_CHUNK_SIZE).asByteBuffer(); - Chunk chunk = new Chunk(cleartext.get(), true); + Chunk chunk = new Chunk(cleartext.get(), true, () -> {}); doAnswer(invocation -> { ByteBuffer ciphertextBuf = invocation.getArgument(1); ciphertextBuf.put(ciphertext.get()); diff --git a/src/test/java/org/cryptomator/cryptofs/fh/ChunkTest.java b/src/test/java/org/cryptomator/cryptofs/fh/ChunkTest.java index 01716e12..4f05ee7e 100644 --- a/src/test/java/org/cryptomator/cryptofs/fh/ChunkTest.java +++ b/src/test/java/org/cryptomator/cryptofs/fh/ChunkTest.java @@ -15,28 +15,10 @@ public class ChunkTest { - @Test - public void testChunkDataWrappingBufferIsNotDirty() { - ByteBuffer buffer = repeat(3).times(200).asByteBuffer(); - - Chunk inTest = new Chunk(buffer, false); - - Assertions.assertFalse(inTest.isDirty()); - } - - @Test - public void testEmptyChunkDataIsNotDirty() { - ByteBuffer buffer = ByteBuffer.allocate(0); - - Chunk inTest = new Chunk(buffer, false); - - Assertions.assertFalse(inTest.isDirty()); - } - @Test // https://github.com/cryptomator/cryptofs/issues/85 public void testRaceConditionsDuringRead() { ByteBuffer src = StandardCharsets.US_ASCII.encode("abcdefg"); - Chunk inTest = new Chunk(src, false); + Chunk inTest = new Chunk(src, false, () -> {}); int attempts = 4000; int threads = 6; diff --git a/src/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFileTest.java b/src/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFileTest.java index afd5616d..0d5c1cf1 100644 --- a/src/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFileTest.java +++ b/src/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFileTest.java @@ -171,7 +171,7 @@ public void testTruncateExistingInvalidatesChunkCache() throws IOException { Files.write(CURRENT_FILE_PATH.get(), new byte[0]); EffectiveOpenOptions options = EffectiveOpenOptions.from(EnumSet.of(StandardOpenOption.TRUNCATE_EXISTING, StandardOpenOption.WRITE), readonlyFlag); openCryptoFile.newFileChannel(options); - verify(chunkCache).invalidateAll(); + verify(chunkCache).flush(); } @Test From 07f776ed59d9e41a7c0609192cba519c8654e89a Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Thu, 16 Mar 2023 17:12:53 +0100 Subject: [PATCH 03/24] =?UTF-8?q?make=20sure=20flush()=20is=20isolated=20a?= =?UTF-8?q?gainst=20getChunk()=20/=20releaseChunk()=20to=20avoid=20concurr?= =?UTF-8?q?ent=20moving=20of=20chunks=20from=20stale=20=E2=86=92=20active?= =?UTF-8?q?=20or=20vice=20versa?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../cryptofs/ch/CleartextFileChannel.java | 2 +- .../cryptomator/cryptofs/fh/ChunkCache.java | 65 ++++++++++++------- .../cryptofs/fh/OpenCryptoFile.java | 2 +- .../cryptofs/ch/CleartextFileChannelTest.java | 1 + .../cryptofs/fh/OpenCryptoFileTest.java | 2 +- 5 files changed, 46 insertions(+), 26 deletions(-) diff --git a/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java b/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java index b5d28d9b..959cd3b6 100644 --- a/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java +++ b/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java @@ -155,7 +155,7 @@ private long writeLockedInternal(ByteSource src, long position) throws IOExcepti ByteBuffer cleartextChunkData = bufferPool.getCleartextBuffer(); src.copyTo(cleartextChunkData); cleartextChunkData.flip(); - chunkCache.putChunk(chunkIndex, cleartextChunkData).close(); + chunkCache.putChunk(chunkIndex, cleartextChunkData).close(); // TODO can we recycle cleartextChunkData (note: depends on branch in putChunk)? } else { /* * TODO performance: diff --git a/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java b/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java index 5ae902b1..c1d071aa 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java @@ -2,7 +2,6 @@ import com.github.benmanes.caffeine.cache.Cache; import com.github.benmanes.caffeine.cache.Caffeine; - import com.github.benmanes.caffeine.cache.RemovalCause; import com.google.common.base.Preconditions; import org.cryptomator.cryptofs.CryptoFileSystemStats; @@ -14,6 +13,8 @@ import java.nio.ByteBuffer; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; @OpenFileScoped public class ChunkCache { @@ -27,6 +28,12 @@ public class ChunkCache { private final Cache staleChunks; private final ConcurrentMap activeChunks; + /** + * This lock ensures no chunks are passed between stale and active state while flushing, + * as flushing requires iteration over both sets. + */ + private final ReadWriteLock flushLock = new ReentrantReadWriteLock(); + @Inject public ChunkCache(ChunkLoader chunkLoader, ChunkSaver chunkSaver, CryptoFileSystemStats stats, BufferPool bufferPool) { this.chunkLoader = chunkLoader; @@ -70,23 +77,25 @@ public Chunk putChunk(long chunkIndex, ByteBuffer chunkData) throws IllegalArgum * @throws IOException If reading or decrypting the chunk failed */ public Chunk getChunk(long chunkIndex) throws IOException { + var lock = flushLock.readLock(); + lock.lock(); try { stats.addChunkCacheAccess(); return activeChunks.compute(chunkIndex, this::acquireInternal); } catch (UncheckedIOException | AuthenticationFailedException e) { throw new IOException(e); + } finally { + lock.unlock(); } } private Chunk acquireInternal(Long index, Chunk activeChunk) throws AuthenticationFailedException, UncheckedIOException { Chunk result = activeChunk; if (result == null) { - //check stale and put into active result = staleChunks.getIfPresent(index); staleChunks.invalidate(index); } if (result == null) { - //load chunk result = loadChunk(index); } @@ -106,33 +115,43 @@ private Chunk loadChunk(long chunkIndex) throws AuthenticationFailedException, U @SuppressWarnings("resource") private void releaseChunk(long chunkIndex) { - activeChunks.compute(chunkIndex, (index, chunk) -> { - assert chunk != null; - if (chunk.currentAccesses().decrementAndGet() == 0) { - staleChunks.put(index, chunk); - return null; //chunk is stale, remove from active - } else { - return chunk; //keep active - } - }); + var lock = flushLock.readLock(); + lock.lock(); + try { + activeChunks.compute(chunkIndex, (index, chunk) -> { + assert chunk != null; + if (chunk.currentAccesses().decrementAndGet() == 0) { + staleChunks.put(index, chunk); + return null; //chunk is stale, remove from active + } else { + return chunk; //keep active + } + }); + } finally { + lock.unlock(); + } } /** * Flushes cached data (but keeps them cached). * @see #invalidateAll() */ - //TODO: needs to be synchronized with the activeChunk map? public void flush() { - activeChunks.replaceAll((index, chunk) -> { - saveChunk(index, chunk); - chunk.dirty().set(false); - return chunk; // keep - }); - // what happens if this thread is currently "here" and other thread just moved one item from stale to active? - staleChunks.asMap().replaceAll((index, chunk) -> { - saveChunk(index, chunk); - return chunk; // keep - }); + var lock = flushLock.writeLock(); + lock.lock(); + try { + activeChunks.replaceAll((index, chunk) -> { + saveChunk(index, chunk); + chunk.dirty().set(false); + return chunk; // keep + }); + staleChunks.asMap().replaceAll((index, chunk) -> { + saveChunk(index, chunk); + return chunk; // keep + }); + } finally { + lock.unlock(); + } } /** diff --git a/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java b/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java index 0651e1c2..24d03020 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java @@ -69,7 +69,7 @@ public synchronized FileChannel newFileChannel(EffectiveOpenOptions options, Fil Path path = currentFilePath.get(); if (options.truncateExisting()) { - chunkCache.flush(); + chunkCache.invalidateAll(); } FileChannel ciphertextFileChannel = null; diff --git a/src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java b/src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java index 6e333510..fe9ac96f 100644 --- a/src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java +++ b/src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java @@ -77,6 +77,7 @@ public void setUp() throws IOException { when(cryptor.fileHeaderCryptor()).thenReturn(fileHeaderCryptor); when(cryptor.fileContentCryptor()).thenReturn(fileContentCryptor); when(chunkCache.getChunk(Mockito.anyLong())).then(invocation -> new Chunk(ByteBuffer.allocate(100), false, () -> {})); + when(chunkCache.putChunk(Mockito.anyLong(), Mockito.any())).thenAnswer(invocation -> new Chunk(invocation.getArgument(1), true, () -> {})); when(bufferPool.getCleartextBuffer()).thenAnswer(invocation -> ByteBuffer.allocate(100)); when(fileHeaderCryptor.headerSize()).thenReturn(50); when(fileContentCryptor.cleartextChunkSize()).thenReturn(100); diff --git a/src/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFileTest.java b/src/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFileTest.java index 0d5c1cf1..afd5616d 100644 --- a/src/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFileTest.java +++ b/src/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFileTest.java @@ -171,7 +171,7 @@ public void testTruncateExistingInvalidatesChunkCache() throws IOException { Files.write(CURRENT_FILE_PATH.get(), new byte[0]); EffectiveOpenOptions options = EffectiveOpenOptions.from(EnumSet.of(StandardOpenOption.TRUNCATE_EXISTING, StandardOpenOption.WRITE), readonlyFlag); openCryptoFile.newFileChannel(options); - verify(chunkCache).flush(); + verify(chunkCache).invalidateAll(); } @Test From 70bfe6a4fc178dbf35b65ef853ca16f7bebea575 Mon Sep 17 00:00:00 2001 From: Armin Schrenk Date: Thu, 16 Mar 2023 23:31:12 +0100 Subject: [PATCH 04/24] fix putChunk method, such that chunk would never be removed from activeChunks --- src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java | 7 ++++--- .../java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java b/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java index c1d071aa..2e7deee4 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java @@ -48,7 +48,7 @@ public ChunkCache(ChunkLoader chunkLoader, ChunkSaver chunkSaver, CryptoFileSyst } /** - * Overwrites data at the given index + * Overwrites data at the given index and increments the access counter of the returned chunk * * @param chunkIndex Which chunk to overwrite * @param chunkData The cleartext data @@ -58,14 +58,15 @@ public ChunkCache(ChunkLoader chunkLoader, ChunkSaver chunkSaver, CryptoFileSyst public Chunk putChunk(long chunkIndex, ByteBuffer chunkData) throws IllegalArgumentException { return activeChunks.compute(chunkIndex, (index, chunk) -> { if (chunk == null) { - return new Chunk(chunkData, true, () -> releaseChunk(chunkIndex)); + chunk = new Chunk(chunkData, true, () -> releaseChunk(chunkIndex)); } else { var dst = chunk.data().duplicate().clear(); Preconditions.checkArgument(chunkData.remaining() == dst.remaining()); dst.put(chunkData); chunk.dirty().set(true); - return chunk; } + chunk.currentAccesses().incrementAndGet(); + return chunk; }); } diff --git a/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java b/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java index 4301f14b..8b10e37a 100644 --- a/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java +++ b/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java @@ -87,7 +87,7 @@ public void testGetInvokesSaverIfMaxEntriesInCacheAreReachedAndAnEntryNotInCache } when(chunkLoader.load(indexNotInCache)).thenReturn(ByteBuffer.allocate(0)); - inTest.getChunk(indexNotInCache); + inTest.getChunk(indexNotInCache).close(); verify(stats).addChunkCacheAccess(); verify(chunkSaver).save(Mockito.eq(firstIndex), Mockito.any()); From 03d106cb19675b4ab565195cd6a54aa26c737199 Mon Sep 17 00:00:00 2001 From: Armin Schrenk Date: Fri, 17 Mar 2023 00:03:26 +0100 Subject: [PATCH 05/24] mark chunk only non-dirty, if saved to file --- src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java | 1 - src/main/java/org/cryptomator/cryptofs/fh/ChunkSaver.java | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java b/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java index 2e7deee4..45dc6ca0 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java @@ -143,7 +143,6 @@ public void flush() { try { activeChunks.replaceAll((index, chunk) -> { saveChunk(index, chunk); - chunk.dirty().set(false); return chunk; // keep }); staleChunks.asMap().replaceAll((index, chunk) -> { diff --git a/src/main/java/org/cryptomator/cryptofs/fh/ChunkSaver.java b/src/main/java/org/cryptomator/cryptofs/fh/ChunkSaver.java index dbb798b3..b56787a9 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/ChunkSaver.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/ChunkSaver.java @@ -37,6 +37,7 @@ public void save(long chunkIndex, Chunk chunkData) throws IOException { cryptor.fileContentCryptor().encryptChunk(cleartextBuf, ciphertextBuf, chunkIndex, headerHolder.get()); ciphertextBuf.flip(); ciphertext.write(ciphertextBuf, ciphertextPos); + chunkData.dirty().set(false); //TODO: what are the consequences } catch (IOException e) { exceptionsDuringWrite.add(e); } finally { From b842b35b1314411d5535e85073921a449d1dcab3 Mon Sep 17 00:00:00 2001 From: Armin Schrenk Date: Fri, 17 Mar 2023 00:19:01 +0100 Subject: [PATCH 06/24] add unit tests --- .../cryptomator/cryptofs/fh/ChunkCacheTest.java | 10 ++++++++++ .../cryptomator/cryptofs/fh/ChunkSaverTest.java | 14 ++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java b/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java index 8b10e37a..5d4e9fda 100644 --- a/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java +++ b/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java @@ -76,6 +76,16 @@ public void testGetDoesNotInvokeLoaderIfEntryInCacheFromPreviousSet() throws IOE verify(stats).addChunkCacheAccess(); } + @Test + @DisplayName("putChunk returns a dirty chunk") + public void testPutChunkReturnsDirtyChunk() throws IOException { + long index = 42L; + var data = ByteBuffer.allocate(0); + var chunk = inTest.putChunk(index, data); + + Assertions.assertTrue(chunk.isDirty()); + } + @Test @DisplayName("getChunk() triggers cache eviction if stale cache contains MAX_CACHED_CLEARTEXT_CHUNKS entries") public void testGetInvokesSaverIfMaxEntriesInCacheAreReachedAndAnEntryNotInCacheIsRequested() throws IOException, AuthenticationFailedException { diff --git a/src/test/java/org/cryptomator/cryptofs/fh/ChunkSaverTest.java b/src/test/java/org/cryptomator/cryptofs/fh/ChunkSaverTest.java index d818c092..e0c629ce 100644 --- a/src/test/java/org/cryptomator/cryptofs/fh/ChunkSaverTest.java +++ b/src/test/java/org/cryptomator/cryptofs/fh/ChunkSaverTest.java @@ -6,6 +6,7 @@ import org.cryptomator.cryptolib.api.FileContentCryptor; import org.cryptomator.cryptolib.api.FileHeader; import org.cryptomator.cryptolib.api.FileHeaderCryptor; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.Mockito; @@ -18,6 +19,7 @@ import static org.cryptomator.cryptofs.util.ByteBuffers.repeat; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; @@ -53,6 +55,18 @@ public void setup() throws IOException { when(bufferPool.getCleartextBuffer()).thenAnswer(invocation -> ByteBuffer.allocate(CLEARTEXT_CHUNK_SIZE)); } + @Test + public void testSuccessfullWrittenChunkIsNonDirty() throws IOException { + long chunkIndex = 43L; + Supplier cleartext = () -> repeat(42).times(CLEARTEXT_CHUNK_SIZE).asByteBuffer(); + Chunk chunk = new Chunk(cleartext.get(), true, () -> {}); + doNothing().when(fileContentCryptor).encryptChunk(argThat(contains(cleartext.get())), Mockito.any(), eq(chunkIndex), eq(header)); + + inTest.save(chunkIndex, chunk); + + Assertions.assertFalse(chunk.isDirty()); + } + @Test public void testChunkInsideFileIsWritten() throws IOException { long chunkIndex = 43L; From 89339fc730af7f5ce4d4353fcd175b12a50b4098 Mon Sep 17 00:00:00 2001 From: Armin Schrenk Date: Fri, 17 Mar 2023 12:57:28 +0100 Subject: [PATCH 07/24] minor changes: * resestablish use of bufferpool everywhere * add sync method to invalidateAll() --- .../org/cryptomator/cryptofs/fh/ChunkCache.java | 15 ++++++++++++--- .../org/cryptomator/cryptofs/fh/ChunkLoader.java | 4 +++- .../org/cryptomator/cryptofs/fh/ChunkSaver.java | 2 +- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java b/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java index 45dc6ca0..b9df899d 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java @@ -121,11 +121,14 @@ private void releaseChunk(long chunkIndex) { try { activeChunks.compute(chunkIndex, (index, chunk) -> { assert chunk != null; - if (chunk.currentAccesses().decrementAndGet() == 0) { + var accessCnt = chunk.currentAccesses().decrementAndGet(); + if (accessCnt == 0) { staleChunks.put(index, chunk); return null; //chunk is stale, remove from active - } else { + } else if(accessCnt > 0) { return chunk; //keep active + } else { + throw new IllegalStateException("Chunk access count is below 0"); } }); } finally { @@ -158,7 +161,13 @@ public void flush() { * Removes not currently used chunks from cache. */ public void invalidateAll() { - staleChunks.invalidateAll(); + var lock = flushLock.writeLock(); + lock.lock(); + try { + staleChunks.invalidateAll(); + } finally { + lock.unlock(); + } } private void evictStaleChunk(Long index, Chunk chunk, RemovalCause removalCause) { diff --git a/src/main/java/org/cryptomator/cryptofs/fh/ChunkLoader.java b/src/main/java/org/cryptomator/cryptofs/fh/ChunkLoader.java index 87a55ee6..69a1242a 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/ChunkLoader.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/ChunkLoader.java @@ -30,7 +30,9 @@ public ByteBuffer load(Long chunkIndex) throws IOException, AuthenticationFailed stats.addChunkCacheMiss(); int chunkSize = cryptor.fileContentCryptor().ciphertextChunkSize(); long ciphertextPos = chunkIndex * chunkSize + cryptor.fileHeaderCryptor().headerSize(); - ByteBuffer ciphertextBuf = ByteBuffer.allocate(cryptor.fileContentCryptor().ciphertextChunkSize()); // bufferPool.getCiphertextBuffer(); + //ByteBuffer ciphertextBuf = ByteBuffer.allocate(cryptor.fileContentCryptor().ciphertextChunkSize()); + ByteBuffer ciphertextBuf = bufferPool.getCiphertextBuffer(); + //ByteBuffer cleartextBuf = ByteBuffer.allocate(cryptor.fileContentCryptor().cleartextChunkSize()); ByteBuffer cleartextBuf = bufferPool.getCleartextBuffer(); try { int read = ciphertext.read(ciphertextBuf, ciphertextPos); diff --git a/src/main/java/org/cryptomator/cryptofs/fh/ChunkSaver.java b/src/main/java/org/cryptomator/cryptofs/fh/ChunkSaver.java index b56787a9..ea041b2a 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/ChunkSaver.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/ChunkSaver.java @@ -37,7 +37,7 @@ public void save(long chunkIndex, Chunk chunkData) throws IOException { cryptor.fileContentCryptor().encryptChunk(cleartextBuf, ciphertextBuf, chunkIndex, headerHolder.get()); ciphertextBuf.flip(); ciphertext.write(ciphertextBuf, ciphertextPos); - chunkData.dirty().set(false); //TODO: what are the consequences + chunkData.dirty().set(false); } catch (IOException e) { exceptionsDuringWrite.add(e); } finally { From 6387c9aa29e91b8e5deb6fc99a6670a7eebd7c05 Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Tue, 21 Mar 2023 13:53:54 +0100 Subject: [PATCH 08/24] make sure not to invoke `evictStaleChunk` non-atomically between ``` result = staleChunks.getIfPresent(index); staleChunks.invalidate(index); ``` --- .../cryptofs/ch/CleartextFileChannel.java | 2 +- .../cryptomator/cryptofs/fh/ChunkCache.java | 64 +++++++----- .../cryptomator/cryptofs/fh/ChunkLoader.java | 2 - .../cryptomator/cryptofs/fh/ChunkSaver.java | 10 +- ...toFileChannelWriteReadIntegrationTest.java | 99 ++----------------- .../cryptofs/fh/ChunkCacheTest.java | 26 ++++- .../cryptofs/fh/ChunkSaverTest.java | 24 +---- 7 files changed, 75 insertions(+), 152 deletions(-) diff --git a/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java b/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java index 959cd3b6..6ce6d320 100644 --- a/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java +++ b/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java @@ -93,7 +93,7 @@ protected boolean isReadable() { } @Override - protected synchronized int readLocked(ByteBuffer dst, long position) throws IOException { + protected int readLocked(ByteBuffer dst, long position) throws IOException { int origLimit = dst.limit(); long limitConsideringEof = fileSize.get() - position; if (limitConsideringEof < 1) { diff --git a/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java b/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java index b9df899d..9bb3bfb5 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java @@ -1,8 +1,9 @@ package org.cryptomator.cryptofs.fh; -import com.github.benmanes.caffeine.cache.Cache; import com.github.benmanes.caffeine.cache.Caffeine; +import com.github.benmanes.caffeine.cache.CaffeineSpec; import com.github.benmanes.caffeine.cache.RemovalCause; +import com.github.benmanes.caffeine.cache.Scheduler; import com.google.common.base.Preconditions; import org.cryptomator.cryptofs.CryptoFileSystemStats; import org.cryptomator.cryptolib.api.AuthenticationFailedException; @@ -11,10 +12,15 @@ import java.io.IOException; import java.io.UncheckedIOException; import java.nio.ByteBuffer; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.Executor; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; import java.util.concurrent.locks.ReadWriteLock; import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.function.BiFunction; @OpenFileScoped public class ChunkCache { @@ -25,7 +31,8 @@ public class ChunkCache { private final ChunkSaver chunkSaver; private final CryptoFileSystemStats stats; private final BufferPool bufferPool; - private final Cache staleChunks; + private final ExceptionsDuringWrite exceptionsDuringWrite; + private final ConcurrentMap staleChunks; private final ConcurrentMap activeChunks; /** @@ -35,15 +42,17 @@ public class ChunkCache { private final ReadWriteLock flushLock = new ReentrantReadWriteLock(); @Inject - public ChunkCache(ChunkLoader chunkLoader, ChunkSaver chunkSaver, CryptoFileSystemStats stats, BufferPool bufferPool) { + public ChunkCache(ChunkLoader chunkLoader, ChunkSaver chunkSaver, CryptoFileSystemStats stats, BufferPool bufferPool, ExceptionsDuringWrite exceptionsDuringWrite) { this.chunkLoader = chunkLoader; this.chunkSaver = chunkSaver; this.stats = stats; this.bufferPool = bufferPool; + this.exceptionsDuringWrite = exceptionsDuringWrite; this.staleChunks = Caffeine.newBuilder() // .maximumSize(MAX_CACHED_CLEARTEXT_CHUNKS) // .evictionListener(this::evictStaleChunk) // - .build(); + .build() // + .asMap(); this.activeChunks = new ConcurrentHashMap<>(); } @@ -93,8 +102,8 @@ public Chunk getChunk(long chunkIndex) throws IOException { private Chunk acquireInternal(Long index, Chunk activeChunk) throws AuthenticationFailedException, UncheckedIOException { Chunk result = activeChunk; if (result == null) { - result = staleChunks.getIfPresent(index); - staleChunks.invalidate(index); + result = staleChunks.remove(index); + assert result == null || result.currentAccesses().get() == 0; } if (result == null) { result = loadChunk(index); @@ -125,10 +134,9 @@ private void releaseChunk(long chunkIndex) { if (accessCnt == 0) { staleChunks.put(index, chunk); return null; //chunk is stale, remove from active - } else if(accessCnt > 0) { - return chunk; //keep active } else { - throw new IllegalStateException("Chunk access count is below 0"); + assert accessCnt > 0; + return chunk; //keep active } }); } finally { @@ -140,47 +148,49 @@ private void releaseChunk(long chunkIndex) { * Flushes cached data (but keeps them cached). * @see #invalidateAll() */ - public void flush() { + public void flush() throws IOException { var lock = flushLock.writeLock(); lock.lock(); + BiFunction saveUnchecked = (index, chunk) -> { + try { + chunkSaver.save(index, chunk); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + return chunk; + }; try { - activeChunks.replaceAll((index, chunk) -> { - saveChunk(index, chunk); - return chunk; // keep - }); - staleChunks.asMap().replaceAll((index, chunk) -> { - saveChunk(index, chunk); - return chunk; // keep - }); + activeChunks.replaceAll(saveUnchecked); + staleChunks.replaceAll(saveUnchecked); + } catch (UncheckedIOException e) { + throw new IOException(e); } finally { lock.unlock(); } } /** - * Removes not currently used chunks from cache. + * Removes stale chunks from cache. */ public void invalidateAll() { var lock = flushLock.writeLock(); lock.lock(); try { - staleChunks.invalidateAll(); + staleChunks.clear(); } finally { lock.unlock(); } } - private void evictStaleChunk(Long index, Chunk chunk, RemovalCause removalCause) { + // visible for testing + void evictStaleChunk(Long index, Chunk chunk, RemovalCause removalCause) { assert removalCause != RemovalCause.EXPLICIT; // as per spec of Caffeine#evictionListener(RemovalListener) - saveChunk(index, chunk); - bufferPool.recycle(chunk.data()); - } - - private void saveChunk(long index, Chunk chunk) throws UncheckedIOException { + assert chunk.currentAccesses().get() == 0; try { chunkSaver.save(index, chunk); } catch (IOException e) { - throw new UncheckedIOException(e); + exceptionsDuringWrite.add(e); } + bufferPool.recycle(chunk.data()); } } diff --git a/src/main/java/org/cryptomator/cryptofs/fh/ChunkLoader.java b/src/main/java/org/cryptomator/cryptofs/fh/ChunkLoader.java index 69a1242a..2802a8e5 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/ChunkLoader.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/ChunkLoader.java @@ -30,9 +30,7 @@ public ByteBuffer load(Long chunkIndex) throws IOException, AuthenticationFailed stats.addChunkCacheMiss(); int chunkSize = cryptor.fileContentCryptor().ciphertextChunkSize(); long ciphertextPos = chunkIndex * chunkSize + cryptor.fileHeaderCryptor().headerSize(); - //ByteBuffer ciphertextBuf = ByteBuffer.allocate(cryptor.fileContentCryptor().ciphertextChunkSize()); ByteBuffer ciphertextBuf = bufferPool.getCiphertextBuffer(); - //ByteBuffer cleartextBuf = ByteBuffer.allocate(cryptor.fileContentCryptor().cleartextChunkSize()); ByteBuffer cleartextBuf = bufferPool.getCleartextBuffer(); try { int read = ciphertext.read(ciphertextBuf, ciphertextPos); diff --git a/src/main/java/org/cryptomator/cryptofs/fh/ChunkSaver.java b/src/main/java/org/cryptomator/cryptofs/fh/ChunkSaver.java index ea041b2a..02504f4b 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/ChunkSaver.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/ChunkSaver.java @@ -13,16 +13,14 @@ class ChunkSaver { private final Cryptor cryptor; private final ChunkIO ciphertext; private final FileHeaderHolder headerHolder; - private final ExceptionsDuringWrite exceptionsDuringWrite; private final CryptoFileSystemStats stats; private final BufferPool bufferPool; @Inject - public ChunkSaver(Cryptor cryptor, ChunkIO ciphertext, FileHeaderHolder headerHolder, ExceptionsDuringWrite exceptionsDuringWrite, CryptoFileSystemStats stats, BufferPool bufferPool) { + public ChunkSaver(Cryptor cryptor, ChunkIO ciphertext, FileHeaderHolder headerHolder, CryptoFileSystemStats stats, BufferPool bufferPool) { this.cryptor = cryptor; this.ciphertext = ciphertext; this.headerHolder = headerHolder; - this.exceptionsDuringWrite = exceptionsDuringWrite; this.stats = stats; this.bufferPool = bufferPool; } @@ -30,7 +28,7 @@ public ChunkSaver(Cryptor cryptor, ChunkIO ciphertext, FileHeaderHolder headerHo public void save(long chunkIndex, Chunk chunkData) throws IOException { if (chunkData.isDirty()) { long ciphertextPos = chunkIndex * cryptor.fileContentCryptor().ciphertextChunkSize() + cryptor.fileHeaderCryptor().headerSize(); - ByteBuffer cleartextBuf = chunkData.data().duplicate(); + ByteBuffer cleartextBuf = chunkData.data().asReadOnlyBuffer(); stats.addBytesEncrypted(cleartextBuf.remaining()); ByteBuffer ciphertextBuf = bufferPool.getCiphertextBuffer(); try { @@ -38,11 +36,9 @@ public void save(long chunkIndex, Chunk chunkData) throws IOException { ciphertextBuf.flip(); ciphertext.write(ciphertextBuf, ciphertextPos); chunkData.dirty().set(false); - } catch (IOException e) { - exceptionsDuringWrite.add(e); } finally { bufferPool.recycle(ciphertextBuf); - } // unchecked exceptions will be propagated to the thread causing removal + } } } diff --git a/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java b/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java index 0e4e18a6..56b336d5 100644 --- a/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java +++ b/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java @@ -52,6 +52,8 @@ import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; import java.util.stream.IntStream; import java.util.stream.Stream; @@ -501,18 +503,19 @@ public void testConcurrentRead() throws IOException, InterruptedException { executor.submit(() -> { ByteBuffer buf = ByteBuffer.allocate(num); try { - System.out.println("thread " + t + " reading " + pos + " - " + (pos + num)); int read = ch.read(buf, pos); if (read != num) { - resultsHandle.setVolatile(results, t, -1); // ERROR invalid number of bytes + System.out.println("thread " + t + " read " + pos + " - " + (pos + num)); + resultsHandle.setOpaque(results, t, -1); // ERROR invalid number of bytes } else if (Arrays.equals(content, pos, pos + num, buf.array(), 0, read)) { - resultsHandle.setVolatile(results, t, 0); // SUCCESS + resultsHandle.setOpaque(results, t, 0); // SUCCESS } else { - resultsHandle.setVolatile(results, t, -2); // ERROR invalid content + System.out.println("thread " + t + " read " + pos + " - " + (pos + num)); + resultsHandle.setOpaque(results, t, -2); // ERROR invalid content } } catch (IOException e) { e.printStackTrace(); - resultsHandle.setVolatile(results, t, -3); // ERROR I/O error + resultsHandle.setOpaque(results, t, -3); // ERROR I/O error } }); } @@ -522,94 +525,10 @@ public void testConcurrentRead() throws IOException, InterruptedException { } Assertions.assertAll(IntStream.range(0, numThreads).mapToObj(t -> { - return () -> Assertions.assertEquals(0, resultsHandle.getVolatile(results, t), "thread " + t + " unsuccessful"); + return () -> Assertions.assertEquals(0, resultsHandle.getOpaque(results, t), "thread " + t + " unsuccessful"); })); } - @RepeatedTest(10) - @SuppressWarnings("PointlessArithmeticExpression") - public void testConcurrentChunkReuse() throws IOException, InterruptedException, BrokenBarrierException { - // prepare 16 chunks of test data: - var content = new byte[16 * 32 * 1024]; - for (int i = 0; i < 16; i++) { - Arrays.fill(content, i * 32 * 1024, (i+1) * 32 * 1024, (byte) (i * 0x11)); - } - try (var ch = FileChannel.open(file, CREATE, WRITE, TRUNCATE_EXISTING)) { - ch.write(ByteBuffer.wrap(content)); - } - - try (var ch = FileChannel.open(file, READ)) { - for (int i = 0; i < 5; i++) { - int pos1 = i * 3 * 32 * 1024; - int pos2 = (i * 3 + 2) * 32 * 1024; - - // read two chunks in this thread - ByteBuffer buf1 = ByteBuffer.allocate(2 * 32 * 1024); - ch.read(buf1, pos1); - - // read one chunk in other thread: - CyclicBarrier barrier1 = new CyclicBarrier(2); - var t1 = new Thread(() -> { - try { - barrier1.await(); - ByteBuffer buf2 = ByteBuffer.allocate(32 * 1024); - ch.read(buf2, pos2); - } catch (Exception e) { - e.printStackTrace(); - } - }); - t1.start(); - barrier1.await(); - - // read again in this thread - ByteBuffer buf3 = ByteBuffer.allocate(32 * 1024); - ch.read(buf3, pos2); - - // check if buf3 contains expected data - Assertions.assertArrayEquals(Arrays.copyOfRange(content, pos2, pos2 + buf3.capacity()), buf3.array(), "mismatch at pos2=" + pos2); - } - } - } - - - @RepeatedTest(1000) - public void testConcurrentByteBuffers() throws BrokenBarrierException, InterruptedException { - ByteBuffer buf = ByteBuffer.wrap("hello world".getBytes(StandardCharsets.US_ASCII)); - CyclicBarrier barrier1 = new CyclicBarrier(2); - CyclicBarrier barrier2 = new CyclicBarrier(2); - - var t1 = new Thread(() -> { - try { - buf.put("world hello".getBytes(StandardCharsets.US_ASCII)); - barrier2.await(); - } catch (Exception e) { - e.printStackTrace(); - } - }); - t1.start(); - - AtomicBoolean result = new AtomicBoolean(); - - var t2 = new Thread(() -> { - try { - barrier2.await(); - var canSeeT1 = Arrays.equals("world hello".getBytes(StandardCharsets.US_ASCII), buf.array()); - result.set(canSeeT1); - barrier1.await(); - } catch (Exception e) { - e.printStackTrace(); - } - }); - t2.start(); - - barrier1.await(); - - Assertions.assertTrue(result.get()); - - Assertions.assertArrayEquals("world hello".getBytes(StandardCharsets.US_ASCII), buf.array()); - } - - } } diff --git a/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java b/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java index 5d4e9fda..051af826 100644 --- a/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java +++ b/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java @@ -1,20 +1,29 @@ package org.cryptomator.cryptofs.fh; +import com.github.benmanes.caffeine.cache.RemovalCause; import org.cryptomator.cryptofs.CryptoFileSystemStats; +import org.cryptomator.cryptofs.matchers.ByteBufferMatcher; import org.cryptomator.cryptolib.api.AuthenticationFailedException; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.RepeatedTest; import org.junit.jupiter.api.Test; import org.mockito.Mockito; import java.io.IOException; import java.nio.ByteBuffer; import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Supplier; +import static org.cryptomator.cryptofs.matchers.ByteBufferMatcher.contains; +import static org.cryptomator.cryptofs.util.ByteBuffers.repeat; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; +import static org.mockito.hamcrest.MockitoHamcrest.argThat; public class ChunkCacheTest { @@ -22,7 +31,8 @@ public class ChunkCacheTest { private final ChunkSaver chunkSaver = mock(ChunkSaver.class); private final CryptoFileSystemStats stats = mock(CryptoFileSystemStats.class); private final BufferPool bufferPool = mock(BufferPool.class); - private final ChunkCache inTest = new ChunkCache(chunkLoader, chunkSaver, stats, bufferPool); + private final ExceptionsDuringWrite exceptionsDuringWrite = mock(ExceptionsDuringWrite.class); + private final ChunkCache inTest = new ChunkCache(chunkLoader, chunkSaver, stats, bufferPool, exceptionsDuringWrite); @Test @DisplayName("getChunk returns chunk with access count == 1") @@ -78,7 +88,7 @@ public void testGetDoesNotInvokeLoaderIfEntryInCacheFromPreviousSet() throws IOE @Test @DisplayName("putChunk returns a dirty chunk") - public void testPutChunkReturnsDirtyChunk() throws IOException { + public void testPutChunkReturnsDirtyChunk() { long index = 42L; var data = ByteBuffer.allocate(0); var chunk = inTest.putChunk(index, data); @@ -168,4 +178,16 @@ public void testFlushInvokesSaverForAllStaleChunks() throws IOException, Authent verify(chunkSaver).save(Mockito.eq(index2), Mockito.any()); } + + @Test + public void testIOExceptionsDuringWriteAreAddedToExceptionsDuringWrite() throws IOException { + IOException ioException = new IOException(); + Chunk chunk = new Chunk(ByteBuffer.allocate(0), true, () -> {}); + Mockito.doThrow(ioException).when(chunkSaver).save(42L, chunk); + + inTest.evictStaleChunk(42L, chunk, RemovalCause.EXPIRED); + + verify(exceptionsDuringWrite).add(ioException); + } + } diff --git a/src/test/java/org/cryptomator/cryptofs/fh/ChunkSaverTest.java b/src/test/java/org/cryptomator/cryptofs/fh/ChunkSaverTest.java index e0c629ce..a5cf7717 100644 --- a/src/test/java/org/cryptomator/cryptofs/fh/ChunkSaverTest.java +++ b/src/test/java/org/cryptomator/cryptofs/fh/ChunkSaverTest.java @@ -39,9 +39,8 @@ public class ChunkSaverTest { private final CryptoFileSystemStats stats = mock(CryptoFileSystemStats.class); private final FileHeader header = mock(FileHeader.class); private final FileHeaderHolder headerHolder = mock(FileHeaderHolder.class); - private final ExceptionsDuringWrite exceptionsDuringWrite = mock(ExceptionsDuringWrite.class); private final BufferPool bufferPool = mock(BufferPool.class); - private final ChunkSaver inTest = new ChunkSaver(cryptor, chunkIO, headerHolder, exceptionsDuringWrite, stats, bufferPool); + private final ChunkSaver inTest = new ChunkSaver(cryptor, chunkIO, headerHolder, stats, bufferPool); @BeforeEach public void setup() throws IOException { @@ -119,25 +118,4 @@ public void testChunkThatWasNotWrittenIsNotWritten() throws IOException { verifyNoInteractions(bufferPool); } - @Test - public void testIOExceptionsDuringWriteAreAddedToExceptionsDuringWrite() throws IOException { - IOException ioException = new IOException(); - long chunkIndex = 43L; - long expectedPosition = HEADER_SIZE + chunkIndex * CIPHERTEXT_CHUNK_SIZE; - Supplier cleartext = () -> repeat(42).times(CLEARTEXT_CHUNK_SIZE).asByteBuffer(); - Supplier ciphertext = () -> repeat(50).times(CIPHERTEXT_CHUNK_SIZE).asByteBuffer(); - Chunk chunk = new Chunk(cleartext.get(), true, () -> {}); - doAnswer(invocation -> { - ByteBuffer ciphertextBuf = invocation.getArgument(1); - ciphertextBuf.put(ciphertext.get()); - return null; - }).when(fileContentCryptor).encryptChunk(argThat(contains(cleartext.get())), Mockito.any(), eq(chunkIndex), eq(header)); - when(chunkIO.write(argThat(contains(ciphertext.get())), eq(expectedPosition))).thenThrow(ioException); - - inTest.save(chunkIndex, chunk); - - verify(exceptionsDuringWrite).add(ioException); - verify(bufferPool).recycle(argThat(ByteBufferMatcher.hasCapacity(CIPHERTEXT_CHUNK_SIZE))); - } - } From 191279220af675f4cc55d47defa88a57263e2eca Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Tue, 21 Mar 2023 17:07:21 +0100 Subject: [PATCH 09/24] make test more reliable (cache eviction may not run immediately) --- .../cryptofs/fh/ChunkCacheTest.java | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java b/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java index 051af826..66827af3 100644 --- a/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java +++ b/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java @@ -2,7 +2,6 @@ import com.github.benmanes.caffeine.cache.RemovalCause; import org.cryptomator.cryptofs.CryptoFileSystemStats; -import org.cryptomator.cryptofs.matchers.ByteBufferMatcher; import org.cryptomator.cryptolib.api.AuthenticationFailedException; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.DisplayName; @@ -12,18 +11,13 @@ import java.io.IOException; import java.nio.ByteBuffer; -import java.util.concurrent.atomic.AtomicInteger; -import java.util.function.Supplier; +import java.time.Duration; +import java.util.concurrent.CountDownLatch; -import static org.cryptomator.cryptofs.matchers.ByteBufferMatcher.contains; -import static org.cryptomator.cryptofs.util.ByteBuffers.repeat; -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; -import static org.mockito.hamcrest.MockitoHamcrest.argThat; public class ChunkCacheTest { @@ -96,19 +90,26 @@ public void testPutChunkReturnsDirtyChunk() { Assertions.assertTrue(chunk.isDirty()); } - @Test + @RepeatedTest(100) @DisplayName("getChunk() triggers cache eviction if stale cache contains MAX_CACHED_CLEARTEXT_CHUNKS entries") public void testGetInvokesSaverIfMaxEntriesInCacheAreReachedAndAnEntryNotInCacheIsRequested() throws IOException, AuthenticationFailedException { long firstIndex = 42L; long indexNotInCache = 40L; - inTest.putChunk(firstIndex, ByteBuffer.allocate(0)).close(); - for (int i = 1; i < ChunkCache.MAX_CACHED_CLEARTEXT_CHUNKS; i++) { - inTest.putChunk(firstIndex + i, ByteBuffer.allocate(0)).close(); + for (long i = firstIndex; i < firstIndex + ChunkCache.MAX_CACHED_CLEARTEXT_CHUNKS; i++) { + inTest.putChunk(i, ByteBuffer.allocate(0)).close(); } - when(chunkLoader.load(indexNotInCache)).thenReturn(ByteBuffer.allocate(0)); + var cdl = new CountDownLatch(1); + Mockito.doReturn(ByteBuffer.allocate(0)).when(chunkLoader).load(indexNotInCache); + Mockito.doAnswer(invocation -> { + cdl.countDown(); + return null; + }).when(chunkSaver).save(Mockito.anyLong(), Mockito.any()); inTest.getChunk(indexNotInCache).close(); + Assertions.assertTimeoutPreemptively(Duration.ofMillis(100), () -> { + cdl.await(); + }); verify(stats).addChunkCacheAccess(); verify(chunkSaver).save(Mockito.eq(firstIndex), Mockito.any()); verify(bufferPool).recycle(Mockito.any()); From a0e7411051cc27d8a1685303546c8b5584be7d67 Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Tue, 21 Mar 2023 17:14:38 +0100 Subject: [PATCH 10/24] added debug logging for CI build --- .../org/cryptomator/cryptofs/ch/CleartextFileChannel.java | 2 +- .../java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java | 1 + .../cryptofs/CryptoFileChannelWriteReadIntegrationTest.java | 6 ++++++ 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java b/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java index 6ce6d320..5fa3a066 100644 --- a/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java +++ b/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java @@ -50,7 +50,7 @@ public class CleartextFileChannel extends AbstractFileChannel { private final ExceptionsDuringWrite exceptionsDuringWrite; private final ChannelCloseListener closeListener; private final CryptoFileSystemStats stats; - private boolean mustWriteHeader; + public boolean mustWriteHeader; @Inject public CleartextFileChannel(FileChannel ciphertextFileChannel, FileHeader fileHeader, @MustWriteHeader boolean mustWriteHeader, ReadWriteLock readWriteLock, Cryptor cryptor, ChunkCache chunkCache, BufferPool bufferPool, EffectiveOpenOptions options, @OpenFileSize AtomicLong fileSize, @OpenFileModifiedDate AtomicReference lastModified, Supplier attrViewProvider, ExceptionsDuringWrite exceptionsDuringWrite, ChannelCloseListener closeListener, CryptoFileSystemStats stats) { diff --git a/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java b/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java index 24d03020..dafcfa2b 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java @@ -79,6 +79,7 @@ public synchronized FileChannel newFileChannel(EffectiveOpenOptions options, Fil final FileHeader header; final boolean isNewHeader; if (ciphertextFileChannel.size() == 0l) { + LOG.warn("ciphertext file channel size is ZERO"); header = headerHolder.createNew(); isNewHeader = true; } else { diff --git a/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java b/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java index 56b336d5..b0f62c1b 100644 --- a/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java +++ b/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java @@ -9,6 +9,7 @@ package org.cryptomator.cryptofs; import com.google.common.jimfs.Jimfs; +import org.cryptomator.cryptofs.ch.CleartextFileChannel; import org.cryptomator.cryptofs.util.ByteBuffers; import org.cryptomator.cryptolib.api.Masterkey; import org.cryptomator.cryptolib.api.MasterkeyLoader; @@ -241,7 +242,12 @@ public void testTruncateExistingWhileStillOpen() throws IOException { try (FileChannel ch1 = FileChannel.open(file, CREATE_NEW, WRITE)) { ch1.write(StandardCharsets.UTF_8.encode("goodbye world"), 0); ch1.force(true); // will generate a file header + System.out.println("opening second channel NOW..."); try (FileChannel ch2 = FileChannel.open(file, CREATE, WRITE, TRUNCATE_EXISTING)) { // reuse existing file header, but will not re-write it + if (ch2 instanceof CleartextFileChannel cfc) { + System.out.println("second channel opened: " + cfc.mustWriteHeader); + } + ch2.write(StandardCharsets.UTF_8.encode("hello world"), 0); } } From 260219aaf129a290c56db267f7b773bacd6bf4a7 Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Tue, 21 Mar 2023 17:18:16 +0100 Subject: [PATCH 11/24] adjust logging (for debug purposes only) --- src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java b/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java index dafcfa2b..e9c681e3 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java @@ -79,7 +79,7 @@ public synchronized FileChannel newFileChannel(EffectiveOpenOptions options, Fil final FileHeader header; final boolean isNewHeader; if (ciphertextFileChannel.size() == 0l) { - LOG.warn("ciphertext file channel size is ZERO"); + System.out.println("ciphertext file channel size is ZERO"); header = headerHolder.createNew(); isNewHeader = true; } else { From 3c90b00e3bf90ac9609f53227074c97c87464957 Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Tue, 21 Mar 2023 17:40:41 +0100 Subject: [PATCH 12/24] undo debug log --- .../org/cryptomator/cryptofs/fh/OpenCryptoFile.java | 1 - .../CryptoFileChannelWriteReadIntegrationTest.java | 12 +----------- 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java b/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java index e9c681e3..24d03020 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java @@ -79,7 +79,6 @@ public synchronized FileChannel newFileChannel(EffectiveOpenOptions options, Fil final FileHeader header; final boolean isNewHeader; if (ciphertextFileChannel.size() == 0l) { - System.out.println("ciphertext file channel size is ZERO"); header = headerHolder.createNew(); isNewHeader = true; } else { diff --git a/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java b/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java index b0f62c1b..8821b6f2 100644 --- a/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java +++ b/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java @@ -8,8 +8,8 @@ *******************************************************************************/ package org.cryptomator.cryptofs; +import com.google.common.jimfs.Configuration; import com.google.common.jimfs.Jimfs; -import org.cryptomator.cryptofs.ch.CleartextFileChannel; import org.cryptomator.cryptofs.util.ByteBuffers; import org.cryptomator.cryptolib.api.Masterkey; import org.cryptomator.cryptolib.api.MasterkeyLoader; @@ -48,13 +48,8 @@ import java.util.Arrays; import java.util.List; import java.util.Random; -import java.util.concurrent.BrokenBarrierException; -import java.util.concurrent.CyclicBarrier; import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicBoolean; -import java.util.concurrent.atomic.AtomicInteger; -import java.util.concurrent.atomic.AtomicReference; import java.util.stream.IntStream; import java.util.stream.Stream; @@ -242,12 +237,7 @@ public void testTruncateExistingWhileStillOpen() throws IOException { try (FileChannel ch1 = FileChannel.open(file, CREATE_NEW, WRITE)) { ch1.write(StandardCharsets.UTF_8.encode("goodbye world"), 0); ch1.force(true); // will generate a file header - System.out.println("opening second channel NOW..."); try (FileChannel ch2 = FileChannel.open(file, CREATE, WRITE, TRUNCATE_EXISTING)) { // reuse existing file header, but will not re-write it - if (ch2 instanceof CleartextFileChannel cfc) { - System.out.println("second channel opened: " + cfc.mustWriteHeader); - } - ch2.write(StandardCharsets.UTF_8.encode("hello world"), 0); } } From 2a7036a6e478aafe84947a15957567f872af21a2 Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Tue, 21 Mar 2023 17:41:03 +0100 Subject: [PATCH 13/24] atomic CAS for `mustWriteHeader` --- .../org/cryptomator/cryptofs/ch/CleartextFileChannel.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java b/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java index 5fa3a066..05c072d1 100644 --- a/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java +++ b/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java @@ -25,6 +25,7 @@ import java.nio.file.attribute.BasicFileAttributeView; import java.nio.file.attribute.FileTime; import java.time.Instant; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; import java.util.concurrent.locks.ReadWriteLock; @@ -50,7 +51,7 @@ public class CleartextFileChannel extends AbstractFileChannel { private final ExceptionsDuringWrite exceptionsDuringWrite; private final ChannelCloseListener closeListener; private final CryptoFileSystemStats stats; - public boolean mustWriteHeader; + private final AtomicBoolean mustWriteHeader; @Inject public CleartextFileChannel(FileChannel ciphertextFileChannel, FileHeader fileHeader, @MustWriteHeader boolean mustWriteHeader, ReadWriteLock readWriteLock, Cryptor cryptor, ChunkCache chunkCache, BufferPool bufferPool, EffectiveOpenOptions options, @OpenFileSize AtomicLong fileSize, @OpenFileModifiedDate AtomicReference lastModified, Supplier attrViewProvider, ExceptionsDuringWrite exceptionsDuringWrite, ChannelCloseListener closeListener, CryptoFileSystemStats stats) { @@ -70,7 +71,7 @@ public CleartextFileChannel(FileChannel ciphertextFileChannel, FileHeader fileHe if (options.append()) { position = fileSize.get(); } - this.mustWriteHeader = mustWriteHeader; + this.mustWriteHeader = new AtomicBoolean(mustWriteHeader); if (options.createNew() || options.create()) { lastModified.compareAndSet(Instant.EPOCH, Instant.now()); } @@ -182,11 +183,10 @@ private long writeLockedInternal(ByteSource src, long position) throws IOExcepti } private void writeHeaderIfNeeded() throws IOException { - if (mustWriteHeader) { + if (mustWriteHeader.getAndSet(false)) { LOG.trace("{} - Writing file header.", this); ByteBuffer encryptedHeader = cryptor.fileHeaderCryptor().encryptHeader(fileHeader); ciphertextFileChannel.write(encryptedHeader, 0); - mustWriteHeader = false; // write the header only once! } } From 066b3c6d989987cc1bb9b97db060a94cb6b0a2b6 Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Wed, 22 Mar 2023 15:52:36 +0100 Subject: [PATCH 14/24] fixes #160: FileHeader not re-generated during TRUNCATE_EXISTING --- .../cryptofs/fh/FileHeaderHolder.java | 7 +--- ...toFileChannelWriteReadIntegrationTest.java | 40 ++++++++++++++----- 2 files changed, 33 insertions(+), 14 deletions(-) diff --git a/src/main/java/org/cryptomator/cryptofs/fh/FileHeaderHolder.java b/src/main/java/org/cryptomator/cryptofs/fh/FileHeaderHolder.java index 8849392d..34abc1e1 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/FileHeaderHolder.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/FileHeaderHolder.java @@ -39,11 +39,8 @@ public FileHeader get() { public FileHeader createNew() { LOG.trace("Generating file header for {}", path.get()); FileHeader newHeader = cryptor.fileHeaderCryptor().create(); - if (header.compareAndSet(null, newHeader)) { - return newHeader; - } else { - return header.get(); - } + header.set(newHeader); + return newHeader; } public FileHeader loadExisting(FileChannel ch) throws IOException { diff --git a/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java b/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java index 8821b6f2..93216d8b 100644 --- a/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java +++ b/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java @@ -18,6 +18,7 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.RepeatedTest; import org.junit.jupiter.api.Test; @@ -233,18 +234,39 @@ public void testReadWhileStillWriting() throws IOException { // tests https://github.com/cryptomator/cryptofs/issues/48 @Test - public void testTruncateExistingWhileStillOpen() throws IOException { - try (FileChannel ch1 = FileChannel.open(file, CREATE_NEW, WRITE)) { - ch1.write(StandardCharsets.UTF_8.encode("goodbye world"), 0); - ch1.force(true); // will generate a file header - try (FileChannel ch2 = FileChannel.open(file, CREATE, WRITE, TRUNCATE_EXISTING)) { // reuse existing file header, but will not re-write it - ch2.write(StandardCharsets.UTF_8.encode("hello world"), 0); + @DisplayName("writing from second channel while first is still open") + public void testWriteFromSecondChannelWhileStillOpen() throws IOException { + try (var ch1 = FileChannel.open(file, CREATE_NEW, WRITE)) { + ch1.write(StandardCharsets.UTF_8.encode("howdy world"), 0); + ch1.force(true); + try (var ch2 = FileChannel.open(file, CREATE, WRITE)) { // reuse existing file header, but will not re-write it + ch2.write(StandardCharsets.UTF_8.encode("hello"), 0); } } - try (FileChannel ch1 = FileChannel.open(file, READ)) { - ByteBuffer buf = ByteBuffer.allocate((int) ch1.size()); - ch1.read(buf); + try (var ch3 = FileChannel.open(file, READ)) { + ByteBuffer buf = ByteBuffer.allocate((int) ch3.size()); + ch3.read(buf); + Assertions.assertArrayEquals("hello world".getBytes(), buf.array()); + } + } + + // tests https://github.com/cryptomator/cryptofs/issues/160 + @Test + @DisplayName("TRUNCATE_EXISTING leads to new file header") + public void testNewFileHeaderWhenTruncateExisting() throws IOException { + try (var ch1 = FileChannel.open(file, CREATE_NEW, WRITE)) { + ch1.write(StandardCharsets.UTF_8.encode("this content will be truncated soon"), 0); + ch1.force(true); + try (var ch2 = FileChannel.open(file, CREATE, WRITE, TRUNCATE_EXISTING)) { // re-roll file header + ch2.write(StandardCharsets.UTF_8.encode("hello"), 0); + } + ch1.write(StandardCharsets.UTF_8.encode(" world"), 5); // should use new file key + } + + try (var ch3 = FileChannel.open(file, READ)) { + ByteBuffer buf = ByteBuffer.allocate((int) ch3.size()); + ch3.read(buf); Assertions.assertArrayEquals("hello world".getBytes(), buf.array()); } } From 4a98152f8107843f509033e9e0205464d519c41f Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Wed, 22 Mar 2023 15:57:56 +0100 Subject: [PATCH 15/24] cleanup [ci skip] --- .../org/cryptomator/cryptofs/ch/CleartextFileChannel.java | 2 +- src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java | 6 ------ 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java b/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java index 05c072d1..883247df 100644 --- a/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java +++ b/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java @@ -156,7 +156,7 @@ private long writeLockedInternal(ByteSource src, long position) throws IOExcepti ByteBuffer cleartextChunkData = bufferPool.getCleartextBuffer(); src.copyTo(cleartextChunkData); cleartextChunkData.flip(); - chunkCache.putChunk(chunkIndex, cleartextChunkData).close(); // TODO can we recycle cleartextChunkData (note: depends on branch in putChunk)? + chunkCache.putChunk(chunkIndex, cleartextChunkData).close(); } else { /* * TODO performance: diff --git a/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java b/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java index 9bb3bfb5..cdfb9a96 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java @@ -1,9 +1,7 @@ package org.cryptomator.cryptofs.fh; import com.github.benmanes.caffeine.cache.Caffeine; -import com.github.benmanes.caffeine.cache.CaffeineSpec; import com.github.benmanes.caffeine.cache.RemovalCause; -import com.github.benmanes.caffeine.cache.Scheduler; import com.google.common.base.Preconditions; import org.cryptomator.cryptofs.CryptoFileSystemStats; import org.cryptomator.cryptolib.api.AuthenticationFailedException; @@ -12,12 +10,8 @@ import java.io.IOException; import java.io.UncheckedIOException; import java.nio.ByteBuffer; -import java.util.concurrent.CompletableFuture; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; -import java.util.concurrent.Executor; -import java.util.concurrent.Future; -import java.util.concurrent.TimeUnit; import java.util.concurrent.locks.ReadWriteLock; import java.util.concurrent.locks.ReentrantReadWriteLock; import java.util.function.BiFunction; From bdcb87e4bc8db27f1bd0f2bbceb258141b4521f1 Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Wed, 22 Mar 2023 16:14:31 +0100 Subject: [PATCH 16/24] fix javadoc [ci skip] --- src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFiles.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFiles.java b/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFiles.java index 1fa916d7..4186e248 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFiles.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFiles.java @@ -51,7 +51,7 @@ public Optional get(Path ciphertextPath) { } /** - * Opens a file to {@link OpenCryptoFile#newFileChannel(EffectiveOpenOptions) retrieve a FileChannel}. If this file is already opened, a shared instance is returned. + * Opens a file to {@link OpenCryptoFile#newFileChannel(EffectiveOpenOptions, java.nio.file.attribute.FileAttribute[]) retrieve a FileChannel}. If this file is already opened, a shared instance is returned. * Getting the file channel should be the next invocation, since the {@link OpenFileScoped lifecycle} of the OpenFile strictly depends on the lifecycle of the channel. * * @param ciphertextPath Path of the file to open From 32f0ef4b93012bd0bae0ec90a5f0a2a639f60749 Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Thu, 23 Mar 2023 09:12:14 +0100 Subject: [PATCH 17/24] improved tests --- .../cryptofs/fh/ChunkCacheTest.java | 282 ++++++++++++------ 1 file changed, 187 insertions(+), 95 deletions(-) diff --git a/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java b/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java index 66827af3..1e5ad2ef 100644 --- a/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java +++ b/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java @@ -4,13 +4,17 @@ import org.cryptomator.cryptofs.CryptoFileSystemStats; import org.cryptomator.cryptolib.api.AuthenticationFailedException; import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Assumptions; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.RepeatedTest; import org.junit.jupiter.api.Test; import org.mockito.Mockito; import java.io.IOException; import java.nio.ByteBuffer; +import java.nio.file.AccessDeniedException; import java.time.Duration; import java.util.concurrent.CountDownLatch; @@ -26,22 +30,15 @@ public class ChunkCacheTest { private final CryptoFileSystemStats stats = mock(CryptoFileSystemStats.class); private final BufferPool bufferPool = mock(BufferPool.class); private final ExceptionsDuringWrite exceptionsDuringWrite = mock(ExceptionsDuringWrite.class); - private final ChunkCache inTest = new ChunkCache(chunkLoader, chunkSaver, stats, bufferPool, exceptionsDuringWrite); + private ChunkCache inTest; - @Test - @DisplayName("getChunk returns chunk with access count == 1") - public void testGetIncreasesAccessCounter() throws IOException { - long index = 42L; - var data = ByteBuffer.allocate(0); - when(chunkLoader.load(index)).thenReturn(data); - - var result = inTest.getChunk(index); - - Assertions.assertEquals(1, result.currentAccesses().get()); + @BeforeEach + public void setup() { + inTest = new ChunkCache(chunkLoader, chunkSaver, stats, bufferPool, exceptionsDuringWrite); } @Test - @DisplayName("getChunk invokes chunkLoader.load() on cache miss") + @DisplayName("getChunk() invokes chunkLoader.load() on cache miss") public void testGetInvokesLoaderIfEntryNotInCache() throws IOException, AuthenticationFailedException { long index = 42L; var data = ByteBuffer.allocate(0); @@ -51,69 +48,7 @@ public void testGetInvokesLoaderIfEntryNotInCache() throws IOException, Authenti verify(chunkLoader).load(index); verify(stats).addChunkCacheAccess(); - } - - @Test - @DisplayName("getChunk does not invoke chunkLoader.load() on cache hit due to getting chunk twice") - public void testGetDoesNotInvokeLoaderIfEntryInCacheFromPreviousGet() throws IOException, AuthenticationFailedException { - long index = 42L; - var data = ByteBuffer.allocate(0); - when(chunkLoader.load(index)).thenReturn(data); - - var chunk = inTest.getChunk(index); - var sameChunk = inTest.getChunk(index); - - Assertions.assertSame(chunk, sameChunk); - verify(chunkLoader, Mockito.times(1)).load(index); - verify(stats, Mockito.times(2)).addChunkCacheAccess(); - verify(stats, Mockito.times(1)).addChunkCacheMiss(); - } - - @Test - @DisplayName("getChunk does not invoke chunkLoader.load() on cache hit due to getting after putting") - public void testGetDoesNotInvokeLoaderIfEntryInCacheFromPreviousSet() throws IOException { - long index = 42L; - var data = ByteBuffer.allocate(0); - var chunk = inTest.putChunk(index, data); - - Assertions.assertSame(chunk, inTest.getChunk(index)); - verify(stats).addChunkCacheAccess(); - } - - @Test - @DisplayName("putChunk returns a dirty chunk") - public void testPutChunkReturnsDirtyChunk() { - long index = 42L; - var data = ByteBuffer.allocate(0); - var chunk = inTest.putChunk(index, data); - - Assertions.assertTrue(chunk.isDirty()); - } - - @RepeatedTest(100) - @DisplayName("getChunk() triggers cache eviction if stale cache contains MAX_CACHED_CLEARTEXT_CHUNKS entries") - public void testGetInvokesSaverIfMaxEntriesInCacheAreReachedAndAnEntryNotInCacheIsRequested() throws IOException, AuthenticationFailedException { - long firstIndex = 42L; - long indexNotInCache = 40L; - for (long i = firstIndex; i < firstIndex + ChunkCache.MAX_CACHED_CLEARTEXT_CHUNKS; i++) { - inTest.putChunk(i, ByteBuffer.allocate(0)).close(); - } - var cdl = new CountDownLatch(1); - Mockito.doReturn(ByteBuffer.allocate(0)).when(chunkLoader).load(indexNotInCache); - Mockito.doAnswer(invocation -> { - cdl.countDown(); - return null; - }).when(chunkSaver).save(Mockito.anyLong(), Mockito.any()); - - inTest.getChunk(indexNotInCache).close(); - - Assertions.assertTimeoutPreemptively(Duration.ofMillis(100), () -> { - cdl.await(); - }); - verify(stats).addChunkCacheAccess(); - verify(chunkSaver).save(Mockito.eq(firstIndex), Mockito.any()); - verify(bufferPool).recycle(Mockito.any()); - verifyNoMoreInteractions(chunkSaver); + verify(stats).addChunkCacheMiss(); } @Test @@ -149,38 +84,195 @@ public void testGetRethrowsIOExceptionFromLoader() throws IOException, Authentic }); } - @Test - @DisplayName("flush() saves all active chunks") - public void testFlushInvokesSaverForAllActiveChunks() throws IOException, AuthenticationFailedException { - long index = 42L; - long index2 = 43L; + @Nested + @DisplayName("With active chunks [1] and stale chunks [42, 43, 44, 45, 46]") + public class Prepopulated { + + private Chunk activeChunk1; + private Chunk staleChunk42; + private Chunk staleChunk43; + private Chunk staleChunk44; + private Chunk staleChunk45; + private Chunk staleChunk46; + + @BeforeEach + public void setup() { + Assumptions.assumeTrue(ChunkCache.MAX_CACHED_CLEARTEXT_CHUNKS == 5); + + activeChunk1 = inTest.putChunk(1L, ByteBuffer.allocate(0)); + Assertions.assertEquals(1, activeChunk1.currentAccesses().get()); + + staleChunk42 = inTest.putChunk(42L, ByteBuffer.allocate(0)); + staleChunk43 = inTest.putChunk(43L, ByteBuffer.allocate(0)); + staleChunk44 = inTest.putChunk(44L, ByteBuffer.allocate(0)); + staleChunk45 = inTest.putChunk(45L, ByteBuffer.allocate(0)); + staleChunk46 = inTest.putChunk(46L, ByteBuffer.allocate(0)); + staleChunk42.close(); + staleChunk43.close(); + staleChunk44.close(); + staleChunk45.close(); + staleChunk46.close(); + Assertions.assertEquals(0, staleChunk42.currentAccesses().get()); + Assertions.assertEquals(0, staleChunk43.currentAccesses().get()); + Assertions.assertEquals(0, staleChunk44.currentAccesses().get()); + Assertions.assertEquals(0, staleChunk45.currentAccesses().get()); + Assertions.assertEquals(0, staleChunk46.currentAccesses().get()); + } + + @Test + @DisplayName("getChunk() does not invoke chunkLoader.load() and returns active chunk") + public void testGetChunkActive() throws IOException { + var chunk = inTest.getChunk(1L); + + Assertions.assertSame(activeChunk1, chunk); + Assertions.assertEquals(2, chunk.currentAccesses().get()); + verify(stats).addChunkCacheAccess(); + verifyNoMoreInteractions(stats); + verifyNoMoreInteractions(chunkLoader); + } + + @Test + @DisplayName("getChunk() does not invoke chunkLoader.load() and returns stale chunk") + public void testGetChunkStale() throws IOException { + var chunk = inTest.getChunk(42L); + + Assertions.assertSame(staleChunk42, chunk); + Assertions.assertEquals(1, chunk.currentAccesses().get()); + verify(stats).addChunkCacheAccess(); + verifyNoMoreInteractions(stats); + verifyNoMoreInteractions(chunkLoader); + } - try (var chunk1 = inTest.putChunk(index, ByteBuffer.allocate(0)); var chunk2 = inTest.putChunk(index2, ByteBuffer.allocate(0))) { + @Test + @DisplayName("chunk.close() keeps chunk active if access count > 1") + public void testClosingActiveChunkThatIsReferencedTwice() throws IOException, AuthenticationFailedException { + try (var chunk = inTest.getChunk(1L)) { + Assertions.assertSame(activeChunk1, chunk); + Assertions.assertEquals(2, activeChunk1.currentAccesses().get()); + } // close + + Assertions.assertEquals(1, activeChunk1.currentAccesses().get()); + verifyNoMoreInteractions(chunkSaver); + verifyNoMoreInteractions(bufferPool); + } + + @RepeatedTest(30) + @DisplayName("chunk.close() triggers eviction of LRU stale chunk") + public void testClosingActiveChunkTriggersEvictionOfStaleChunk() throws IOException, AuthenticationFailedException { + var cdl = new CountDownLatch(1); + Mockito.doAnswer(invocation -> { + cdl.countDown(); + return null; + }).when(chunkSaver).save(Mockito.anyLong(), Mockito.any()); + + activeChunk1.close(); + + Assertions.assertTimeoutPreemptively(Duration.ofMillis(100), () -> { + cdl.await(); + }); + verify(chunkSaver).save(42L, staleChunk42); + verify(bufferPool).recycle(staleChunk42.data()); + verifyNoMoreInteractions(chunkSaver); + } + + @Test + @DisplayName("flush() saves all stale chunks") + public void testFlushInvokesSaverForAllStaleChunks() throws IOException, AuthenticationFailedException { inTest.flush(); - verify(chunkSaver).save(Mockito.eq(index), Mockito.any()); - verify(chunkSaver).save(Mockito.eq(index2), Mockito.any()); + verify(chunkSaver).save(42L, staleChunk42); + verify(chunkSaver).save(43L, staleChunk43); + verify(chunkSaver).save(44L, staleChunk44); + verify(chunkSaver).save(45L, staleChunk45); + verify(chunkSaver).save(46L, staleChunk46); } - } - @Test - @DisplayName("flush() saves all stale chunks") - public void testFlushInvokesSaverForAllStaleChunks() throws IOException, AuthenticationFailedException { - long index = 42L; - long index2 = 43L; + @Test + @DisplayName("flush() saves all active chunks") + public void testFlushInvokesSaverForAllActiveChunks() throws IOException, AuthenticationFailedException { + try (var activeChunk2 = inTest.putChunk(2L, ByteBuffer.allocate(0))) { + inTest.flush(); - try (var chunk1 = inTest.putChunk(index, ByteBuffer.allocate(0)); var chunk2 = inTest.putChunk(index2, ByteBuffer.allocate(0))) { - // no-op + verify(chunkSaver).save(1L, activeChunk1); + verify(chunkSaver).save(2L, activeChunk2); + } } - inTest.flush(); + @Test + @DisplayName("flush() does not evict cached chunks") + public void testFlushKeepsItemInCache() throws IOException, AuthenticationFailedException { + inTest.flush(); - verify(chunkSaver).save(Mockito.eq(index), Mockito.any()); - verify(chunkSaver).save(Mockito.eq(index2), Mockito.any()); - } + Assertions.assertSame(activeChunk1, inTest.getChunk(1L)); + Assertions.assertSame(staleChunk42, inTest.getChunk(42L)); + verifyNoMoreInteractions(chunkLoader); + } + + @Test + @DisplayName("flush() does not evict cached chunks despite I/O error") + public void testFlushKeepsItemInCacheDespiteIOException() throws IOException, AuthenticationFailedException { + Mockito.doThrow(new AccessDeniedException("")).when(chunkSaver).save(42L, staleChunk42); + + Assertions.assertThrows(IOException.class, () -> inTest.flush()); + + Assertions.assertSame(activeChunk1, inTest.getChunk(1L)); + Assertions.assertSame(staleChunk42, inTest.getChunk(42L)); + verifyNoMoreInteractions(chunkLoader); + } + + @Test + @DisplayName("invalidateAll() flushes stale chunks but keeps active chunks") + public void testInvalidateAll() throws IOException, AuthenticationFailedException { + when(chunkLoader.load(Mockito.anyLong())).thenReturn(ByteBuffer.allocate(0)); + + inTest.invalidateAll(); + + Assertions.assertSame(activeChunk1, inTest.getChunk(1L)); + Assertions.assertNotSame(staleChunk42, inTest.getChunk(42L)); + Assertions.assertNotSame(staleChunk43, inTest.getChunk(43L)); + Assertions.assertNotSame(staleChunk44, inTest.getChunk(44L)); + Assertions.assertNotSame(staleChunk45, inTest.getChunk(45L)); + Assertions.assertNotSame(staleChunk46, inTest.getChunk(46L)); + verify(chunkLoader).load(42L); + verify(chunkLoader).load(43L); + verify(chunkLoader).load(44L); + verify(chunkLoader).load(45L); + verify(chunkLoader).load(46L); + verifyNoMoreInteractions(chunkLoader); + } + @Test + @DisplayName("putChunk() returns active chunk if already present") + public void testPutChunkReturnsActiveChunk() { + activeChunk1.dirty().set(false); + + var chunk = inTest.putChunk(1L, ByteBuffer.allocate(0)); + + Assertions.assertSame(activeChunk1, chunk); + Assertions.assertEquals(2, chunk.currentAccesses().get()); + Assertions.assertTrue(chunk.isDirty()); + } + + @Test + @DisplayName("putChunk() returns new chunk if neither stale nor active") + public void testPutChunkReturnsNewChunk() { + var chunk = inTest.putChunk(100L, ByteBuffer.allocate(0)); + + Assertions.assertNotSame(activeChunk1, chunk); + Assertions.assertNotSame(staleChunk42, chunk); + Assertions.assertNotSame(staleChunk43, chunk); + Assertions.assertNotSame(staleChunk44, chunk); + Assertions.assertNotSame(staleChunk45, chunk); + Assertions.assertNotSame(staleChunk46, chunk); + Assertions.assertEquals(1, chunk.currentAccesses().get()); + Assertions.assertTrue(chunk.isDirty()); + } + + + } @Test + @DisplayName("IOException during eviction of stale chunks is stored to exceptionsDuringWrite") public void testIOExceptionsDuringWriteAreAddedToExceptionsDuringWrite() throws IOException { IOException ioException = new IOException(); Chunk chunk = new Chunk(ByteBuffer.allocate(0), true, () -> {}); From 7c450a4bfe88cdcaaa30a0302cf05372a518ccb1 Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Thu, 23 Mar 2023 10:20:40 +0100 Subject: [PATCH 18/24] recycle stale buffers when overwriting via `putChunk()` --- .../java/org/cryptomator/cryptofs/fh/ChunkCache.java | 6 ++++++ .../org/cryptomator/cryptofs/fh/ChunkCacheTest.java | 11 +++++++++++ 2 files changed, 17 insertions(+) diff --git a/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java b/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java index cdfb9a96..e8aa56d5 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java @@ -60,6 +60,12 @@ public ChunkCache(ChunkLoader chunkLoader, ChunkSaver chunkSaver, CryptoFileSyst */ public Chunk putChunk(long chunkIndex, ByteBuffer chunkData) throws IllegalArgumentException { return activeChunks.compute(chunkIndex, (index, chunk) -> { + // stale chunk for this index is obsolete: + var staleChunk = staleChunks.remove(index); + if (staleChunk != null) { + bufferPool.recycle(staleChunk.data()); + } + // either create completely new chunk or replace all data of existing active chunk: if (chunk == null) { chunk = new Chunk(chunkData, true, () -> releaseChunk(chunkIndex)); } else { diff --git a/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java b/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java index 1e5ad2ef..d4cf0615 100644 --- a/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java +++ b/src/test/java/org/cryptomator/cryptofs/fh/ChunkCacheTest.java @@ -253,6 +253,17 @@ public void testPutChunkReturnsActiveChunk() { Assertions.assertTrue(chunk.isDirty()); } + @Test + @DisplayName("putChunk() recycles stale chunk if present") + public void testPutChunkRecyclesStaleChunk() { + var chunk = inTest.putChunk(42L, ByteBuffer.allocate(0)); + + Assertions.assertNotSame(staleChunk42, chunk); + Assertions.assertEquals(1, chunk.currentAccesses().get()); + Assertions.assertTrue(chunk.isDirty()); + verify(bufferPool).recycle(staleChunk42.data()); + } + @Test @DisplayName("putChunk() returns new chunk if neither stale nor active") public void testPutChunkReturnsNewChunk() { From 2b2d486ac7b6bf0b25e3d84c65aad54a3cff5308 Mon Sep 17 00:00:00 2001 From: Armin Schrenk Date: Thu, 23 Mar 2023 11:14:30 +0100 Subject: [PATCH 19/24] only record cache miss once --- src/main/java/org/cryptomator/cryptofs/fh/ChunkLoader.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/org/cryptomator/cryptofs/fh/ChunkLoader.java b/src/main/java/org/cryptomator/cryptofs/fh/ChunkLoader.java index 2802a8e5..e2fafb04 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/ChunkLoader.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/ChunkLoader.java @@ -27,7 +27,6 @@ public ChunkLoader(Cryptor cryptor, ChunkIO ciphertext, FileHeaderHolder headerH } public ByteBuffer load(Long chunkIndex) throws IOException, AuthenticationFailedException { - stats.addChunkCacheMiss(); int chunkSize = cryptor.fileContentCryptor().ciphertextChunkSize(); long ciphertextPos = chunkIndex * chunkSize + cryptor.fileHeaderCryptor().headerSize(); ByteBuffer ciphertextBuf = bufferPool.getCiphertextBuffer(); From 3a6733a3e56b309dce17f816c227d2698bdc5ef1 Mon Sep 17 00:00:00 2001 From: Armin Schrenk Date: Thu, 23 Mar 2023 12:11:25 +0100 Subject: [PATCH 20/24] fix unit tests --- src/test/java/org/cryptomator/cryptofs/fh/ChunkLoaderTest.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/test/java/org/cryptomator/cryptofs/fh/ChunkLoaderTest.java b/src/test/java/org/cryptomator/cryptofs/fh/ChunkLoaderTest.java index 15fe5fd3..dac1d303 100644 --- a/src/test/java/org/cryptomator/cryptofs/fh/ChunkLoaderTest.java +++ b/src/test/java/org/cryptomator/cryptofs/fh/ChunkLoaderTest.java @@ -67,7 +67,6 @@ public void testLoadReturnsEmptyChunkAfterEOF() throws IOException, Authenticati var data = inTest.load(chunkIndex); - verify(stats).addChunkCacheMiss(); verify(bufferPool).recycle(argThat(ByteBufferMatcher.hasCapacity(CIPHERTEXT_CHUNK_SIZE))); Assertions.assertEquals(0, data.remaining()); Assertions.assertEquals(CLEARTEXT_CHUNK_SIZE, data.capacity()); @@ -91,7 +90,6 @@ public void testLoadReturnsDecryptedDataInsideFile() throws IOException, Authent var data = inTest.load(chunkIndex); - verify(stats).addChunkCacheMiss(); verify(stats).addBytesDecrypted(data.remaining()); verify(bufferPool).recycle(argThat(ByteBufferMatcher.hasCapacity(CIPHERTEXT_CHUNK_SIZE))); assertThat(data, contains(decryptedData.get())); @@ -117,7 +115,6 @@ public void testLoadReturnsDecrytedDataNearEOF() throws IOException, Authenticat var data = inTest.load(chunkIndex); - verify(stats).addChunkCacheMiss(); verify(stats).addBytesDecrypted(data.remaining()); verify(bufferPool).recycle(argThat(ByteBufferMatcher.hasCapacity(CIPHERTEXT_CHUNK_SIZE))); assertThat(data, contains(decryptedData.get())); From 98629ef8fcf131362638116ecee9c9e703bf9f1f Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Thu, 23 Mar 2023 13:02:52 +0100 Subject: [PATCH 21/24] store NonWritableChannelException during cache eviction --- src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java | 2 +- .../java/org/cryptomator/cryptofs/fh/ExceptionsDuringWrite.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java b/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java index e8aa56d5..2364005f 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java @@ -188,7 +188,7 @@ void evictStaleChunk(Long index, Chunk chunk, RemovalCause removalCause) { assert chunk.currentAccesses().get() == 0; try { chunkSaver.save(index, chunk); - } catch (IOException e) { + } catch (IOException | NonWritableChannelException e) { exceptionsDuringWrite.add(e); } bufferPool.recycle(chunk.data()); diff --git a/src/main/java/org/cryptomator/cryptofs/fh/ExceptionsDuringWrite.java b/src/main/java/org/cryptomator/cryptofs/fh/ExceptionsDuringWrite.java index 1586a6b7..b0cf0931 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/ExceptionsDuringWrite.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/ExceptionsDuringWrite.java @@ -18,7 +18,7 @@ public class ExceptionsDuringWrite { public ExceptionsDuringWrite() { } - public synchronized void add(IOException e) { + public synchronized void add(Exception e) { e.addSuppressed(e); } From dc0e99fc89fe157b6f6bc2522db03ab11047b92f Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Fri, 24 Mar 2023 11:07:47 +0100 Subject: [PATCH 22/24] fix missing import in 98629ef --- src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java b/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java index 2364005f..199c4cb1 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/ChunkCache.java @@ -10,6 +10,7 @@ import java.io.IOException; import java.io.UncheckedIOException; import java.nio.ByteBuffer; +import java.nio.channels.NonWritableChannelException; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; import java.util.concurrent.locks.ReadWriteLock; From cb9a4ffb2a7812a2843391aee707c75d816a0d70 Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Fri, 24 Mar 2023 11:33:46 +0100 Subject: [PATCH 23/24] migrated from Guava Cache to Coffeine --- .../cryptofs/CryptoPathMapper.java | 31 ++++++------- .../cryptofs/DirectoryIdLoader.java | 9 ++-- .../cryptofs/DirectoryIdProvider.java | 17 +++---- .../cryptofs/LongFileNameProvider.java | 44 ++++++++----------- .../cryptofs/DirectoryIdLoaderTest.java | 9 ++-- .../cryptofs/DirectoryIdProviderTest.java | 7 +-- 6 files changed, 59 insertions(+), 58 deletions(-) diff --git a/src/main/java/org/cryptomator/cryptofs/CryptoPathMapper.java b/src/main/java/org/cryptomator/cryptofs/CryptoPathMapper.java index 136d697f..8e3b8120 100644 --- a/src/main/java/org/cryptomator/cryptofs/CryptoPathMapper.java +++ b/src/main/java/org/cryptomator/cryptofs/CryptoPathMapper.java @@ -8,11 +8,9 @@ *******************************************************************************/ package org.cryptomator.cryptofs; -import com.google.common.base.Throwables; -import com.google.common.cache.Cache; -import com.google.common.cache.CacheBuilder; -import com.google.common.cache.CacheLoader; -import com.google.common.cache.LoadingCache; +import com.github.benmanes.caffeine.cache.Cache; +import com.github.benmanes.caffeine.cache.Caffeine; +import com.github.benmanes.caffeine.cache.LoadingCache; import com.google.common.io.BaseEncoding; import org.cryptomator.cryptofs.common.CiphertextFileType; import org.cryptomator.cryptofs.common.Constants; @@ -22,6 +20,7 @@ import javax.inject.Inject; import java.io.IOException; +import java.io.UncheckedIOException; import java.nio.charset.StandardCharsets; import java.nio.file.FileAlreadyExistsException; import java.nio.file.Files; @@ -32,7 +31,6 @@ import java.time.Duration; import java.util.Objects; import java.util.Optional; -import java.util.concurrent.ExecutionException; import static org.cryptomator.cryptofs.common.Constants.DATA_DIR_NAME; @@ -61,8 +59,8 @@ public class CryptoPathMapper { this.dirIdProvider = dirIdProvider; this.longFileNameProvider = longFileNameProvider; this.vaultConfig = vaultConfig; - this.ciphertextNames = CacheBuilder.newBuilder().maximumSize(MAX_CACHED_CIPHERTEXT_NAMES).build(CacheLoader.from(this::getCiphertextFileName)); - this.ciphertextDirectories = CacheBuilder.newBuilder().maximumSize(MAX_CACHED_DIR_PATHS).expireAfterWrite(MAX_CACHE_AGE).build(); + this.ciphertextNames = Caffeine.newBuilder().maximumSize(MAX_CACHED_CIPHERTEXT_NAMES).build(this::getCiphertextFileName); + this.ciphertextDirectories = Caffeine.newBuilder().maximumSize(MAX_CACHED_DIR_PATHS).expireAfterWrite(MAX_CACHE_AGE).build(); this.rootDirectory = resolveDirectory(Constants.ROOT_DIR_ID); } @@ -127,7 +125,7 @@ public CiphertextFilePath getCiphertextFilePath(CryptoPath cleartextPath) throws } public CiphertextFilePath getCiphertextFilePath(Path parentCiphertextDir, String parentDirId, String cleartextName) { - String ciphertextName = ciphertextNames.getUnchecked(new DirIdAndName(parentDirId, cleartextName)); + String ciphertextName = ciphertextNames.get(new DirIdAndName(parentDirId, cleartextName)); Path c9rPath = parentCiphertextDir.resolve(ciphertextName); if (ciphertextName.length() > vaultConfig.getShorteningThreshold()) { LongFileNameProvider.DeflatedFileName deflatedFileName = longFileNameProvider.deflate(c9rPath); @@ -160,13 +158,16 @@ public CiphertextDirectory getCiphertextDir(CryptoPath cleartextPath) throws IOE return rootDirectory; } else { try { - return ciphertextDirectories.get(cleartextPath, () -> { - Path dirFile = getCiphertextFilePath(cleartextPath).getDirFilePath(); - return resolveDirectory(dirFile); + return ciphertextDirectories.get(cleartextPath, p -> { + try { + Path dirFile = getCiphertextFilePath(p).getDirFilePath(); + return resolveDirectory(dirFile); + } catch (IOException e) { + throw new UncheckedIOException(e); + } }); - } catch (ExecutionException e) { - Throwables.throwIfInstanceOf(e.getCause(), IOException.class); - throw new IOException("Unexpected exception", e); + } catch (UncheckedIOException e) { + throw new IOException(e); } } } diff --git a/src/main/java/org/cryptomator/cryptofs/DirectoryIdLoader.java b/src/main/java/org/cryptomator/cryptofs/DirectoryIdLoader.java index 627ee94b..c03e85a6 100644 --- a/src/main/java/org/cryptomator/cryptofs/DirectoryIdLoader.java +++ b/src/main/java/org/cryptomator/cryptofs/DirectoryIdLoader.java @@ -1,10 +1,11 @@ package org.cryptomator.cryptofs; -import com.google.common.cache.CacheLoader; +import com.github.benmanes.caffeine.cache.CacheLoader; import javax.inject.Inject; import java.io.IOException; import java.io.InputStream; +import java.io.UncheckedIOException; import java.nio.channels.Channels; import java.nio.channels.FileChannel; import java.nio.charset.StandardCharsets; @@ -14,7 +15,7 @@ import java.util.UUID; @CryptoFileSystemScoped -class DirectoryIdLoader extends CacheLoader { +class DirectoryIdLoader implements CacheLoader { private static final int MAX_DIR_ID_LENGTH = 1000; @@ -23,7 +24,7 @@ public DirectoryIdLoader() { } @Override - public String load(Path dirFilePath) throws IOException { + public String load(Path dirFilePath) throws UncheckedIOException { try (FileChannel ch = FileChannel.open(dirFilePath, StandardOpenOption.READ); InputStream in = Channels.newInputStream(ch)) { long size = ch.size(); @@ -39,6 +40,8 @@ public String load(Path dirFilePath) throws IOException { } } catch (NoSuchFileException e) { return UUID.randomUUID().toString(); + } catch (IOException e) { + throw new UncheckedIOException(e); } } diff --git a/src/main/java/org/cryptomator/cryptofs/DirectoryIdProvider.java b/src/main/java/org/cryptomator/cryptofs/DirectoryIdProvider.java index 94c79432..adfe0829 100644 --- a/src/main/java/org/cryptomator/cryptofs/DirectoryIdProvider.java +++ b/src/main/java/org/cryptomator/cryptofs/DirectoryIdProvider.java @@ -8,14 +8,14 @@ *******************************************************************************/ package org.cryptomator.cryptofs; -import java.io.IOException; -import java.nio.file.Path; -import java.util.concurrent.ExecutionException; +import com.github.benmanes.caffeine.cache.Cache; +import com.github.benmanes.caffeine.cache.Caffeine; +import com.github.benmanes.caffeine.cache.LoadingCache; import javax.inject.Inject; - -import com.google.common.cache.CacheBuilder; -import com.google.common.cache.LoadingCache; +import java.io.IOException; +import java.io.UncheckedIOException; +import java.nio.file.Path; @CryptoFileSystemScoped class DirectoryIdProvider { @@ -26,13 +26,13 @@ class DirectoryIdProvider { @Inject public DirectoryIdProvider(DirectoryIdLoader directoryIdLoader) { - ids = CacheBuilder.newBuilder().maximumSize(MAX_CACHE_SIZE).build(directoryIdLoader); + this.ids = Caffeine.newBuilder().maximumSize(MAX_CACHE_SIZE).build(directoryIdLoader); } public String load(Path dirFilePath) throws IOException { try { return ids.get(dirFilePath); - } catch (ExecutionException e) { + } catch (UncheckedIOException e) { throw new IOException("Failed to load contents of directory file at path " + dirFilePath, e); } } @@ -62,4 +62,5 @@ public void move(Path srcDirFilePath, Path dstDirFilePath) { } } + } diff --git a/src/main/java/org/cryptomator/cryptofs/LongFileNameProvider.java b/src/main/java/org/cryptomator/cryptofs/LongFileNameProvider.java index 94d32b68..8cfd6931 100644 --- a/src/main/java/org/cryptomator/cryptofs/LongFileNameProvider.java +++ b/src/main/java/org/cryptomator/cryptofs/LongFileNameProvider.java @@ -8,10 +8,9 @@ *******************************************************************************/ package org.cryptomator.cryptofs; +import com.github.benmanes.caffeine.cache.Cache; +import com.github.benmanes.caffeine.cache.Caffeine; import com.google.common.base.Throwables; -import com.google.common.cache.CacheBuilder; -import com.google.common.cache.CacheLoader; -import com.google.common.cache.LoadingCache; import com.google.common.io.BaseEncoding; import org.cryptomator.cryptolib.common.MessageDigestSupplier; @@ -24,7 +23,6 @@ import java.nio.file.Path; import java.nio.file.StandardOpenOption; import java.time.Duration; -import java.util.concurrent.ExecutionException; import static java.nio.charset.StandardCharsets.UTF_8; import static org.cryptomator.cryptofs.common.Constants.DEFLATED_FILE_SUFFIX; @@ -39,31 +37,28 @@ public class LongFileNameProvider { private static final Duration MAX_CACHE_AGE = Duration.ofMinutes(1); private final ReadonlyFlag readonlyFlag; - private final LoadingCache longNames; // Maps from c9s paths to inflated filenames + private final Cache longNames; // Maps from c9s paths to inflated filenames @Inject public LongFileNameProvider(ReadonlyFlag readonlyFlag) { this.readonlyFlag = readonlyFlag; - this.longNames = CacheBuilder.newBuilder().expireAfterAccess(MAX_CACHE_AGE).build(new Loader()); + this.longNames = Caffeine.newBuilder().expireAfterAccess(MAX_CACHE_AGE).build(); } - private class Loader extends CacheLoader { - - @Override - public String load(Path c9sPath) throws IOException { - Path longNameFile = c9sPath.resolve(INFLATED_FILE_NAME); - try (SeekableByteChannel ch = Files.newByteChannel(longNameFile, StandardOpenOption.READ)) { - if (ch.size() > MAX_FILENAME_BUFFER_SIZE) { - throw new IOException("Unexpectedly large file: " + longNameFile); - } - assert ch.size() <= MAX_FILENAME_BUFFER_SIZE; - ByteBuffer buf = ByteBuffer.allocate((int) ch.size()); - ch.read(buf); - buf.flip(); - return UTF_8.decode(buf).toString(); + private String load(Path c9sPath) throws UncheckedIOException { + Path longNameFile = c9sPath.resolve(INFLATED_FILE_NAME); + try (SeekableByteChannel ch = Files.newByteChannel(longNameFile, StandardOpenOption.READ)) { + if (ch.size() > MAX_FILENAME_BUFFER_SIZE) { + throw new IOException("Unexpectedly large file: " + longNameFile); } + assert ch.size() <= MAX_FILENAME_BUFFER_SIZE; + ByteBuffer buf = ByteBuffer.allocate((int) ch.size()); + ch.read(buf); + buf.flip(); + return UTF_8.decode(buf).toString(); + } catch (IOException e) { + throw new UncheckedIOException(e); } - } public boolean isDeflated(String possiblyDeflatedFileName) { @@ -72,10 +67,9 @@ public boolean isDeflated(String possiblyDeflatedFileName) { public String inflate(Path c9sPath) throws IOException { try { - return longNames.get(c9sPath); - } catch (ExecutionException e) { - Throwables.throwIfInstanceOf(e.getCause(), IOException.class); - throw new IllegalStateException("Unexpected exception", e); + return longNames.get(c9sPath, this::load); + } catch (UncheckedIOException e) { + throw e.getCause(); // rethrow original to keep exception types such as NoSuchFileException } } diff --git a/src/test/java/org/cryptomator/cryptofs/DirectoryIdLoaderTest.java b/src/test/java/org/cryptomator/cryptofs/DirectoryIdLoaderTest.java index 3943207a..6141325f 100644 --- a/src/test/java/org/cryptomator/cryptofs/DirectoryIdLoaderTest.java +++ b/src/test/java/org/cryptomator/cryptofs/DirectoryIdLoaderTest.java @@ -7,6 +7,7 @@ import org.mockito.Mockito; import java.io.IOException; +import java.io.UncheckedIOException; import java.nio.ByteBuffer; import java.nio.channels.FileChannel; import java.nio.file.FileSystem; @@ -83,10 +84,10 @@ public void testIOExceptionWhenExistingFileIsEmpty() throws IOException { when(provider.newFileChannel(eq(dirFilePath), any())).thenReturn(channel); when(channel.size()).thenReturn(0l); - IOException e = Assertions.assertThrows(IOException.class, () -> { + UncheckedIOException e = Assertions.assertThrows(UncheckedIOException.class, () -> { inTest.load(dirFilePath); }); - MatcherAssert.assertThat(e.getMessage(), containsString("Invalid, empty directory file")); + MatcherAssert.assertThat(e.getCause().getMessage(), containsString("Invalid, empty directory file")); } @Test @@ -95,10 +96,10 @@ public void testIOExceptionWhenExistingFileIsTooLarge() throws IOException { when(provider.newFileChannel(eq(dirFilePath), any())).thenReturn(channel); when(channel.size()).thenReturn((long) Integer.MAX_VALUE); - IOException e = Assertions.assertThrows(IOException.class, () -> { + UncheckedIOException e = Assertions.assertThrows(UncheckedIOException.class, () -> { inTest.load(dirFilePath); }); - MatcherAssert.assertThat(e.getMessage(), containsString("Unexpectedly large directory file")); + MatcherAssert.assertThat(e.getCause().getMessage(), containsString("Unexpectedly large directory file")); } } diff --git a/src/test/java/org/cryptomator/cryptofs/DirectoryIdProviderTest.java b/src/test/java/org/cryptomator/cryptofs/DirectoryIdProviderTest.java index 8525403c..33c6d04d 100644 --- a/src/test/java/org/cryptomator/cryptofs/DirectoryIdProviderTest.java +++ b/src/test/java/org/cryptomator/cryptofs/DirectoryIdProviderTest.java @@ -9,6 +9,7 @@ import org.mockito.verification.VerificationMode; import java.io.IOException; +import java.io.UncheckedIOException; import java.nio.file.Path; import java.util.concurrent.ExecutionException; @@ -38,14 +39,14 @@ public void testLoadInvokesLoader() throws IOException { } @Test - public void testIOExceptionFromLoaderIsWrappedAndRethrown() throws IOException { + public void testIOExceptionFromLoaderIsWrappedAndRethrown() { IOException originalIoException = new IOException(); - when(loader.load(aPath)).thenThrow(originalIoException); + when(loader.load(aPath)).thenThrow(new UncheckedIOException(originalIoException)); IOException e = Assertions.assertThrows(IOException.class, () -> { inTest.load(aPath); }); - Assertions.assertTrue(e.getCause() instanceof ExecutionException); + Assertions.assertTrue(e.getCause() instanceof UncheckedIOException); Assertions.assertEquals(originalIoException, e.getCause().getCause()); } From b6f15b330bcefaa72b3f558a98f9c5bf98157181 Mon Sep 17 00:00:00 2001 From: Armin Schrenk Date: Tue, 4 Apr 2023 12:02:55 +0200 Subject: [PATCH 24/24] prepare 2.6.3 --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 949e3af9..35a0e9df 100644 --- a/pom.xml +++ b/pom.xml @@ -2,7 +2,7 @@ 4.0.0 org.cryptomator cryptofs - 2.7.0-SNAPSHOT + 2.6.3 Cryptomator Crypto Filesystem This library provides the Java filesystem provider used by Cryptomator. https://github.com/cryptomator/cryptofs