Merge branch 'po-safefile-no-ctx' into 'master'

Remove context from safe file

See merge request gitlab-org/gitaly!1369

6 files changed, 115 insertions, 117 deletions

diff --git a/changelogs/unreleased/po-safefile-no-ctx.yml b/changelogs/unreleased/po-safefile-no-ctx.yml
new file mode 100644
index 000000000..c35a92b10
--- /dev/null
+++ b/changelogs/unreleased/po-safefile-no-ctx.yml
@@ -0,0 +1,5 @@
+---
+title: Remove context from safe file
+merge_request: 1369
+author:
+type: fixed
diff --git a/internal/cache/cachedb.go b/internal/cache/cachedb.go
index 9ba6deb25..787137233 100644
--- a/internal/cache/cachedb.go
+++ b/internal/cache/cachedb.go
@@ -83,10 +83,11 @@ func (sdb *StreamDB) PutStream(ctx context.Context, repo *gitalypb.Repository, r
 		return err
 	}
 
-	sf, err := safe.CreateFileWriter(ctx, reqPath)
+	sf, err := safe.CreateFileWriter(reqPath)
 	if err != nil {
 		return err
 	}
+	defer sf.Close()
 
 	n, err := io.Copy(sf, src)
 	if err != nil {
diff --git a/internal/cache/keyer.go b/internal/cache/keyer.go
index c32dac1fe..da9b53b7d 100644
--- a/internal/cache/keyer.go
+++ b/internal/cache/keyer.go
@@ -59,7 +59,7 @@ type lease struct {
 // EndLease will end the lease by removing the pending lease file and updating
 // the key file with the current lease ID.
 func (l lease) EndLease(ctx context.Context) error {
-	_, err := updateLatest(ctx, l.repo)
+	_, err := updateLatest(l.repo)
 	if err != nil {
 		return err
 	}
@@ -74,7 +74,7 @@ func (l lease) EndLease(ctx context.Context) error {
 	return nil
 }
 
-func updateLatest(ctx context.Context, repo *gitalypb.Repository) (string, error) {
+func updateLatest(repo *gitalypb.Repository) (string, error) {
 	repoPath, err := helper.GetRepoPath(repo)
 	if err != nil {
 		return "", err
@@ -85,10 +85,11 @@ func updateLatest(ctx context.Context, repo *gitalypb.Repository) (string, error
 		return "", err
 	}
 
-	latest, err := safe.CreateFileWriter(ctx, lPath)
+	latest, err := safe.CreateFileWriter(lPath)
 	if err != nil {
 		return "", err
 	}
+	defer latest.Close()
 
 	nextGenID := uuid.New().String()
 	if nextGenID == "" {
@@ -162,7 +163,7 @@ func (LeaseKeyer) KeyPath(ctx context.Context, repo *gitalypb.Repository, req pr
 		return "", countErr(ErrPendingExists)
 	}
 
-	genID, err := currentGenID(ctx, repo)
+	genID, err := currentGenID(repo)
 	if err != nil {
 		return "", err
 	}
@@ -242,7 +243,7 @@ func currentLeases(repo *gitalypb.Repository) ([]os.FileInfo, error) {
 	return pendings, nil
 }
 
-func currentGenID(ctx context.Context, repo *gitalypb.Repository) (string, error) {
+func currentGenID(repo *gitalypb.Repository) (string, error) {
 	repoPath, err := helper.GetRepoPath(repo)
 	if err != nil {
 		return "", err
@@ -252,7 +253,7 @@ func currentGenID(ctx context.Context, repo *gitalypb.Repository) (string, error
 	switch {
 	case os.IsNotExist(err):
 		// latest file doesn't exist, so create one
-		return updateLatest(ctx, repo)
+		return updateLatest(repo)
 	case err == nil:
 		return string(latestBytes), nil
 	default:
diff --git a/internal/safe/file_writer.go b/internal/safe/file_writer.go
index 62f6860ee..018fea3c9 100644
--- a/internal/safe/file_writer.go
+++ b/internal/safe/file_writer.go
@@ -1,32 +1,30 @@
 package safe
 
 import (
-	"context"
 	"errors"
 	"fmt"
 	"io/ioutil"
 	"os"
 	"path/filepath"
 	"sync"
+)
 
-	"github.com/sirupsen/logrus"
+var (
+	// ErrAlreadyDone is returned when the safe file has already been closed
+	// or committed
+	ErrAlreadyDone = errors.New("safe file was already committed or closed")
 )
 
 // FileWriter is a thread safe writer that does an atomic write to the target file. It allows one
 // writer at a time to acquire a lock, write the file, and atomically replace the contents of the target file.
 type FileWriter struct {
-	tmpFile   *os.File
-	path      string
-	closeErr  error
-	closeOnce sync.Once
+	tmpFile       *os.File
+	path          string
+	commitOrClose sync.Once
 }
 
 // CreateFileWriter takes path as an absolute path of the target file and creates a new FileWriter by attempting to create a tempfile
-func CreateFileWriter(ctx context.Context, path string) (*FileWriter, error) {
-	if ctx.Done() == nil {
-		return nil, errors.New("context cannot be cancelled")
-	}
-	var err error
+func CreateFileWriter(path string) (*FileWriter, error) {
 	writer := &FileWriter{path: path}
 
 	directory := filepath.Dir(path)
@@ -38,43 +36,43 @@ func CreateFileWriter(ctx context.Context, path string) (*FileWriter, error) {
 
 	writer.tmpFile = tmpFile
 
-	go writer.cleanupOnContextDone(ctx)
-
 	return writer, nil
 }
 
-func (fw *FileWriter) cleanupOnContextDone(ctx context.Context) {
-	<-ctx.Done()
-	if err := fw.cleanup(); err != nil {
-		logrus.WithField("path", fw.path).WithError(err).Error("error when closing FileWriter")
-	}
-}
-
 // Write wraps the temporary file's Write.
 func (fw *FileWriter) Write(p []byte) (n int, err error) {
 	return fw.tmpFile.Write(p)
 }
 
-// Commit will close the temporary file and rename it to the target file name the first call to Commit() will close and
-// delete the temporary file, so subsequenty calls to Commit() are gauaranteed to return an error.
+// Commit will close the temporary file and rename it to the target file name
+// the first call to Commit() will close and delete the temporary file, so
+// subsequenty calls to Commit() are gauaranteed to return an error.
 func (fw *FileWriter) Commit() error {
-	if err := fw.tmpFile.Sync(); err != nil {
-		return fmt.Errorf("syncing temp file: %v", err)
-	}
-
-	if err := fw.close(); err != nil {
-		return fmt.Errorf("closing temp file: %v", err)
-	}
-
-	if err := fw.rename(); err != nil {
-		return fmt.Errorf("renaming temp file: %v", err)
-	}
-
-	if err := fw.syncDir(); err != nil {
-		return fmt.Errorf("syncing dir: %v", err)
-	}
+	err := ErrAlreadyDone
+
+	fw.commitOrClose.Do(func() {
+		if err = fw.tmpFile.Sync(); err != nil {
+			err = fmt.Errorf("syncing temp file: %v", err)
+			return
+		}
+
+		if err = fw.tmpFile.Close(); err != nil {
+			err = fmt.Errorf("closing temp file: %v", err)
+			return
+		}
+
+		if err = fw.rename(); err != nil {
+			err = fmt.Errorf("renaming temp file: %v", err)
+			return
+		}
+
+		if err = fw.syncDir(); err != nil {
+			err = fmt.Errorf("syncing dir: %v", err)
+			return
+		}
+	})
 
-	return nil
+	return err
 }
 
 // rename renames the temporary file to the target file
@@ -93,26 +91,20 @@ func (fw *FileWriter) syncDir() error {
 	return f.Sync()
 }
 
-// cleanup will close the temporary file and remove it.
-func (fw *FileWriter) cleanup() error {
-	var err error
-
-	if err = fw.close(); err != nil {
-		return err
-	}
-
-	if err = os.Remove(fw.tmpFile.Name()); err != nil && !os.IsNotExist(err) {
-		return err
-	}
-
-	return nil
-}
-
-// close uses sync.Once to guarantee that the file gets closed only once
-func (fw *FileWriter) close() error {
-	fw.closeOnce.Do(func() {
-		fw.closeErr = fw.tmpFile.Close()
+// Close will close and remove the temp file artifact iff it exists. If the file
+// was already committed, an ErrAlreadyClosed error will be returned and no
+// changes will be made to the filesystem.
+func (fw *FileWriter) Close() error {
+	err := ErrAlreadyDone
+
+	fw.commitOrClose.Do(func() {
+		if err = fw.tmpFile.Close(); err != nil {
+			return
+		}
+		if err = os.Remove(fw.tmpFile.Name()); err != nil && !os.IsNotExist(err) {
+			return
+		}
 	})
 
-	return fw.closeErr
+	return err
 }
diff --git a/internal/safe/file_writer_test.go b/internal/safe/file_writer_test.go
index a7e281be7..5acf484db 100644
--- a/internal/safe/file_writer_test.go
+++ b/internal/safe/file_writer_test.go
@@ -8,7 +8,6 @@ import (
 	"path/filepath"
 	"sync"
 	"testing"
-	"time"
 
 	"github.com/stretchr/testify/require"
 
@@ -20,12 +19,9 @@ func TestFile(t *testing.T) {
 	dir, cleanup := testhelper.TempDir(t, "", t.Name())
 	defer cleanup()
 
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-
 	filePath := filepath.Join(dir, "test_file_contents")
 	fileContents := "very important contents"
-	file, err := safe.CreateFileWriter(ctx, filePath)
+	file, err := safe.CreateFileWriter(filePath)
 	require.NoError(t, err)
 
 	_, err = io.Copy(file, bytes.NewBufferString(fileContents))
@@ -45,45 +41,6 @@ func TestFile(t *testing.T) {
 	require.Equal(t, filepath.Base(filePath), filesInTempDir[0].Name())
 }
 
-func TestFileContextCancelled(t *testing.T) {
-	dir, cleanup := testhelper.TempDir(t, "", t.Name())
-	defer cleanup()
-
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-
-	filePath := filepath.Join(dir, "test_file_contents")
-	fileContents := "very important contents"
-	file, err := safe.CreateFileWriter(ctx, filePath)
-	require.NoError(t, err)
-
-	_, err = io.Copy(file, bytes.NewBufferString(fileContents))
-	require.NoError(t, err)
-
-	testhelper.AssertFileNotExists(t, filePath)
-
-	files, err := ioutil.ReadDir(dir)
-	require.NoError(t, err)
-	require.Len(t, files, 1, "expect only the temp file to exist")
-	require.Contains(t, files[0].Name(), "test_file_contents", "the one file in the directory should be the temp file")
-
-	cancel()
-
-	testhelper.AssertFileNotExists(t, filePath)
-
-	// wait for the cleanup functions to run
-	for i := 0; i < 10; i++ {
-		time.Sleep(10 * time.Millisecond)
-		filesInTempDir, err := ioutil.ReadDir(dir)
-		require.NoError(t, err)
-		if len(filesInTempDir) > 0 {
-			continue
-		}
-		return
-	}
-	t.Error("directory should not have any files left")
-}
-
 func TestFileRace(t *testing.T) {
 	dir, cleanup := testhelper.TempDir(t, "", t.Name())
 	defer cleanup()
@@ -95,9 +52,7 @@ func TestFileRace(t *testing.T) {
 	for i := 0; i < 10; i++ {
 		wg.Add(1)
 		go func(i int) {
-			ctx, cancel := testhelper.Context()
-			defer cancel()
-			w, err := safe.CreateFileWriter(ctx, filePath)
+			w, err := safe.CreateFileWriter(filePath)
 			require.NoError(t, err)
 			_, err = w.Write([]byte(fmt.Sprintf("message # %d", i)))
 			require.NoError(t, err)
@@ -112,3 +67,50 @@ func TestFileRace(t *testing.T) {
 	require.NoError(t, err)
 	require.Len(t, filesInTempDir, 1, "make sure no other files were written")
 }
+
+func TestFileCloseBeforeCommit(t *testing.T) {
+	dir, cleanup := testhelper.TempDir(t, "", t.Name())
+	defer cleanup()
+
+	dstPath := filepath.Join(dir, "safety_meow")
+	sf, err := safe.CreateFileWriter(dstPath)
+	require.NoError(t, err)
+
+	require.True(t, !dirEmpty(t, dir), "should contain something")
+
+	_, err = sf.Write([]byte("MEOW MEOW MEOW MEOW"))
+	require.NoError(t, err)
+
+	require.NoError(t, sf.Close())
+	require.True(t, dirEmpty(t, dir), "should be empty")
+
+	require.Equal(t, safe.ErrAlreadyDone, sf.Commit())
+}
+
+func TestFileCommitBeforeClose(t *testing.T) {
+	dir, cleanup := testhelper.TempDir(t, "", t.Name())
+	defer cleanup()
+
+	dstPath := filepath.Join(dir, "safety_meow")
+	sf, err := safe.CreateFileWriter(dstPath)
+	require.NoError(t, err)
+
+	require.False(t, dirEmpty(t, dir), "should contain something")
+
+	_, err = sf.Write([]byte("MEOW MEOW MEOW MEOW"))
+	require.NoError(t, err)
+
+	require.NoError(t, sf.Commit())
+	require.FileExists(t, dstPath)
+
+	require.Equal(t, safe.ErrAlreadyDone, sf.Close(),
+		"Close should be impotent after call to commit",
+	)
+	require.FileExists(t, dstPath)
+}
+
+func dirEmpty(t testing.TB, dirPath string) bool {
+	infos, err := ioutil.ReadDir(dirPath)
+	require.NoError(t, err)
+	return len(infos) == 0
+}
diff --git a/internal/storage/metadata.go b/internal/storage/metadata.go
index 90abf74f3..f15a770eb 100644
--- a/internal/storage/metadata.go
+++ b/internal/storage/metadata.go
@@ -1,7 +1,6 @@
 package storage
 
 import (
-	"context"
 	"encoding/json"
 	"os"
 	"path/filepath"
@@ -24,19 +23,17 @@ type Metadata struct {
 
 // WriteMetadataFile marshals and writes a metadata file
 func WriteMetadataFile(storage config.Storage) error {
-	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel()
-
 	path := filepath.Join(storage.Path, metadataFilename)
 
 	if _, err := os.Stat(path); !os.IsNotExist(err) {
 		return err
 	}
 
-	fw, err := safe.CreateFileWriter(ctx, path)
+	fw, err := safe.CreateFileWriter(path)
 	if err != nil {
 		return err
 	}
+	defer fw.Close()
 
 	if err = json.NewEncoder(fw).Encode(&Metadata{
 		GitalyFilesystemID: uuid.New().String(),