Feature: First close file channel, then persist lastModified

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

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

src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java

@@ -233,7 +233,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();
@@ -324,18 +325,31 @@ long beginOfChunk(long cleartextPos) {
 	protected void implCloseChannel() throws IOException {
 		try {
 			flush();
-			ciphertextFileChannel.force(true);
+		} finally {
+			implCloseChannelFinally1();
+		}
+	}
+
+	private void implCloseChannelFinally1() throws IOException {
+		try {
+			super.implCloseChannel();
+		} finally {
+			closeListener.closed(this);
+			implCloseChannelFinally2();
+		}
+	}
+
+	private void implCloseChannelFinally2() throws IOException {
+		try {
+			ciphertextFileChannel.close();
+		} finally {
 			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());
 			}
-		} finally {
-			super.implCloseChannel();
-			closeListener.closed(this);
 		}
 	}
 }

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

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