Merge pull request #228 from cryptomator/feature/205-close-before-las…

…tModifiedPersist Feature: First close file channel, then persist lastModified
cryptomator · Aug 2, 2024 · 8d7251a · 8d7251a
2 parents 93bfa1c + 6757801
commit 8d7251a
Show file tree

Hide file tree

Showing 5 changed files with 70 additions and 36 deletions.
diff --git a/src/main/java/org/cryptomator/cryptofs/attr/CryptoBasicFileAttributeView.java b/src/main/java/org/cryptomator/cryptofs/attr/CryptoBasicFileAttributeView.java
@@ -48,10 +48,10 @@ public BasicFileAttributes readAttributes() throws IOException {
 	@Override
 	public void setTimes(FileTime lastModifiedTime, FileTime lastAccessTime, FileTime createTime) throws IOException {
 		readonlyFlag.assertWritable();
-		getCiphertextAttributeView(BasicFileAttributeView.class).setTimes(lastModifiedTime, lastAccessTime, createTime);
 		if (lastModifiedTime != null) {
 			getOpenCryptoFile().ifPresent(file -> file.setLastModifiedTime(lastModifiedTime));
 		}
+		getCiphertextAttributeView(BasicFileAttributeView.class).setTimes(lastModifiedTime, lastAccessTime, createTime);
 	}
 
 }
diff --git a/src/main/java/org/cryptomator/cryptofs/ch/ChannelCloseListener.java b/src/main/java/org/cryptomator/cryptofs/ch/ChannelCloseListener.java
@@ -1,11 +1,9 @@
 package org.cryptomator.cryptofs.ch;
 
 
-import java.io.IOException;
-
 @FunctionalInterface
 public interface ChannelCloseListener {
 
-	void closed(CleartextFileChannel channel) throws IOException;
+	void closed(CleartextFileChannel channel);
 
 }
diff --git a/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java b/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java
@@ -29,6 +29,8 @@
 import java.nio.file.attribute.BasicFileAttributeView;
 import java.nio.file.attribute.FileTime;
 import java.time.Instant;
+import java.util.Iterator;
+import java.util.List;
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.concurrent.locks.ReadWriteLock;
@@ -233,7 +235,8 @@ private void forceInternal(boolean metaData) throws IOException {
 	 *
 	 * @throws IOException
 	 */
-	private void flush() throws IOException {
+	@VisibleForTesting
+	void flush() throws IOException {
 		if (isWritable()) {
 			writeHeaderIfNeeded();
 			chunkCache.flush();
@@ -322,20 +325,38 @@ long beginOfChunk(long cleartextPos) {
 
 	@Override
 	protected void implCloseChannel() throws IOException {
+		var closeActions = List.<CloseAction>of(this::flush, //
+				super::implCloseChannel, //
+				() -> closeListener.closed(this), //
+				ciphertextFileChannel::close, //
+				this::tryPersistLastModified);
+		tryAll(closeActions.iterator());
+	}
+
+	private void tryPersistLastModified() {
 		try {
-			flush();
-			ciphertextFileChannel.force(true);
+			persistLastModified();
+		} catch (NoSuchFileException nsfe) {
+			//no-op, see https://github.com/cryptomator/cryptofs/issues/169
+		} catch (IOException e) {
+			LOG.warn("Failed to persist last modified timestamp for encrypted file: {}", e.getMessage());
+		}
+	}
+
+	private void tryAll(Iterator<CloseAction> actions) throws IOException {
+		if (actions.hasNext()) {
 			try {
-				persistLastModified();
-			} catch (NoSuchFileException nsfe) {
-				//no-op, see https://github.com/cryptomator/cryptofs/issues/169
-			} catch (IOException e) {
-				//only best effort attempt
-				LOG.warn("Failed to persist last modified timestamp for encrypted file: {}", e.getMessage());
+				actions.next().run();
+			} finally {
+				tryAll(actions);
 			}
-		} finally {
-			super.implCloseChannel();
-			closeListener.closed(this);
 		}
 	}
+
+	@FunctionalInterface
+	private interface CloseAction {
+
+		void run() throws IOException;
+	}
+
 }
diff --git a/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java b/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java
@@ -183,17 +183,13 @@ public void updateCurrentFilePath(Path newFilePath) {
 		currentFilePath.updateAndGet(p -> p == null ? null : newFilePath);
 	}
 
-	private synchronized void channelClosed(CleartextFileChannel cleartextFileChannel) throws IOException {
-		try {
-			FileChannel ciphertextFileChannel = openChannels.remove(cleartextFileChannel);
-			if (ciphertextFileChannel != null) {
-				chunkIO.unregisterChannel(ciphertextFileChannel);
-				ciphertextFileChannel.close();
-			}
-		} finally {
-			if (openChannels.isEmpty()) {
-				close();
-			}
+	private synchronized void channelClosed(CleartextFileChannel cleartextFileChannel) {
+		FileChannel ciphertextFileChannel = openChannels.remove(cleartextFileChannel);
+		if (ciphertextFileChannel != null) {
+			chunkIO.unregisterChannel(ciphertextFileChannel);
+		}
+		if (openChannels.isEmpty()) {
+			close();
 		}
 	}
 

diff --git a/src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java b/src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java
@@ -43,7 +43,6 @@
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReadWriteLock;
-import java.util.function.Supplier;
 
 import static org.hamcrest.CoreMatchers.is;
 import static org.mockito.ArgumentMatchers.any;
@@ -71,7 +70,7 @@ public class CleartextFileChannelTest {
 	private FileHeaderHolder headerHolder = mock(FileHeaderHolder.class);
 	private AtomicBoolean headerIsPersisted = mock(AtomicBoolean.class);
 	private EffectiveOpenOptions options = mock(EffectiveOpenOptions.class);
-	private Path filePath = Mockito.mock(Path.class,"/foo/bar");
+	private Path filePath = Mockito.mock(Path.class, "/foo/bar");
 	private AtomicReference<Path> currentFilePath = new AtomicReference<>(filePath);
 	private AtomicLong fileSize = new AtomicLong(100);
 	private AtomicReference<Instant> lastModified = new AtomicReference<>(Instant.ofEpochMilli(0));
@@ -98,7 +97,7 @@ public void setUp() throws IOException {
 		var fsProvider = Mockito.mock(FileSystemProvider.class);
 		when(filePath.getFileSystem()).thenReturn(fs);
 		when(fs.provider()).thenReturn(fsProvider);
-		when(fsProvider.getFileAttributeView(filePath,BasicFileAttributeView.class)).thenReturn(attributeView);
+		when(fsProvider.getFileAttributeView(filePath, BasicFileAttributeView.class)).thenReturn(attributeView);
 		when(readWriteLock.readLock()).thenReturn(readLock);
 		when(readWriteLock.writeLock()).thenReturn(writeLock);
 
@@ -236,20 +235,26 @@ public void testForceWithoutMetadataDoesntUpdatesLastModifiedTime() throws IOExc
 	public class Close {
 
 		@Test
-		public void testCloseTriggersCloseListener() throws IOException {
-			inTest.implCloseChannel();
+		@DisplayName("IOException during flush cleans up, persists lastModified and rethrows")
+		public void testCloseIoExceptionFlush() throws IOException {
+			var inSpy = Mockito.spy(inTest);
+			Mockito.doThrow(IOException.class).when(inSpy).flush();
+
+			Assertions.assertThrows(IOException.class, () -> inSpy.implCloseChannel());
 
-			verify(closeListener).closed(inTest);
+			verify(closeListener).closed(inSpy);
+			verify(ciphertextFileChannel).close();
+			verify(inSpy).persistLastModified();
 		}
 
 		@Test
-		@DisplayName("On close, first flush channel, then persist lastModified")
-		public void testCloseFlushBeforePersist() throws IOException {
+		@DisplayName("On close, first close channel, then persist lastModified")
+		public void testCloseCipherChannelCloseBeforePersist() throws IOException {
 			var inSpy = spy(inTest);
 			inSpy.implCloseChannel();
 
 			var ordering = inOrder(inSpy, ciphertextFileChannel);
-			ordering.verify(ciphertextFileChannel).force(true);
+			ordering.verify(ciphertextFileChannel).close();
 			ordering.verify(inSpy).persistLastModified();
 		}
 
@@ -264,6 +269,20 @@ public void testCloseUpdatesLastModifiedTimeIfWriteable() throws IOException {
 			verify(attributeView).setTimes(Mockito.eq(fileTime), Mockito.any(), Mockito.isNull());
 		}
 
+		@Test
+		@DisplayName("IOException on persisting lastModified during close is ignored")
+		public void testCloseExceptionOnLastModifiedPersistenceIgnored() throws IOException {
+			when(options.writable()).thenReturn(true);
+			lastModified.set(Instant.ofEpochMilli(123456789000l));
+
+			var inSpy = Mockito.spy(inTest);
+			Mockito.doThrow(IOException.class).when(inSpy).persistLastModified();
+
+			Assertions.assertDoesNotThrow(() -> inSpy.implCloseChannel());
+			verify(closeListener).closed(inSpy);
+			verify(ciphertextFileChannel).close();
+		}
+
 		@Test
 		public void testCloseDoesNotUpdateLastModifiedTimeIfReadOnly() throws IOException {
 			when(options.writable()).thenReturn(false);