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 3ee1c0080..000000000 --- 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 a22b0fa4d..2447c10a6 100644 --- a/src/main/java/org/cryptomator/cryptofs/ch/ChannelComponent.java +++ b/src/main/java/org/cryptomator/cryptofs/ch/ChannelComponent.java @@ -5,6 +5,7 @@ import org.cryptomator.cryptofs.EffectiveOpenOptions; import java.nio.channels.FileChannel; +import java.util.function.Consumer; @ChannelScoped @Subcomponent @@ -17,7 +18,7 @@ interface Factory { ChannelComponent create(@BindsInstance FileChannel ciphertextChannel, // @BindsInstance EffectiveOpenOptions options, // - @BindsInstance ChannelCloseListener listener); // + @BindsInstance Consumer closeListener); // } } diff --git a/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java b/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java index 44d165a8b..f40a34d72 100644 --- a/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java +++ b/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java @@ -34,6 +34,7 @@ import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; import java.util.concurrent.locks.ReadWriteLock; +import java.util.function.Consumer; import static java.lang.Math.max; import static java.lang.Math.min; @@ -53,11 +54,11 @@ public class CleartextFileChannel extends AbstractFileChannel { private final AtomicLong fileSize; private final AtomicReference lastModified; private final ExceptionsDuringWrite exceptionsDuringWrite; - private final ChannelCloseListener closeListener; + private final Consumer 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, ChunkCache chunkCache, BufferPool bufferPool, EffectiveOpenOptions options, @OpenFileSize AtomicLong fileSize, @OpenFileModifiedDate AtomicReference lastModified, @CurrentOpenFilePath AtomicReference currentPath, ExceptionsDuringWrite exceptionsDuringWrite, Consumer closeListener, CryptoFileSystemStats stats) { super(readWriteLock); this.ciphertextFileChannel = ciphertextFileChannel; this.fileHeaderHolder = fileHeaderHolder; @@ -327,7 +328,7 @@ long beginOfChunk(long cleartextPos) { protected void implCloseChannel() throws IOException { var closeActions = List.of(this::flush, // super::implCloseChannel, // - () -> closeListener.closed(this), // + () -> closeListener.accept(ciphertextFileChannel), ciphertextFileChannel::close, // this::tryPersistLastModified); tryAll(closeActions.iterator()); diff --git a/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java b/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java index 14e5d072e..7ed4b4316 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,7 @@ 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.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; @@ -42,10 +33,13 @@ public class OpenCryptoFile implements Closeable { private final AtomicReference currentFilePath; private final AtomicLong fileSize; private final OpenCryptoFileComponent component; - private final ConcurrentMap openChannels = new ConcurrentHashMap<>(); + + private final AtomicInteger openChannelsCount = new AtomicInteger(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, ChunkIO chunkIO, // + @CurrentOpenFilePath AtomicReference currentFilePath, @OpenFileSize AtomicLong fileSize, // + @OpenFileModifiedDate AtomicReference lastModified, OpenCryptoFileComponent component) { this.listener = listener; this.chunkCache = chunkCache; this.cryptor = cryptor; @@ -71,6 +65,8 @@ public synchronized FileChannel newFileChannel(EffectiveOpenOptions options, Fil } FileChannel ciphertextFileChannel = null; CleartextFileChannel cleartextFileChannel = null; + + openChannelsCount.incrementAndGet(); // synchronized context, hence we can proactively increase the number try { ciphertextFileChannel = path.getFileSystem().provider().newFileChannel(path, options.createOpenOptionsForEncryptedFile(), attrs); initFileHeader(options, ciphertextFileChannel); @@ -81,20 +77,16 @@ public synchronized FileChannel newFileChannel(EffectiveOpenOptions options, Fil } initFileSize(ciphertextFileChannel); cleartextFileChannel = component.newChannelComponent() // - .create(ciphertextFileChannel, options, this::channelClosed) // + .create(ciphertextFileChannel, options, this::cleartextChannelClosed) // .channel(); } finally { if (cleartextFileChannel == null) { // i.e. something didn't work + cleartextChannelClosed(ciphertextFileChannel); closeQuietly(ciphertextFileChannel); - // is this the first file channel to be opened? - if (openChannels.isEmpty()) { - close(); // then also close the file again. - } } } assert cleartextFileChannel != null; // otherwise there would have been an exception - openChannels.put(cleartextFileChannel, ciphertextFileChannel); chunkIO.registerChannel(ciphertextFileChannel, options.writable()); return cleartextFileChannel; } @@ -183,12 +175,11 @@ public void updateCurrentFilePath(Path newFilePath) { currentFilePath.updateAndGet(p -> p == null ? null : newFilePath); } - private synchronized void channelClosed(CleartextFileChannel cleartextFileChannel) { - FileChannel ciphertextFileChannel = openChannels.remove(cleartextFileChannel); + private synchronized void cleartextChannelClosed(FileChannel ciphertextFileChannel) { if (ciphertextFileChannel != null) { chunkIO.unregisterChannel(ciphertextFileChannel); } - if (openChannels.isEmpty()) { + if (openChannelsCount.decrementAndGet() == 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 a19e508df..e203b16b0 100644 --- a/src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java +++ b/src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java @@ -43,6 +43,7 @@ import java.util.concurrent.atomic.AtomicReference; import java.util.concurrent.locks.Lock; import java.util.concurrent.locks.ReadWriteLock; +import java.util.function.Consumer; import static org.hamcrest.CoreMatchers.is; import static org.mockito.ArgumentMatchers.any; @@ -76,7 +77,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 Consumer closeListener = mock(Consumer.class); private CryptoFileSystemStats stats = mock(CryptoFileSystemStats.class); private CleartextFileChannel inTest; @@ -242,11 +243,22 @@ public void testCloseIoExceptionFlush() throws IOException { Assertions.assertThrows(IOException.class, () -> inSpy.implCloseChannel()); - verify(closeListener).closed(inSpy); + verify(closeListener).accept(ciphertextFileChannel); verify(ciphertextFileChannel).close(); 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, closeListener); + ordering.verify(inSpy).flush(); + verify(closeListener).accept(ciphertextFileChannel); + } + @Test @DisplayName("On close, first close channel, then persist lastModified") public void testCloseCipherChannelCloseBeforePersist() throws IOException { @@ -278,8 +290,8 @@ public void testCloseExceptionOnLastModifiedPersistenceIgnored() throws IOExcept var inSpy = Mockito.spy(inTest); Mockito.doThrow(IOException.class).when(inSpy).persistLastModified(); - Assertions.assertDoesNotThrow(() -> inSpy.implCloseChannel()); - verify(closeListener).closed(inSpy); + Assertions.assertDoesNotThrow(inSpy::implCloseChannel); + verify(closeListener).accept(ciphertextFileChannel); verify(ciphertextFileChannel).close(); } diff --git a/src/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFileTest.java b/src/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFileTest.java index 3de6a2f91..7f90893fb 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; @@ -35,6 +34,7 @@ import java.util.EnumSet; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Consumer; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; @@ -191,7 +191,7 @@ public class FileChannelFactoryTest { private final AtomicLong realFileSize = new AtomicLong(-1L); private OpenCryptoFile openCryptoFile; private CleartextFileChannel cleartextFileChannel; - private AtomicReference listener; + private AtomicReference> listener; private AtomicReference ciphertextChannel; @BeforeAll @@ -280,7 +280,7 @@ public void triggerCloseListener() throws IOException { Assumptions.assumeTrue(listener.get() != null); Assumptions.assumeTrue(ciphertextChannel.get() != null); - listener.get().closed(cleartextFileChannel); + listener.get().accept(ciphertextChannel.get()); verify(chunkIO).unregisterChannel(ciphertextChannel.get()); }