Merge pull request #238 from cryptomator/feature/inversion-of-chunkre…

…gister

Refactoring: Simplify OpenCryptoFile

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

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<Instant> lastModified;
 	private final ExceptionsDuringWrite exceptionsDuringWrite;
-	private final ChannelCloseListener closeListener;
+	private final Consumer<FileChannel> 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, ChunkCache chunkCache, BufferPool bufferPool, EffectiveOpenOptions options, @OpenFileSize AtomicLong fileSize, @OpenFileModifiedDate AtomicReference<Instant> lastModified, @CurrentOpenFilePath AtomicReference<Path> currentPath, ExceptionsDuringWrite exceptionsDuringWrite, Consumer<FileChannel> 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.<CloseAction>of(this::flush, //
 				super::implCloseChannel, //
-				() -> closeListener.closed(this), //
+				() -> closeListener.accept(ciphertextFileChannel),
 				ciphertextFileChannel::close, //
 				this::tryPersistLastModified);
 		tryAll(closeActions.iterator());

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<Path> currentFilePath;
 	private final AtomicLong fileSize;
 	private final OpenCryptoFileComponent component;
-	private final ConcurrentMap<CleartextFileChannel, FileChannel> 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<Path> currentFilePath, @OpenFileSize AtomicLong fileSize, @OpenFileModifiedDate AtomicReference<Instant> lastModified, OpenCryptoFileComponent component) {
+	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) {
 		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();
 		}
 	}

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<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 Consumer<FileChannel> 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();
 		}
 

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<ChannelCloseListener> listener;
+		private AtomicReference<Consumer<FileChannel>> listener;
 		private AtomicReference<FileChannel> 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());
 		}