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 zyedidia#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.

Author: dmaluka

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:

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
 }
 

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

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
 	{