Feature: First close file channel, then persist lastModified #228
Conversation
…nderlying storage
# Conflicts: # src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java
WalkthroughOverall, the changes improve the handling of file attributes and channel closures in the Cryptomator project. Key modifications include the restructuring of methods for better readability, the removal of unnecessary exception declarations, and enhanced testability. These adjustments ensure a more robust, maintainable, and test-friendly codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CryptoBasicFileAttributeView
participant Filesystem
participant CleartextFileChannel
participant OpenCryptoFile
User ->> CryptoBasicFileAttributeView: setTimes(lastModifiedTime, lastAccessTime, createTime)
CryptoBasicFileAttributeView ->> Filesystem: Set ciphertext attributes
CryptoBasicFileAttributeView ->> OpenCryptoFile: Check and set last modified time
User ->> CleartextFileChannel: Close Channel
CleartextFileChannel ->> OpenCryptoFile: channelClosed(cleartextFileChannel)
OpenCryptoFile ->> CleartextFileChannel: Close if no open channels
CleartextFileChannel ->> Filesystem: Flush and close
PoemIn Cryptomator's code we delve, Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- src/main/java/org/cryptomator/cryptofs/attr/CryptoBasicFileAttributeView.java (1 hunks)
- src/main/java/org/cryptomator/cryptofs/ch/ChannelCloseListener.java (1 hunks)
- src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java (2 hunks)
- src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java (1 hunks)
- src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java (5 hunks)
Additional comments not posted (4)
src/main/java/org/cryptomator/cryptofs/ch/ChannelCloseListener.java (1)
7-7: Simplified method signature enhances interface usability.#!/bin/bash # Description: Verify that all implementations of `closed` method handle IOException internally if necessary. rg --type java $'void closed(CleartextFileChannel channel)' -A 10src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java (1)
186-192: Improved resource management by ensuring proper channel and file closure.#!/bin/bash # Description: Verify that all error scenarios during channel closure are adequately handled. rg --type java $'channelClosed' -A 20 -B 10src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java (2)
251-257: Ensure correct order of operations intestCloseCipherChannelCloseBeforePersist.This test correctly checks the sequence of closing the channel before persisting
lastModified, aligning with the PR's intent.
73-73: Ensure the mock path setup is correct and consistent with the expected test environment.#!/bin/bash # Description: Check if the mock setup for `filePath` is consistently used across the test suite. rg --type java --multiline $'Mockito.mock(Path.class, "/foo/bar")' src/test/java/org/cryptomator/cryptofs/ch/
src/main/java/org/cryptomator/cryptofs/attr/CryptoBasicFileAttributeView.java
Show resolved
Hide resolved
| @VisibleForTesting | ||
| void flush() throws IOException { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java (2 hunks)
Additional comments not posted (4)
src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java (4)
236-237: LGTM! The@VisibleForTestingannotation is appropriate.The
flushmethod is now accessible for testing, which is beneficial for verifying the flush logic.
328-331: LGTM! The refactoring improves reliability.The
implCloseChannelmethod now ensures thatimplCloseChannelFinally1is always called, even if an exception occurs during theflushmethod.
333-340: LGTM! The refactoring improves reliability and readability.The
implCloseChannelFinally1method ensures thatcloseListener.closed(this)is always called, even if an exception occurs during thesuper.implCloseChannel()method. It also ensures thatimplCloseChannelFinally2is always called.
342-345: LGTM! The refactoring improves reliability and error handling.The
implCloseChannelFinally2method ensures thatciphertextFileChannel.close()is always called, even if an exception occurs during thepersistLastModifiedmethod. The handling ofNoSuchFileExceptionand logging of otherIOExceptioncases is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java (3 hunks)
Additional comments not posted (1)
src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java (1)
238-239: Visibility change offlushmethod for testing purposes.The visibility of the
flushmethod has been changed to package-private and annotated with@VisibleForTesting. This is a good practice as it allows for unit testing of methods that would otherwise be private, enhancing testability without exposing them as part of the public API.
| 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()); | ||
| } |
There was a problem hiding this comment.
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.
| 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; | |
| } |
This PR changes the logic of persisting the lastModfied date from before closing the ciphertext channel to afterwards. Additionally, the cryptofs internal lastModifiedDate is always updated, even when persisting to storage fails.
This impl change is motivated by the behaviour of some fs/storage implemenations, which set the lastModified time to "now" when closing a channel, regardless of changes made.
Before merging, we should test the implementation with different scenarios:
The test was to edit and save an spreadsheet/document file and copying files of different sizes into the vault.
1: iCloud on Windows is... strange. First of all, everything copied into the iCloud dir gets as mTime the current time. Inside a Cryptomator vault, this becomes even stranger. Sometimes, the files are set to the current time, but especially bigger files (<30MB) get ... "some" time. "Some" can be the current, the former file mTime or something in between.
2: On Linux there are no offical filesystem integrations. GNOME offers "online account", but these are not accessible from the filesystem.