Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feature: First close file channel, then persist lastModified #228

Merged
merged 9 commits into from
Aug 2, 2024
Merged

Feature: First close file channel, then persist lastModified #228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
34f1f80
fixes #205
infeo Feb 8, 2024
6a3c352
first store updated lastModified in filesystem before writing it to u…
infeo Feb 8, 2024
9fc9780
improve unit tests
infeo Feb 8, 2024
ae742d1
always persist lastModified
infeo Feb 9, 2024
608ff17
Merge branch 'develop' into feature/205-close-before-lastModifiedPersist
infeo Feb 28, 2024
df785eb
fix unit test
infeo Feb 28, 2024
ecca2a6
Merge branch 'develop' into feature/205-close-before-lastModifiedPersist
infeo Jun 20, 2024
e247a2e
strech the finally methods over 3 methods
infeo Jul 2, 2024
6757801
use helper function to execute all close actions
infeo Jul 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/org/cryptomator/cryptofs/attr/CryptoBasicFileAttributeView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
infeo marked this conversation as resolved.
Show resolved Hide resolved
}

}
4 changes: 1 addition & 3 deletions src/main/java/org/cryptomator/cryptofs/ch/ChannelCloseListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);

}
45 changes: 33 additions & 12 deletions src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -233,7 +235,8 @@ private void forceInternal(boolean metaData) throws IOException {
*
* @throws IOException
*/
private void flush() throws IOException {
@VisibleForTesting
void flush() throws IOException {
Comment on lines +238 to +239
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarified testing intent and enhanced data integrity during channel closure.

Would you like me to help in adding or enhancing the unit tests to cover these new scenarios?

Also applies to: 331-342

if (isWritable()) {
writeHeaderIfNeeded();
chunkCache.flush();
Expand Down Expand Up @@ -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());
}
Comment on lines +336 to +343
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handling of exceptions in tryPersistLastModified.

The method tryPersistLastModified appropriately handles NoSuchFileException by performing no operation, which is a valid approach given the context provided in the linked issue. However, the catch block for IOException logs a warning, which is good, but it might also be beneficial to rethrow the exception to ensure that callers are aware that not all operations completed successfully.

Consider rethrowing the exception after logging to ensure that error handling is consistent and robust across the application.

- LOG.warn("Failed to persist last modified timestamp for encrypted file: {}", e.getMessage());
+ LOG.warn("Failed to persist last modified timestamp for encrypted file: {}", e.getMessage());
+ throw e;
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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 tryPersistLastModified() {
try {
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());
throw e;
}

}

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

infeo marked this conversation as resolved.
Show resolved Hide resolved
}
infeo marked this conversation as resolved.
Show resolved Hide resolved
18 changes: 7 additions & 11 deletions src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
infeo marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
37 changes: 28 additions & 9 deletions src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
Expand All @@ -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);

Expand Down Expand Up @@ -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();
infeo marked this conversation as resolved.
Show resolved Hide resolved
}

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

Expand All @@ -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);
Expand Down