From 2c010afbe4c2b2373277b4c43c87cec47ba5d470 Mon Sep 17 00:00:00 2001 From: Dmytro Maluka Date: Sat, 2 Aug 2025 22:13:35 +0200 Subject: [PATCH] Fix races between removing backups and creating periodic backups 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 #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. --- cmd/micro/micro.go | 2 -- internal/buffer/backup.go | 50 +++++++++++++++++++++++++++++---------- internal/buffer/buffer.go | 8 +------ internal/buffer/save.go | 23 +++++++++--------- 4 files changed, 50 insertions(+), 33 deletions(-) diff --git a/cmd/micro/micro.go b/cmd/micro/micro.go index 70b940bf..775301ee 100644 --- a/cmd/micro/micro.go +++ b/cmd/micro/micro.go @@ -489,8 +489,6 @@ func DoEvent() { } case f := <-timerChan: f() - case b := <-buffer.BackupCompleteChan: - b.RequestedBackup = false case <-sighup: exit(0) case <-util.Sigterm: diff --git a/internal/buffer/backup.go b/internal/buffer/backup.go index 941ac04f..8b728ec6 100644 --- a/internal/buffer/backup.go +++ b/internal/buffer/backup.go @@ -34,20 +34,49 @@ Options: [r]ecover, [i]gnore, [a]bort: ` const backupSeconds = 8 -var BackupCompleteChan chan *SharedBuffer +type backupRequestType int + +const ( + backupCreate = iota + backupRemove +) + +type backupRequest struct { + buf *SharedBuffer + reqType backupRequestType +} + +var requestedBackups map[*SharedBuffer]bool func init() { - BackupCompleteChan = make(chan *SharedBuffer, 10) + requestedBackups = make(map[*SharedBuffer]bool) } func (b *SharedBuffer) RequestBackup() { - if !b.RequestedBackup { - select { - case backupRequestChan <- b: - default: - // channel is full + backupRequestChan <- backupRequest{buf: b, reqType: backupCreate} +} + +func (b *SharedBuffer) CancelBackup() { + backupRequestChan <- backupRequest{buf: b, reqType: backupRemove} +} + +func handleBackupRequest(br backupRequest) { + switch br.reqType { + case backupCreate: + // schedule periodic backup + requestedBackups[br.buf] = true + case backupRemove: + br.buf.RemoveBackup() + delete(requestedBackups, br.buf) + } +} + +func periodicBackup() { + for buf := range requestedBackups { + err := buf.Backup() + if err == nil { + delete(requestedBackups, buf) } - b.RequestedBackup = true } } @@ -77,9 +106,6 @@ func (b *SharedBuffer) Backup() error { name := util.DetermineEscapePath(backupdir, b.AbsPath) if _, err := os.Stat(name); errors.Is(err, fs.ErrNotExist) { _, err = b.overwriteFile(name) - if err == nil { - BackupCompleteChan <- b - } return err } @@ -95,8 +121,6 @@ func (b *SharedBuffer) Backup() error { return err } - BackupCompleteChan <- b - return err } diff --git a/internal/buffer/buffer.go b/internal/buffer/buffer.go index 9bd7ecfa..bc33abfd 100644 --- a/internal/buffer/buffer.go +++ b/internal/buffer/buffer.go @@ -13,7 +13,6 @@ import ( "strconv" "strings" "sync" - "sync/atomic" "time" luar "layeh.com/gopher-luar" @@ -101,7 +100,6 @@ type SharedBuffer struct { diffLock sync.RWMutex diff map[int]DiffStatus - RequestedBackup bool forceKeepBackup bool // ReloadDisabled allows the user to disable reloads if they @@ -123,8 +121,6 @@ type SharedBuffer struct { // Hash of the original buffer -- empty if fastdirty is on origHash [md5.Size]byte - - fini int32 } func (b *SharedBuffer) insert(pos Loc, value []byte) { @@ -495,13 +491,11 @@ func (b *Buffer) Fini() { if !b.Modified() { b.Serialize() } - b.RemoveBackup() + b.CancelBackup() if b.Type == BTStdout { fmt.Fprint(util.Stdout, string(b.Bytes())) } - - atomic.StoreInt32(&(b.fini), int32(1)) } // GetName returns the name that should be displayed in the statusline diff --git a/internal/buffer/save.go b/internal/buffer/save.go index aaab39ba..d2cd3533 100644 --- a/internal/buffer/save.go +++ b/internal/buffer/save.go @@ -11,7 +11,6 @@ import ( "os/signal" "path/filepath" "runtime" - "sync/atomic" "time" "unicode" @@ -47,11 +46,14 @@ type saveRequest struct { } var saveRequestChan chan saveRequest -var backupRequestChan chan *SharedBuffer +var backupRequestChan chan backupRequest func init() { - saveRequestChan = make(chan saveRequest, 10) - backupRequestChan = make(chan *SharedBuffer, 10) + // Both saveRequestChan and backupRequestChan need to be non-buffered + // so the save/backup goroutine receives both save and backup requests + // in the same order the main goroutine sends them. + saveRequestChan = make(chan saveRequest) + backupRequestChan = make(chan backupRequest) go func() { duration := backupSeconds * float64(time.Second) @@ -62,14 +64,10 @@ func init() { case sr := <-saveRequestChan: size, err := sr.buf.safeWrite(sr.path, sr.withSudo, sr.newFile) sr.saveResponseChan <- saveResponse{size, err} + case br := <-backupRequestChan: + handleBackupRequest(br) case <-backupTicker.C: - for len(backupRequestChan) > 0 { - b := <-backupRequestChan - bfini := atomic.LoadInt32(&(b.fini)) != 0 - if !bfini { - b.Backup() - } - } + periodicBackup() } } }() @@ -380,6 +378,9 @@ func (b *SharedBuffer) safeWrite(path string, withSudo bool, newFile bool) (int, return 0, err } + // Backup saved, so cancel pending periodic backup, if any + delete(requestedBackups, b) + b.forceKeepBackup = true size := 0 {