properly close files in edge cases in various places

Michael Muré created 2 years ago

Change summary

cache/repo_cache.go             | 17 ++++++++++-------
cache/subcache.go               | 10 ++++++++--
commands/select/select.go       | 18 ++++++++++--------
util/lamport/persisted_clock.go | 11 ++++++++---
4 files changed, 36 insertions(+), 20 deletions(-)

Detailed changes

cache/repo_cache.go

@@ -3,7 +3,6 @@ package cache
 import (
 	"fmt"
 	"io"
-	"io/ioutil"
 	"os"
 	"strconv"
 	"sync"
@@ -181,6 +180,7 @@ func (c *RepoCache) lock(events chan BuildEvent) error {
 	pid := fmt.Sprintf("%d", os.Getpid())
 	_, err = f.Write([]byte(pid))
 	if err != nil {
+		_ = f.Close()
 		return err
 	}
 
@@ -279,11 +279,18 @@ func repoIsAvailable(repo repository.RepoStorage, events chan BuildEvent) error
 
 	if err == nil {
 		// lock file already exist
-		buf, err := ioutil.ReadAll(io.LimitReader(f, 10))
+		buf, err := io.ReadAll(io.LimitReader(f, 10))
 		if err != nil {
+			_ = f.Close()
 			return err
 		}
-		if len(buf) == 10 {
+
+		err = f.Close()
+		if err != nil {
+			return err
+		}
+
+		if len(buf) >= 10 {
 			return fmt.Errorf("the lock file should be < 10 bytes")
 		}
 
@@ -299,10 +306,6 @@ func repoIsAvailable(repo repository.RepoStorage, events chan BuildEvent) error
 		// The lock file is just laying there after a crash, clean it
 
 		events <- BuildEvent{Event: BuildEventRemoveLock}
-		err = f.Close()
-		if err != nil {
-			return err
-		}
 
 		err = repo.LocalStorage().Remove(lockfile)
 		if err != nil {

cache/subcache.go

@@ -101,14 +101,19 @@ func (sc *SubCache[EntityT, ExcerptT, CacheT]) Load() error {
 		return err
 	}
 
-	decoder := gob.NewDecoder(f)
-
 	aux := struct {
 		Version  uint
 		Excerpts map[entity.Id]ExcerptT
 	}{}
 
+	decoder := gob.NewDecoder(f)
 	err = decoder.Decode(&aux)
+	if err != nil {
+		_ = f.Close()
+		return err
+	}
+
+	err = f.Close()
 	if err != nil {
 		return err
 	}
@@ -173,6 +178,7 @@ func (sc *SubCache[EntityT, ExcerptT, CacheT]) write() error {
 
 	_, err = f.Write(data.Bytes())
 	if err != nil {
+		_ = f.Close()
 		return err
 	}
 

commands/select/select.go

@@ -3,7 +3,6 @@ package _select
 import (
 	"fmt"
 	"io"
-	"io/ioutil"
 	"os"
 	"path/filepath"
 
@@ -101,6 +100,7 @@ func Select(repo *cache.RepoCache, namespace string, id entity.Id) error {
 
 	_, err = f.Write([]byte(id.String()))
 	if err != nil {
+		_ = f.Close()
 		return err
 	}
 
@@ -124,11 +124,18 @@ func selected[CacheT cache.CacheEntity](repo *cache.RepoCache, resolver Resolver
 		}
 	}
 
-	buf, err := ioutil.ReadAll(io.LimitReader(f, 100))
+	buf, err := io.ReadAll(io.LimitReader(f, 100))
 	if err != nil {
+		_ = f.Close()
 		return nil, err
 	}
-	if len(buf) == 100 {
+
+	err = f.Close()
+	if err != nil {
+		return nil, err
+	}
+
+	if len(buf) >= 100 {
 		return nil, fmt.Errorf("the select file should be < 100 bytes")
 	}
 
@@ -147,10 +154,5 @@ func selected[CacheT cache.CacheEntity](repo *cache.RepoCache, resolver Resolver
 		return nil, err
 	}
 
-	err = f.Close()
-	if err != nil {
-		return nil, err
-	}
-
 	return &cached, nil
 }

util/lamport/persisted_clock.go

@@ -3,7 +3,7 @@ package lamport
 import (
 	"errors"
 	"fmt"
-	"io/ioutil"
+	"io"
 	"os"
 
 	"github.com/go-git/go-billy/v5"
@@ -77,9 +77,14 @@ func (pc *PersistedClock) read() error {
 	if err != nil {
 		return err
 	}
-	defer f.Close()
 
-	content, err := ioutil.ReadAll(f)
+	content, err := io.ReadAll(f)
+	if err != nil {
+		_ = f.Close()
+		return err
+	}
+
+	err = f.Close()
 	if err != nil {
 		return err
 	}