[Bug] File zeroed on save if you quit too quickly

[iainh]

When testing zee's performance when working with large files I came across an issue where a file's content can be partially of completely lost on save if you quit the application before the save operation has completed.

Steps to reproduce:

1. Create or download a very large text file (I used http://mattmahoney.net/dc/enwik9.zip)
2. Open the file, save it, and quit with save and quit happened very close together
3. Notice the file is now 0 bytes

Digging into the code, it appears the following potential issues exist which together allow for zeroing the file:

1. Files are truncated before save by File::create()
2. strip_trailing_whitespace() slows down the save operation enough that it actually takes 5-10 seconds to write out a 953M file
3. Exiting the application is allowed while there is an in progress save operation

My testing was done using a build of zee from the master branch on macOS 12.4 (arm64).

[kevinmatthes]

At the moment, I am considering possible solutions to the Colour Panic bug, see #42, and came across the strip_trailing_whitespace() function, as well.

I found this implementation working identically compared to the current one:

pub fn strip_trailing_whitespace(text: Rope) -> Rope {
    text.lines()
        .map(|x| x.to_string().trim_end().to_string() + "\n")
        .collect()
}

Does this version raise the performance when changing the implementation in zee-edit/src/graphemes.rs, line 122, or is it even worse? Maybe the performance boost suffices to save the file before the exit?

[kevinmatthes]

Despite that my suggestion adds another newline to end of the buffer, how does it perform in contrast to the original implementation regarding your example on your machine?

[kevinmatthes]

In my opinion, there are two major starting points to solve this issue:

1. Implement a mechanism to wait for the saving to finish before leaving the application.
2. Optimise the whitespace cropping.

Implementing just one of them leaves the other one open and, thus, makes it source for further bugs. The complete solution should consist of an implementation of both features.

Regarding the waiting mechanism, we would need to signal the end of the saving operation to the key input handler. The first question to answer is how the writing operation's ending can be queried without interrupting it. Freezing the editor as long as the file is written is surely not the most preferable option but in the end, it would be an emergency solution.

When taking a look at the implementation of the whitespace cropping function, it looks for me as if the an implementation using iterators would be more efficient since at the moment, the required values are queried whenever they are needed which seems to cause this implementation taking much time. The iterator approach would just go over the whole text once without needing to lookup neighboured data multiple times since it is implicit. Unfortunately, the collectors are not very flexible which may cause unnecessary type conversions.

Any ideas?

[iainh]

I've opened PR #60 to add a configuration parameter to disable removing trailing white space on save. My thought process on this is that it is expensive and might not be the right for all users in all cases. As mentioned in the PR, my changes also move the call to remove trailing white space from between when the file is truncated and when it is written to before it is truncated which doesn't fix the issue with the user being able to close the application while the file is still being written but it should greatly reduce the chances of triggering it in most cases.

In addition, I agree with the need to implement both a mechanism to wait for saving to finish and to optimize the white space trimming code. I did so prototyping on disallowing exiting while a file is being saved (why "dirty buffers exist really) and it looks fairly straight forward. Buffers track their modify status so anything that isn't ModifiedStatus::Unchanged can be considered "dirty". Not what we'd want to commit but here's a PoC:

diff --git a/zee/src/editor/mod.rs b/zee/src/editor/mod.rs
index 4075477..73e9aa4 100644
--- a/zee/src/editor/mod.rs
+++ b/zee/src/editor/mod.rs
@@ -374,7 +374,17 @@ impl Component for Editor {
                 self.prompt_height = self.prompt_action.initial_height();
             }
             Message::Quit => {
-                self.context.link.exit();
+                if !self
+                    .buffers
+                    .iter()
+                    .any(|buffer| buffer.modified_status() != ModifiedStatus::Unchanged)
+                {
+                    self.context.link.exit();
+                } else {
+                    self.context.link.send(Message::Log(Some(
+                        "Unable to quit. Unsaved buffers exist".to_string(),
+                    )));
+                }
             }
             Message::Buffer(message) => self.buffers.handle_message(message),
             _ => {}

To do this properly, I think we would need a new variant of PromptAction which displays a message such as "Unsaved buffer exist, quit anyway? yes/no" and allows the user to enter yes/no or y/n. The user's input would then control whether the original request to quit is completed utilizing the callback mechanism.

Optimizing strip_trailing_whitespace() is itself an interesting problem to solve. I'll write a standalone application that performs only that operation to get some concrete performance numbers and I'll try your implementation, see how it compares.

[kevinmatthes]

How about making the waiting for the saving to finish an option for the configuration file, as well?

[iainh]

@kevinmatthes the big issue with the above PoC and why I mention that it shouldn't be committed it is that it stops the user from being able to exit without saving even in cases where they don't want to persist the changes made, for example when using zee to temporarily write something up that will be copied to another application. I think adding a option to the configuration for this would be short lived since it would go away if/when interactive messages are added to zee.