Fix spurious backups of unmodified files#3822
Conversation
This will make it easier to use calcHash() in other SharedBuffer methods.
Instead of calculating the hash of the buffer every time Modified() is called, do that every time b.isModified is updated (i.e. every time the buffer is modified) and set b.isModified value accordingly. This change means that the hash will be recalculated every time the user types or deletes a character. But that is what already happens anyway, since inserting or deleting characters triggers redrawing the display, in particular redrawing the status line, which triggers Modified() in order to show the up-to-date modified/unmodified status in the status line. And with this change, we will be able to check this status more than once during a single "handle event & redraw" cycle, while still recalculating the hash only once.
Various methods of Buffer should be rather methods of SharedBuffer. This commit doesn't move all of them to SharedBuffer yet, only those that need to be moved to SharedBuffer in order to be able to request creating or removing backups in other SharedBuffer methods.
Micro's logic for periodic backup creation is racy and may cause spurious backups of unmodified buffers, at least for the following reasons: 1. When a buffer is closed, its backup is removed by the main goroutine, without any synchronization with the backup/save goroutine which creates periodic backups in the background. A part of the problem here is that the main goroutine removes the backup before setting b.fini to true, not after it, so the backup/save goroutine may start creating a new backup even after it has been removed by the main goroutine. But even if we move the b.RemoveBackup() call after setting b.fini, it will not solve the problem, since the backup/save goroutine may have already started creating a new periodic backup just before b.fini was set to true. 2. When a buffer is successfully saved and thus its backup is removed, if there was a periodic backup for this buffer requested by the main goroutine but not saved by the backup/save goroutine yet (i.e. this request is still pending in backupRequestChan), micro doesn't cancel this pending request, so a backup is unexpectedly saved a couple of seconds after the file itself was saved. Although usually this erroneous backup is removed later, when the buffer is closed. But if micro terminates abnormally and the buffer is not properly closed, this backup is not removed. Also if this issue occurs in combination with the race issue micro-editor#1 described above, this backup may not be successfully removed either. So, to fix these issues: 1. Do the backup removal in the backup/save goroutine (at requests from the main goroutine), not directly in the main goroutine. 2. Make the communication between these goroutines fully synchronous: 2a. Instead of using the buffered channel backupRequestChan as a storage for pending requests for periodic backups, let the backup/save goroutine itself store this information, in the requestesBackups map. Then, backupRequestChan can be made non-buffered. 2b. Make saveRequestChan a non-buffered channel as well. (There was no point in making it buffered in the first place, actually.) Once both channels are non-buffered, the backup/save goroutine receives both backup and save requests from the main goroutine in exactly the same order as the main goroutine sends them, so we can guarantee that saving the buffer will cancel the previous pending backup request for this buffer.
We should cancel previously requested periodic backup (and remove this backup if it has already been created) not only when saving or closing the buffer but also in other cases when the buffer's state changes from modified to unmodified, i.e. when the user undoes all unsaved changes. Otherwise, if micro terminates abnormally before the buffer is closed, this backup will not get removed (so next time micro will suggest the user to recover this file), even though all changes to this file were successfully saved.
When we need to remove existing backup for whatever reason (e.g. because we've just successfully saved the file), we should do that regardless of whether backups are enabled or not, since a backup may exist regardless (it could have been created before the `backup` option got disabled).
Use the same backup write helper function for both periodic background backups and for temporary backups in safeWrite(). Besides just removing code duplication, this brings the advantages of both together: - Temporary backups in safeWrite() now use the same atomic mechanism when replacing an already existing backup. So that if micro crashes in the middle of writing the backup in safeWrite(), this corrupted backup will not overwrite a previous good backup. - Better error handling for periodic backups.
When writing a backup file, we should write it atomically (i.e. use a temporary file + rename) in all cases, not only when replacing an existing backup. Just like we do in util.SafeWrite(). Otherwise, if micro crashes while writing this backup, even if that doesn't result in corrupting an existing good backup, it still results in creating an undesired backup with invalid contents.
|
...On top of this, just pushed 2 more (slightly unrelated) changes:
|
|
|
||
| if b.isModified { | ||
| b.RequestBackup() | ||
| } else { | ||
| b.CancelBackup() | ||
| } |
There was a problem hiding this comment.
But this doesn't help for large file, right? Because b.isModified is always true due to fastdirty being forced to true.
At least we know when the UndoStack is empty. 🤔
There was a problem hiding this comment.
Yes, but that's a separate issue, not exactly related to backups, right? It's annoying that the buffer status remains "modified" until the user saves the buffer, but that is something the user is aware of (the statusline shows that the buffer is modified, and micro asks to save the file when the user closes it), so at least it is not unexpected that micro does not remove the backup until the user saves the buffer, right?
...Though while we're at this topic, I agree we should improve that by simply checking if the undo stack is empty (or more precisely, if it has the same size as the last time when the buffer was saved). Unfortunately, as I noted elsewhere, it is not that easy: first we need to fix bypassing the undo stack in Retab() and MoveLinesUp() (at least).
And last time I tried to fix that in MoveLinesUp(), I failed to figure out how to do that correctly (i.e. in such a way that wouldn't break MoveLinesUp() itself, when the cursor is at the last line).
There was a problem hiding this comment.
[...] not exactly related to backups, right?
At least not more than 7aa495f. 😉
it is not that easy
Agree. Should be handled independent of this PR then. 👍
For the record: since we merged this PR, I've never observed this issue again. So I'm pretty convinced that it was indeed caused by some of the issues fixed by this PR. (And it feels that most likely that was issue 1, i.e. race between creating and removing backups). |
2 participants
Since a very long time I've been sporadically encountering a nasty hard-to-diagnose issue [*]: when opening a file, micro detects a backup for this file (and thus suggests to recover the file from this backup), whereas the content of this backup is identical to the content of the file itself, i.e. it doesn't have any unsaved changed (IOW there is nothing to recover). Which indicates that the backup was created mistakenly, or that it was not removed after the file was successfully saved.
This PR attempts to fix this problem, by fixing at least the following actual existing bugs that might cause it:
Possible race between creating backups in the backup/save goroutine (periodically in the background) and removing backups in the main goroutine (when closing a buffer), which may result in a backup not being removed successfully.
When a buffer is saved, its backup is removed. However, if there is a pending request for the next periodic backup, micro doesn't cancel this request, so a new backup is unexpectedly created a couple of seconds after the file was saved. (Although usually this erroneous backup is removed later, when the buffer is closed. But if micro terminates abnormally and the buffer is not properly closed, this backup is not removed. Also if this issue occurs in combination with the race issue 1 described above, this backup may not be successfully removed either.)
When the user undoes all changes made since the last save, so the buffer status changes from "modified" to "unmodified", micro doesn't remove its backup. (Although, again, usually it is removed later when the buffer is closed. But again, it may not be the case if micro terminates abnormally, or if this issue occurs together with the race issue 1.)
When micro detects a panic and performs an emergency exit, it creates backups for all opened buffers, even those that don't have any unsaved changed.
[*] I should also note: it seems that since merging #3273 I'm reproducing this problem much more often than before. Although I have no idea how exactly #3273 could cause that. Perhaps it just affected timings in such a way that, for example, made the race condition issue 1 more likely to occur?..