diff --git a/src/main/java/org/cryptomator/cryptofs/ch/ChannelCloseListener.java b/src/main/java/org/cryptomator/cryptofs/ch/ChannelCloseListener.java deleted file mode 100644 index 3ee1c008..00000000 --- a/src/main/java/org/cryptomator/cryptofs/ch/ChannelCloseListener.java +++ /dev/null @@ -1,9 +0,0 @@ -package org.cryptomator.cryptofs.ch; - - -@FunctionalInterface -public interface ChannelCloseListener { - - void closed(CleartextFileChannel channel); - -} \ No newline at end of file diff --git a/src/main/java/org/cryptomator/cryptofs/ch/ChannelComponent.java b/src/main/java/org/cryptomator/cryptofs/ch/ChannelComponent.java index a22b0fa4..1199227d 100644 --- a/src/main/java/org/cryptomator/cryptofs/ch/ChannelComponent.java +++ b/src/main/java/org/cryptomator/cryptofs/ch/ChannelComponent.java @@ -17,7 +17,7 @@ interface Factory { ChannelComponent create(@BindsInstance FileChannel ciphertextChannel, // @BindsInstance EffectiveOpenOptions options, // - @BindsInstance ChannelCloseListener listener); // + @BindsInstance Runnable closeListener); // } } diff --git a/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java b/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java index 44d165a8..e752ff93 100644 --- a/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java +++ b/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java @@ -7,6 +7,7 @@ import org.cryptomator.cryptofs.fh.BufferPool; import org.cryptomator.cryptofs.fh.Chunk; import org.cryptomator.cryptofs.fh.ChunkCache; +import org.cryptomator.cryptofs.fh.ChunkIO; import org.cryptomator.cryptofs.fh.CurrentOpenFilePath; import org.cryptomator.cryptofs.fh.ExceptionsDuringWrite; import org.cryptomator.cryptofs.fh.FileHeaderHolder; @@ -46,6 +47,7 @@ public class CleartextFileChannel extends AbstractFileChannel { private final FileChannel ciphertextFileChannel; private final FileHeaderHolder fileHeaderHolder; private final Cryptor cryptor; + private final ChunkIO chunkIO; private final ChunkCache chunkCache; private final BufferPool bufferPool; private final EffectiveOpenOptions options; @@ -53,15 +55,16 @@ public class CleartextFileChannel extends AbstractFileChannel { private final AtomicLong fileSize; private final AtomicReference lastModified; private final ExceptionsDuringWrite exceptionsDuringWrite; - private final ChannelCloseListener closeListener; + private final Runnable closeListener; private final CryptoFileSystemStats stats; @Inject - public CleartextFileChannel(FileChannel ciphertextFileChannel, FileHeaderHolder fileHeaderHolder, ReadWriteLock readWriteLock, Cryptor cryptor, ChunkCache chunkCache, BufferPool bufferPool, EffectiveOpenOptions options, @OpenFileSize AtomicLong fileSize, @OpenFileModifiedDate AtomicReference lastModified, @CurrentOpenFilePath AtomicReference currentPath, ExceptionsDuringWrite exceptionsDuringWrite, ChannelCloseListener closeListener, CryptoFileSystemStats stats) { + public CleartextFileChannel(FileChannel ciphertextFileChannel, FileHeaderHolder fileHeaderHolder, ReadWriteLock readWriteLock, Cryptor cryptor, ChunkIO chunkIO, ChunkCache chunkCache, BufferPool bufferPool, EffectiveOpenOptions options, @OpenFileSize AtomicLong fileSize, @OpenFileModifiedDate AtomicReference lastModified, @CurrentOpenFilePath AtomicReference currentPath, ExceptionsDuringWrite exceptionsDuringWrite, Runnable closeListener, CryptoFileSystemStats stats) { super(readWriteLock); this.ciphertextFileChannel = ciphertextFileChannel; this.fileHeaderHolder = fileHeaderHolder; this.cryptor = cryptor; + this.chunkIO = chunkIO; this.chunkCache = chunkCache; this.bufferPool = bufferPool; this.options = options; @@ -77,6 +80,7 @@ public CleartextFileChannel(FileChannel ciphertextFileChannel, FileHeaderHolder if (options.createNew() || options.create()) { lastModified.compareAndSet(Instant.EPOCH, Instant.now()); } + chunkIO.registerChannel(ciphertextFileChannel, options.writable()); } @Override @@ -327,7 +331,8 @@ long beginOfChunk(long cleartextPos) { protected void implCloseChannel() throws IOException { var closeActions = List.of(this::flush, // super::implCloseChannel, // - () -> closeListener.closed(this), // + () -> chunkIO.unregisterChannel(ciphertextFileChannel), // _after_ flush, since the flushed chunk cache uses chunkIO file channels + closeListener::run, // ciphertextFileChannel::close, // this::tryPersistLastModified); tryAll(closeActions.iterator()); diff --git a/src/main/java/org/cryptomator/cryptofs/fh/ChunkIO.java b/src/main/java/org/cryptomator/cryptofs/fh/ChunkIO.java index e1e73c63..858e8e54 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/ChunkIO.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/ChunkIO.java @@ -11,7 +11,7 @@ import java.util.concurrent.ConcurrentHashMap; @OpenFileScoped -class ChunkIO { +public class ChunkIO { private final Set readableChannels = ConcurrentHashMap.newKeySet(); private final Set writableChannels = ConcurrentHashMap.newKeySet(); diff --git a/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java b/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java index 14e5d072..f3a5224a 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java @@ -1,11 +1,3 @@ -/******************************************************************************* - * Copyright (c) 2016 Sebastian Stenzel and others. - * All rights reserved. This program and the accompanying materials - * are made available under the terms of the accompanying LICENSE.txt. - * - * Contributors: - * Sebastian Stenzel - initial API and implementation - *******************************************************************************/ package org.cryptomator.cryptofs.fh; import org.cryptomator.cryptofs.EffectiveOpenOptions; @@ -23,8 +15,6 @@ import java.nio.file.attribute.FileTime; import java.time.Instant; import java.util.Optional; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentMap; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; @@ -38,19 +28,18 @@ public class OpenCryptoFile implements Closeable { private final ChunkCache chunkCache; private final Cryptor cryptor; private final FileHeaderHolder headerHolder; - private final ChunkIO chunkIO; private final AtomicReference currentFilePath; private final AtomicLong fileSize; private final OpenCryptoFileComponent component; - private final ConcurrentMap openChannels = new ConcurrentHashMap<>(); + + private volatile int openChannelsCount = 0; @Inject - public OpenCryptoFile(FileCloseListener listener, ChunkCache chunkCache, Cryptor cryptor, FileHeaderHolder headerHolder, ChunkIO chunkIO, @CurrentOpenFilePath AtomicReference currentFilePath, @OpenFileSize AtomicLong fileSize, @OpenFileModifiedDate AtomicReference lastModified, OpenCryptoFileComponent component) { + public OpenCryptoFile(FileCloseListener listener, ChunkCache chunkCache, Cryptor cryptor, FileHeaderHolder headerHolder, @CurrentOpenFilePath AtomicReference currentFilePath, @OpenFileSize AtomicLong fileSize, @OpenFileModifiedDate AtomicReference lastModified, OpenCryptoFileComponent component) { this.listener = listener; this.chunkCache = chunkCache; this.cryptor = cryptor; this.headerHolder = headerHolder; - this.chunkIO = chunkIO; this.currentFilePath = currentFilePath; this.fileSize = fileSize; this.component = component; @@ -71,6 +60,8 @@ public synchronized FileChannel newFileChannel(EffectiveOpenOptions options, Fil } FileChannel ciphertextFileChannel = null; CleartextFileChannel cleartextFileChannel = null; + + openChannelsCount = openChannelsCount + 1; // synchronized context, hence we can proactively increase the number try { ciphertextFileChannel = path.getFileSystem().provider().newFileChannel(path, options.createOpenOptionsForEncryptedFile(), attrs); initFileHeader(options, ciphertextFileChannel); @@ -86,16 +77,11 @@ public synchronized FileChannel newFileChannel(EffectiveOpenOptions options, Fil } finally { if (cleartextFileChannel == null) { // i.e. something didn't work closeQuietly(ciphertextFileChannel); - // is this the first file channel to be opened? - if (openChannels.isEmpty()) { - close(); // then also close the file again. - } + channelClosed(); // if first channel and it fails, close this again } } assert cleartextFileChannel != null; // otherwise there would have been an exception - openChannels.put(cleartextFileChannel, ciphertextFileChannel); - chunkIO.registerChannel(ciphertextFileChannel, options.writable()); return cleartextFileChannel; } @@ -183,12 +169,9 @@ public void updateCurrentFilePath(Path newFilePath) { currentFilePath.updateAndGet(p -> p == null ? null : newFilePath); } - private synchronized void channelClosed(CleartextFileChannel cleartextFileChannel) { - FileChannel ciphertextFileChannel = openChannels.remove(cleartextFileChannel); - if (ciphertextFileChannel != null) { - chunkIO.unregisterChannel(ciphertextFileChannel); - } - if (openChannels.isEmpty()) { + private synchronized void channelClosed() { + openChannelsCount = openChannelsCount - 1; + if (openChannelsCount == 0) { close(); } } diff --git a/src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java b/src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java index a19e508d..698c3541 100644 --- a/src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java +++ b/src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java @@ -5,6 +5,7 @@ import org.cryptomator.cryptofs.fh.BufferPool; import org.cryptomator.cryptofs.fh.Chunk; import org.cryptomator.cryptofs.fh.ChunkCache; +import org.cryptomator.cryptofs.fh.ChunkIO; import org.cryptomator.cryptofs.fh.ExceptionsDuringWrite; import org.cryptomator.cryptofs.fh.FileHeaderHolder; import org.cryptomator.cryptolib.api.Cryptor; @@ -59,6 +60,7 @@ public class CleartextFileChannelTest { private ChunkCache chunkCache = mock(ChunkCache.class); + private ChunkIO chunkIO = mock(ChunkIO.class); private BufferPool bufferPool = mock(BufferPool.class); private ReadWriteLock readWriteLock = mock(ReadWriteLock.class); private Lock readLock = mock(Lock.class); @@ -76,7 +78,7 @@ public class CleartextFileChannelTest { private AtomicReference lastModified = new AtomicReference<>(Instant.ofEpochMilli(0)); private BasicFileAttributeView attributeView = mock(BasicFileAttributeView.class); private ExceptionsDuringWrite exceptionsDuringWrite = mock(ExceptionsDuringWrite.class); - private ChannelCloseListener closeListener = mock(ChannelCloseListener.class); + private Runnable closeListener = mock(Runnable.class); private CryptoFileSystemStats stats = mock(CryptoFileSystemStats.class); private CleartextFileChannel inTest; @@ -87,6 +89,8 @@ public void setUp() throws IOException { 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, () -> {})); + doNothing().when(chunkIO).registerChannel(Mockito.eq(ciphertextFileChannel), Mockito.anyBoolean()); + doNothing().when(chunkIO).unregisterChannel(ciphertextFileChannel); when(bufferPool.getCleartextBuffer()).thenAnswer(invocation -> ByteBuffer.allocate(100)); when(fileHeaderCryptor.headerSize()).thenReturn(50); when(headerHolder.headerIsPersisted()).thenReturn(headerIsPersisted); @@ -101,7 +105,7 @@ public void setUp() throws IOException { when(readWriteLock.readLock()).thenReturn(readLock); when(readWriteLock.writeLock()).thenReturn(writeLock); - inTest = new CleartextFileChannel(ciphertextFileChannel, headerHolder, readWriteLock, cryptor, chunkCache, bufferPool, options, fileSize, lastModified, currentFilePath, exceptionsDuringWrite, closeListener, stats); + inTest = new CleartextFileChannel(ciphertextFileChannel, headerHolder, readWriteLock, cryptor, chunkIO, chunkCache, bufferPool, options, fileSize, lastModified, currentFilePath, exceptionsDuringWrite, closeListener, stats); } @Test @@ -242,11 +246,23 @@ public void testCloseIoExceptionFlush() throws IOException { Assertions.assertThrows(IOException.class, () -> inSpy.implCloseChannel()); - verify(closeListener).closed(inSpy); + verify(closeListener).run(); verify(ciphertextFileChannel).close(); + verify(chunkIO).unregisterChannel(ciphertextFileChannel); verify(inSpy).persistLastModified(); } + @Test + @DisplayName("On close, first flush channel, then unregister") + public void testCloseCipherChannelFlushBeforeUnregister() throws IOException { + var inSpy = spy(inTest); + inSpy.implCloseChannel(); + + var ordering = inOrder(inSpy, chunkIO); + ordering.verify(inSpy).flush(); + ordering.verify(chunkIO).unregisterChannel(ciphertextFileChannel); + } + @Test @DisplayName("On close, first close channel, then persist lastModified") public void testCloseCipherChannelCloseBeforePersist() throws IOException { @@ -279,7 +295,8 @@ public void testCloseExceptionOnLastModifiedPersistenceIgnored() throws IOExcept Mockito.doThrow(IOException.class).when(inSpy).persistLastModified(); Assertions.assertDoesNotThrow(() -> inSpy.implCloseChannel()); - verify(closeListener).closed(inSpy); + verify(chunkIO).unregisterChannel(ciphertextFileChannel); + verify(closeListener).run(); verify(ciphertextFileChannel).close(); } @@ -385,7 +402,7 @@ public void testReadFromMultipleChunks() throws IOException { fileSize.set(5_000_000_100l); // initial cleartext size will be 5_000_000_100l when(options.readable()).thenReturn(true); - inTest = new CleartextFileChannel(ciphertextFileChannel, headerHolder, readWriteLock, cryptor, chunkCache, bufferPool, options, fileSize, lastModified, currentFilePath, exceptionsDuringWrite, closeListener, stats); + inTest = new CleartextFileChannel(ciphertextFileChannel, headerHolder, readWriteLock, cryptor, chunkIO, chunkCache, bufferPool, options, fileSize, lastModified, currentFilePath, exceptionsDuringWrite, closeListener, stats); ByteBuffer buf = ByteBuffer.allocate(10); // A read from frist chunk: @@ -557,7 +574,7 @@ public void testWriteHeaderFailsResetsPersistenceState() throws IOException { public void testDontRewriteHeader() throws IOException { when(options.writable()).thenReturn(true); when(headerIsPersisted.get()).thenReturn(true); - inTest = new CleartextFileChannel(ciphertextFileChannel, headerHolder, readWriteLock, cryptor, chunkCache, bufferPool, options, fileSize, lastModified, currentFilePath, exceptionsDuringWrite, closeListener, stats); + inTest = new CleartextFileChannel(ciphertextFileChannel, headerHolder, readWriteLock, cryptor, chunkIO, chunkCache, bufferPool, options, fileSize, lastModified, currentFilePath, exceptionsDuringWrite, closeListener, stats); inTest.force(true); diff --git a/src/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFileTest.java b/src/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFileTest.java index 3de6a2f9..d071dfb9 100644 --- a/src/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFileTest.java +++ b/src/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFileTest.java @@ -4,7 +4,6 @@ import com.google.common.jimfs.Jimfs; import org.cryptomator.cryptofs.EffectiveOpenOptions; import org.cryptomator.cryptofs.ReadonlyFlag; -import org.cryptomator.cryptofs.ch.ChannelCloseListener; import org.cryptomator.cryptofs.ch.ChannelComponent; import org.cryptomator.cryptofs.ch.CleartextFileChannel; import org.cryptomator.cryptolib.api.Cryptor; @@ -12,7 +11,6 @@ import org.cryptomator.cryptolib.api.FileHeaderCryptor; import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.Assertions; -import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.MethodOrderer; @@ -52,7 +50,6 @@ public class OpenCryptoFileTest { private Cryptor cryptor = mock(Cryptor.class); private FileHeaderCryptor fileHeaderCryptor = mock(FileHeaderCryptor.class); private FileHeaderHolder headerHolder = mock(FileHeaderHolder.class); - private ChunkIO chunkIO = mock(ChunkIO.class); private AtomicLong fileSize = Mockito.mock(AtomicLong.class); private AtomicReference lastModified = new AtomicReference(Instant.ofEpochMilli(0)); private OpenCryptoFileComponent openCryptoFileComponent = mock(OpenCryptoFileComponent.class); @@ -72,7 +69,7 @@ public static void tearDown() throws IOException { @Test public void testCloseTriggersCloseListener() { - OpenCryptoFile openCryptoFile = new OpenCryptoFile(closeListener, chunkCache, cryptor, headerHolder, chunkIO, CURRENT_FILE_PATH, fileSize, lastModified, openCryptoFileComponent); + OpenCryptoFile openCryptoFile = new OpenCryptoFile(closeListener, chunkCache, cryptor, headerHolder, CURRENT_FILE_PATH, fileSize, lastModified, openCryptoFileComponent); openCryptoFile.close(); verify(closeListener).close(CURRENT_FILE_PATH.get(), openCryptoFile); } @@ -83,7 +80,7 @@ public void testCloseImmediatelyIfOpeningFirstChannelFails() { UncheckedIOException expectedException = new UncheckedIOException(new IOException("fail!")); EffectiveOpenOptions options = Mockito.mock(EffectiveOpenOptions.class); Mockito.when(options.createOpenOptionsForEncryptedFile()).thenThrow(expectedException); - OpenCryptoFile openCryptoFile = new OpenCryptoFile(closeListener, chunkCache, cryptor, headerHolder, chunkIO, CURRENT_FILE_PATH, fileSize, lastModified, openCryptoFileComponent); + OpenCryptoFile openCryptoFile = new OpenCryptoFile(closeListener, chunkCache, cryptor, headerHolder, CURRENT_FILE_PATH, fileSize, lastModified, openCryptoFileComponent); UncheckedIOException exception = Assertions.assertThrows(UncheckedIOException.class, () -> { openCryptoFile.newFileChannel(options); @@ -102,7 +99,7 @@ public void testFileSizeZerodOnTruncateExisting() throws IOException { Mockito.when(openCryptoFileComponent.newChannelComponent()).thenReturn(channelComponentFactory); Mockito.when(channelComponentFactory.create(any(), any(), any())).thenReturn(channelComponent); Mockito.when(channelComponent.channel()).thenReturn(mock(CleartextFileChannel.class)); - OpenCryptoFile openCryptoFile = new OpenCryptoFile(closeListener, chunkCache, cryptor, headerHolder, chunkIO, CURRENT_FILE_PATH, fileSize, lastModified, openCryptoFileComponent); + OpenCryptoFile openCryptoFile = new OpenCryptoFile(closeListener, chunkCache, cryptor, headerHolder, CURRENT_FILE_PATH, fileSize, lastModified, openCryptoFileComponent); openCryptoFile.newFileChannel(options); verify(fileSize).set(0L); @@ -114,7 +111,7 @@ public class InitFilHeaderTests { EffectiveOpenOptions options = Mockito.mock(EffectiveOpenOptions.class); FileChannel cipherFileChannel = Mockito.mock(FileChannel.class, "cipherFilechannel"); - OpenCryptoFile inTest = new OpenCryptoFile(closeListener, chunkCache, cryptor, headerHolder, chunkIO, CURRENT_FILE_PATH, fileSize, lastModified, openCryptoFileComponent); + OpenCryptoFile inTest = new OpenCryptoFile(closeListener, chunkCache, cryptor, headerHolder, CURRENT_FILE_PATH, fileSize, lastModified, openCryptoFileComponent); @Test @DisplayName("Skip file header init, if the file header already exists in memory") @@ -191,22 +188,19 @@ public class FileChannelFactoryTest { private final AtomicLong realFileSize = new AtomicLong(-1L); private OpenCryptoFile openCryptoFile; private CleartextFileChannel cleartextFileChannel; - private AtomicReference listener; private AtomicReference ciphertextChannel; @BeforeAll public void setup() throws IOException { FS = Jimfs.newFileSystem("OpenCryptoFileTest.FileChannelFactoryTest", Configuration.unix().toBuilder().setAttributeViews("basic", "posix").build()); CURRENT_FILE_PATH = new AtomicReference<>(FS.getPath("currentFile")); - openCryptoFile = new OpenCryptoFile(closeListener, chunkCache, cryptor, headerHolder, chunkIO, CURRENT_FILE_PATH, realFileSize, lastModified, openCryptoFileComponent); + openCryptoFile = new OpenCryptoFile(closeListener, chunkCache, cryptor, headerHolder, CURRENT_FILE_PATH, realFileSize, lastModified, openCryptoFileComponent); cleartextFileChannel = mock(CleartextFileChannel.class); - listener = new AtomicReference<>(); ciphertextChannel = new AtomicReference<>(); Mockito.when(openCryptoFileComponent.newChannelComponent()).thenReturn(channelComponentFactory); Mockito.when(channelComponentFactory.create(Mockito.any(), Mockito.any(), Mockito.any())).thenAnswer(invocation -> { ciphertextChannel.set(invocation.getArgument(0)); - listener.set(invocation.getArgument(2)); return channelComponent; }); Mockito.when(channelComponent.channel()).thenReturn(cleartextFileChannel); @@ -227,7 +221,6 @@ public void createFileChannel() throws IOException { EffectiveOpenOptions options = EffectiveOpenOptions.from(EnumSet.of(StandardOpenOption.CREATE_NEW, StandardOpenOption.WRITE), readonlyFlag); FileChannel ch = openCryptoFile.newFileChannel(options, attrs); Assertions.assertSame(cleartextFileChannel, ch); - verify(chunkIO).registerChannel(ciphertextChannel.get(), true); } @Test @@ -273,17 +266,6 @@ public void testTruncateExistingInvalidatesChunkCache() throws IOException { verify(chunkCache).invalidateStale(); } - @Test - @Order(100) - @DisplayName("closeListener triggers chunkIO.unregisterChannel()") - public void triggerCloseListener() throws IOException { - Assumptions.assumeTrue(listener.get() != null); - Assumptions.assumeTrue(ciphertextChannel.get() != null); - - listener.get().closed(cleartextFileChannel); - verify(chunkIO).unregisterChannel(ciphertextChannel.get()); - } - } }