diff --git a/src/main/java/org/cryptomator/cryptofs/attr/CryptoBasicFileAttributeView.java b/src/main/java/org/cryptomator/cryptofs/attr/CryptoBasicFileAttributeView.java index 1e44bfa18..f621b770b 100644 --- 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 index 4933e1bcf..3ee1c0080 100644 --- 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); } \ No newline at end of file diff --git a/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java b/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java index 4723e80b1..44d165a8b 100644 --- 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.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 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 index a07b5db26..14e5d072e 100644 --- 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 index 89213ddf5..a19e508df 100644 --- 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 currentFilePath = new AtomicReference<>(filePath); private AtomicLong fileSize = new AtomicLong(100); private AtomicReference 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);