Refactoring: Simplify OpenCryptoFile

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); //
 	}
 
 }

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,22 +47,24 @@ 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;
 	private final AtomicReference<Path> currentFilePath;
 	private final AtomicLong fileSize;
 	private final AtomicReference<Instant> 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<Instant> lastModified, @CurrentOpenFilePath AtomicReference<Path> 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<Instant> lastModified, @CurrentOpenFilePath AtomicReference<Path> 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.<CloseAction>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());

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<FileChannel> readableChannels = ConcurrentHashMap.newKeySet();
 	private final Set<FileChannel> writableChannels = ConcurrentHashMap.newKeySet();

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<Path> currentFilePath;
 	private final AtomicLong fileSize;
 	private final OpenCryptoFileComponent component;
-	private final ConcurrentMap<CleartextFileChannel, FileChannel> openChannels = new ConcurrentHashMap<>();
+
+	private volatile int openChannelsCount = 0;
 
 	@Inject
-	public OpenCryptoFile(FileCloseListener listener, ChunkCache chunkCache, Cryptor cryptor, FileHeaderHolder headerHolder, ChunkIO chunkIO, @CurrentOpenFilePath AtomicReference<Path> currentFilePath, @OpenFileSize AtomicLong fileSize, @OpenFileModifiedDate AtomicReference<Instant> lastModified, OpenCryptoFileComponent component) {
+	public OpenCryptoFile(FileCloseListener listener, ChunkCache chunkCache, Cryptor cryptor, FileHeaderHolder headerHolder, @CurrentOpenFilePath AtomicReference<Path> currentFilePath, @OpenFileSize AtomicLong fileSize, @OpenFileModifiedDate AtomicReference<Instant> 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();
 		}
 	}

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<Instant> 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);
 

src/test/java/org/cryptomator/cryptofs/fh/OpenCryptoFileTest.java

@@ -4,15 +4,13 @@
 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;
 import org.cryptomator.cryptolib.api.FileHeader;
 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<Instant> 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<ChannelCloseListener> listener;
 		private AtomicReference<FileChannel> 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());
-		}
-
 	}
 
 }