minified refactoring by not breaking package boundaries

infeo · infeo · commit ff50fbab2bb2 · 2024-12-07T11:59:28.000+01:00
diff --git 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 Runnable closeListener); //
+								@BindsInstance Consumer<FileChannel> closeListener); //
 	}
 
 }
diff --git a/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java b/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java
@@ -7,7 +7,6 @@
 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;
@@ -35,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;
@@ -47,24 +47,22 @@ 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 Runnable closeListener;
+	private final Consumer<FileChannel> closeListener;
 	private final CryptoFileSystemStats stats;
 
 	@Inject
-	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) {
+	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;
 		this.cryptor = cryptor;
-		this.chunkIO = chunkIO;
 		this.chunkCache = chunkCache;
 		this.bufferPool = bufferPool;
 		this.options = options;
@@ -80,7 +78,6 @@ public CleartextFileChannel(FileChannel ciphertextFileChannel, FileHeaderHolder
 		if (options.createNew() || options.create()) {
 			lastModified.compareAndSet(Instant.EPOCH, Instant.now());
 		}
-		chunkIO.registerChannel(ciphertextFileChannel, options.writable());
 	}
 
 	@Override
@@ -331,8 +328,7 @@ long beginOfChunk(long cleartextPos) {
 	protected void implCloseChannel() throws IOException {
 		var closeActions = List.<CloseAction>of(this::flush, //
 				super::implCloseChannel, //
-				() -> chunkIO.unregisterChannel(ciphertextFileChannel), // _after_ flush, since the flushed chunk cache uses chunkIO file channels
-				closeListener::run, //
+				() -> closeListener.accept(ciphertextFileChannel),
 				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
@@ -11,7 +11,7 @@
 import java.util.concurrent.ConcurrentHashMap;
 
 @OpenFileScoped
-public class ChunkIO {
+class ChunkIO {
 
 	private final Set<FileChannel> readableChannels = ConcurrentHashMap.newKeySet();
 	private final Set<FileChannel> 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
@@ -28,18 +28,22 @@ 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 volatile int openChannelsCount = 0;
 
 	@Inject
-	public OpenCryptoFile(FileCloseListener listener, ChunkCache chunkCache, Cryptor cryptor, FileHeaderHolder headerHolder, @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;
 		this.headerHolder = headerHolder;
+		this.chunkIO = chunkIO;
 		this.currentFilePath = currentFilePath;
 		this.fileSize = fileSize;
 		this.component = component;
@@ -72,12 +76,13 @@ public synchronized FileChannel newFileChannel(EffectiveOpenOptions options, Fil
 			}
 			initFileSize(ciphertextFileChannel);
 			cleartextFileChannel = component.newChannelComponent() //
-					.create(ciphertextFileChannel, options, this::channelClosed) //
+					.create(ciphertextFileChannel, options, this::cleartextChannelClosed) //
 					.channel();
+			chunkIO.registerChannel(ciphertextFileChannel,options.writable());
 		} finally {
 			if (cleartextFileChannel == null) { // i.e. something didn't work
+				cleartextChannelClosed(ciphertextFileChannel);
 				closeQuietly(ciphertextFileChannel);
-				channelClosed(); // if first channel and it fails, close this again
 			}
 		}
 
@@ -169,7 +174,11 @@ public void updateCurrentFilePath(Path newFilePath) {
 		currentFilePath.updateAndGet(p -> p == null ? null : newFilePath);
 	}
 
-	private synchronized void channelClosed() {
+	private synchronized void cleartextChannelClosed(FileChannel ciphertextFileChannel) {
+		if( ciphertextFileChannel != null ){
+			chunkIO.unregisterChannel(ciphertextFileChannel);
+		}
+
 		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
@@ -5,7 +5,6 @@
 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;
@@ -44,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;
@@ -60,7 +60,6 @@
 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);
@@ -78,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 Runnable closeListener = mock(Runnable.class);
+	private Consumer<FileChannel> closeListener = mock(Consumer.class);
 	private CryptoFileSystemStats stats = mock(CryptoFileSystemStats.class);
 
 	private CleartextFileChannel inTest;
@@ -89,8 +88,6 @@ 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);
@@ -105,7 +102,7 @@ public void setUp() throws IOException {
 		when(readWriteLock.readLock()).thenReturn(readLock);
 		when(readWriteLock.writeLock()).thenReturn(writeLock);
 
-		inTest = new CleartextFileChannel(ciphertextFileChannel, headerHolder, readWriteLock, cryptor, chunkIO, chunkCache, bufferPool, options, fileSize, lastModified, currentFilePath, exceptionsDuringWrite, closeListener, stats);
+		inTest = new CleartextFileChannel(ciphertextFileChannel, headerHolder, readWriteLock, cryptor, chunkCache, bufferPool, options, fileSize, lastModified, currentFilePath, exceptionsDuringWrite, closeListener, stats);
 	}
 
 	@Test
@@ -246,9 +243,8 @@ public void testCloseIoExceptionFlush() throws IOException {
 
 			Assertions.assertThrows(IOException.class, () -> inSpy.implCloseChannel());
 
-			verify(closeListener).run();
+			verify(closeListener).accept(ciphertextFileChannel);
 			verify(ciphertextFileChannel).close();
-			verify(chunkIO).unregisterChannel(ciphertextFileChannel);
 			verify(inSpy).persistLastModified();
 		}
 
@@ -258,9 +254,9 @@ public void testCloseCipherChannelFlushBeforeUnregister() throws IOException {
 			var inSpy = spy(inTest);
 			inSpy.implCloseChannel();
 
-			var ordering = inOrder(inSpy, chunkIO);
+			var ordering = inOrder(inSpy, closeListener);
 			ordering.verify(inSpy).flush();
-			ordering.verify(chunkIO).unregisterChannel(ciphertextFileChannel);
+			verify(closeListener).accept(ciphertextFileChannel);
 		}
 
 		@Test
@@ -294,9 +290,8 @@ public void testCloseExceptionOnLastModifiedPersistenceIgnored() throws IOExcept
 			var inSpy = Mockito.spy(inTest);
 			Mockito.doThrow(IOException.class).when(inSpy).persistLastModified();
 
-			Assertions.assertDoesNotThrow(() -> inSpy.implCloseChannel());
-			verify(chunkIO).unregisterChannel(ciphertextFileChannel);
-			verify(closeListener).run();
+			Assertions.assertDoesNotThrow(inSpy::implCloseChannel);
+			verify(closeListener).accept(ciphertextFileChannel);
 			verify(ciphertextFileChannel).close();
 		}
 
@@ -402,7 +397,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, chunkIO, chunkCache, bufferPool, options, fileSize, lastModified, currentFilePath, exceptionsDuringWrite, closeListener, stats);
+			inTest = new CleartextFileChannel(ciphertextFileChannel, headerHolder, readWriteLock, cryptor, chunkCache, bufferPool, options, fileSize, lastModified, currentFilePath, exceptionsDuringWrite, closeListener, stats);
 			ByteBuffer buf = ByteBuffer.allocate(10);
 
 			// A read from frist chunk:
@@ -574,7 +569,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, chunkIO, chunkCache, bufferPool, options, fileSize, lastModified, currentFilePath, exceptionsDuringWrite, closeListener, stats);
+			inTest = new CleartextFileChannel(ciphertextFileChannel, headerHolder, readWriteLock, cryptor, 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
@@ -11,6 +11,7 @@
 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;
@@ -33,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;
@@ -50,6 +52,7 @@ 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);
@@ -69,7 +72,7 @@ public static void tearDown() throws IOException {
 
 	@Test
 	public void testCloseTriggersCloseListener() {
-		OpenCryptoFile openCryptoFile = new OpenCryptoFile(closeListener, chunkCache, cryptor, headerHolder, CURRENT_FILE_PATH, fileSize, lastModified, openCryptoFileComponent);
+		OpenCryptoFile openCryptoFile = new OpenCryptoFile(closeListener, chunkCache, cryptor, headerHolder, chunkIO, CURRENT_FILE_PATH, fileSize, lastModified, openCryptoFileComponent);
 		openCryptoFile.close();
 		verify(closeListener).close(CURRENT_FILE_PATH.get(), openCryptoFile);
 	}
@@ -80,7 +83,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, CURRENT_FILE_PATH, fileSize, lastModified, openCryptoFileComponent);
+		OpenCryptoFile openCryptoFile = new OpenCryptoFile(closeListener, chunkCache, cryptor, headerHolder, chunkIO, CURRENT_FILE_PATH, fileSize, lastModified, openCryptoFileComponent);
 
 		UncheckedIOException exception = Assertions.assertThrows(UncheckedIOException.class, () -> {
 			openCryptoFile.newFileChannel(options);
@@ -99,7 +102,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, CURRENT_FILE_PATH, fileSize, lastModified, openCryptoFileComponent);
+		OpenCryptoFile openCryptoFile = new OpenCryptoFile(closeListener, chunkCache, cryptor, headerHolder, chunkIO, CURRENT_FILE_PATH, fileSize, lastModified, openCryptoFileComponent);
 
 		openCryptoFile.newFileChannel(options);
 		verify(fileSize).set(0L);
@@ -111,7 +114,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, CURRENT_FILE_PATH, fileSize, lastModified, openCryptoFileComponent);
+		OpenCryptoFile inTest = new OpenCryptoFile(closeListener, chunkCache, cryptor, headerHolder, chunkIO, CURRENT_FILE_PATH, fileSize, lastModified, openCryptoFileComponent);
 
 		@Test
 		@DisplayName("Skip file header init, if the file header already exists in memory")
@@ -188,19 +191,22 @@ public class FileChannelFactoryTest {
 		private final AtomicLong realFileSize = new AtomicLong(-1L);
 		private OpenCryptoFile openCryptoFile;
 		private CleartextFileChannel cleartextFileChannel;
+		private AtomicReference<Consumer<FileChannel>> 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, CURRENT_FILE_PATH, realFileSize, lastModified, openCryptoFileComponent);
+			openCryptoFile = new OpenCryptoFile(closeListener, chunkCache, cryptor, headerHolder, chunkIO, 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);
@@ -221,6 +227,7 @@ 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
@@ -266,6 +273,17 @@ 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().accept(ciphertextChannel.get());
+			verify(chunkIO).unregisterChannel(ciphertextChannel.get());
+		}
+
 	}
 
 }

Original file line number	Diff line number	Diff line change
`@@ -5,6 +5,7 @@`
`5`	`5`	`import org.cryptomator.cryptofs.EffectiveOpenOptions;`
`6`	`6`
`7`	`7`	`import java.nio.channels.FileChannel;`
	`8`	`+import java.util.function.Consumer;`
`8`	`9`
`9`	`10`	`@ChannelScoped`
`10`	`11`	`@Subcomponent`
`@@ -17,7 +18,7 @@ interface Factory {`
`17`	`18`
`18`	`19`	`ChannelComponent create(@BindsInstance FileChannel ciphertextChannel, //`
`19`	`20`	`@BindsInstance EffectiveOpenOptions options, //`
`20`		`- @BindsInstance Runnable closeListener); //`
	`21`	`+ @BindsInstance Consumer<FileChannel> closeListener); //`
`21`	`22`	`}`
`22`	`23`
`23`	`24`	`}`