Merge branch 'main' into add-lua-filter

Author: arl

CHANGELOG.md

@@ -60,8 +60,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Fix building on windows [#115](https://github.com/AdRoll/baker/issues/115)
 - Fix `list_test` with file URI to be compatible with windows paths [#117](https://github.com/AdRoll/baker/pull/117)
 - Fix `List` input, some `io.Reader`'s were left opened [#118](https://github.com/AdRoll/baker/pull/118)
-- `split_writer.Close` leaves some file descriptor open [#119](https://github.com/AdRoll/baker/pull/119)
 - Fix some bugs in `s3.s3UploadFile` [#120](https://github.com/AdRoll/baker/pull/120)
+- `SplitWriter` leaves some file descriptors open [#119](https://github.com/AdRoll/baker/pull/119) and [#121](https://github.com/AdRoll/baker/pull/121)
 
 ### Maintenance
 

pkg/splitwriter/split_writer.go

@@ -191,23 +191,31 @@ func doNextSplit(f *os.File, dir, next string, off int64) (*os.File, error) {
 	nextpath := filepath.Join(dir, next)
 	nextf, err := open(nextpath)
 	if err != nil {
+		closeFiles(f)
 		return nil, err
 	}
 
 	if _, err := f.Seek(off, io.SeekStart); err != nil {
+		closeFiles(f, nextf)
 		return nil, err
 	}
 
 	if _, err := io.Copy(nextf, f); err != nil {
+		closeFiles(f, nextf)
 		return nil, err
 	}
 
 	// Conclude the current split
 	if err := f.Truncate(off); err != nil {
+		closeFiles(f, nextf)
 		return nil, err
 	}
 
-	return nextf, f.Close()
+	if err := f.Close(); err != nil {
+		closeFiles(nextf)
+		return nil, err
+	}
+	return nextf, nil
 }
 
 // doFirstSplit creates the first splits of f, at offset off.
@@ -222,36 +230,45 @@ func doFirstSplit(f *os.File, dir, fname, next1 string, off int64) (*os.File, er
 	// Open the 2 first parts
 	f1, err := open(next1path)
 	if err != nil {
+		closeFiles(f)
 		return nil, err
 	}
 	f2, err := open(next2path)
 	if err != nil {
+		closeFiles(f, f1)
 		return nil, err
 	}
 
 	// Split the original file into 2 parts
 	if _, err := f.Seek(0, io.SeekStart); err != nil {
+		closeFiles(f, f1, f2)
 		return nil, err
 	}
 
 	if _, err := io.CopyN(f1, f, off); err != nil {
+		closeFiles(f, f1, f2)
 		return nil, err
 	}
 
 	if _, err := io.Copy(f2, f); err != nil {
+		closeFiles(f, f1, f2)
 		return nil, err
 	}
 
 	// Close the 2 parts and remove the original file
 	if err := f.Close(); err != nil {
+		closeFiles(f1, f2)
 		return nil, err
 	}
-
 	if err := f1.Close(); err != nil {
+		closeFiles(f2)
 		return nil, err
 	}
-
-	return f2, os.Remove(filepath.Join(dir, fname))
+	if err := os.Remove(filepath.Join(dir, fname)); err != nil {
+		closeFiles(f2)
+		return nil, err
+	}
+	return f2, nil
 }
 
 var splitFnameRx = regexp.MustCompile(`(\S+)-part-(\d+)(.*)`)
@@ -298,3 +315,13 @@ func fileExists(fname string) bool {
 	}
 	return !info.IsDir()
 }
+
+// closeFiles closes a list of files and returns the first error (if any).
+func closeFiles(fs ...*os.File) (cerr error) {
+	for _, f := range fs {
+		if err := f.Close(); err != nil && cerr == nil {
+			cerr = err
+		}
+	}
+	return
+}

pkg/splitwriter/split_writer_test.go

@@ -1,9 +1,14 @@
 package splitwriter
 
 import (
+	"fmt"
+	"io"
 	"io/ioutil"
 	"os"
 	"path"
+	"path/filepath"
+	"runtime"
+	"strings"
 	"testing"
 )
 
@@ -178,12 +183,12 @@ func TestSplitWriter(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			for f, data := range tt.previous {
-				if err := ioutil.WriteFile(path.Join(dir, f), []byte(data), 0666); err != nil {
+				if err := ioutil.WriteFile(filepath.Join(dir, f), []byte(data), 0666); err != nil {
 					t.Fatal(err)
 				}
 			}
 
-			path := path.Join(dir, tt.fname)
+			path := filepath.Join(dir, tt.fname)
 			w, err := New(path, int64(tt.maxsize), int64(tt.bufsize))
 			if err != nil {
 				t.Fatal(err)
@@ -201,6 +206,215 @@ func TestSplitWriter(t *testing.T) {
 	}
 }
 
+func TestSplitWriterErr(t *testing.T) {
+	type swConf struct {
+		path             string // path to file
+		maxsize, bufsize int    // splitWriter parameters
+		data             string // data to write in fname
+	}
+	initSplitWriter := func(t *testing.T, cf swConf) io.WriteCloser {
+		w, err := New(cf.path, int64(cf.maxsize), int64(cf.bufsize))
+		if err != nil {
+			t.Fatal(err)
+		}
+		if _, err := w.Write([]byte(cf.data)); err != nil {
+			t.Fatal(err)
+		}
+		return w
+	}
+
+	t.Run("doFirstSplit f1 error ", func(t *testing.T) {
+		dir := t.TempDir()
+
+		cfg := swConf{
+			path:    filepath.Join(dir, "big-file-1-split"),
+			maxsize: 12,
+			bufsize: 4,
+			data:    "012\n34567890123",
+		}
+		w := initSplitWriter(t, cfg)
+
+		// Create the first part whitout write permission
+		f1, _ := os.OpenFile(cfg.path+"-part-1", os.O_CREATE, 0444)
+		f1.Close()
+
+		if err := w.Close(); err == nil {
+			t.Errorf("get nil, want error")
+		}
+
+		// Check that all files where closed
+		if err := os.RemoveAll(dir); err != nil {
+			t.Fatal(err)
+		}
+	})
+
+	t.Run("doFirstSplit f2 err", func(t *testing.T) {
+		dir := t.TempDir()
+
+		cfg := swConf{
+			path:    filepath.Join(dir, "big-file-1-split"),
+			maxsize: 12,
+			bufsize: 4,
+			data:    "012\n34567890123",
+		}
+		w := initSplitWriter(t, cfg)
+
+		// Create the second part whitout write permission
+		f2, _ := os.OpenFile(cfg.path+"-part-2", os.O_CREATE, 0444)
+		f2.Close()
+
+		if err := w.Close(); err == nil {
+			t.Errorf("get nil, want error")
+		}
+
+		// Check that all files where closed
+		if err := os.RemoveAll(dir); err != nil {
+			t.Fatal(err)
+		}
+	})
+
+	t.Run("doFirstSplit error on remove", func(t *testing.T) {
+		if "windows" != runtime.GOOS {
+			t.Skip("Cannot produce an error on file remove on a not windows platform")
+		}
+
+		dir := t.TempDir()
+
+		cfg := swConf{
+			path:    filepath.Join(dir, "big-file-1-split"),
+			maxsize: 12,
+			bufsize: 4,
+			data:    "012\n34567890123",
+		}
+		w := initSplitWriter(t, cfg)
+
+		// Open the log file to prevent cancellation during w.Close()
+		f, _ := os.OpenFile(cfg.path, os.O_CREATE|os.O_RDWR, 0)
+
+		if err := w.Close(); err == nil {
+			t.Errorf("get nil, want error")
+		}
+
+		// Check that all files where closed
+		f.Close()
+		if err := os.RemoveAll(dir); err != nil {
+			t.Fatal(err)
+		}
+	})
+
+	t.Run("doNextSplit f error", func(t *testing.T) {
+		dir := t.TempDir()
+
+		cfg := swConf{
+			path:    filepath.Join(dir, "big-file-ext-2-split"),
+			maxsize: 6,
+			bufsize: 4,
+			data:    "012\n3456\n7890123",
+		}
+		w := initSplitWriter(t, cfg)
+
+		// Create the third part whitout write permission
+		f2, _ := os.OpenFile(cfg.path+"-part-3", os.O_CREATE, 0444)
+		f2.Close()
+
+		if err := w.Close(); err == nil {
+			t.Errorf("get nil, want error")
+		}
+
+		// Check that all files where closed
+		if err := os.RemoveAll(dir); err != nil {
+			t.Fatal(err)
+		}
+	})
+
+}
+
+func TestCloseFiles(t *testing.T) {
+	tests := []struct {
+		name   string   // test name
+		fnames []string // fname to create
+		close  []int    // indeces of file to close
+	}{
+		{
+			name:   "good 1",
+			fnames: []string{"test0"},
+			close:  []int{}, // want no error
+		},
+		{
+			name:   "good 2",
+			fnames: []string{"test0", "test1"},
+		},
+		{
+			name:   "good 3",
+			fnames: []string{"test0", "test1", "test2"},
+		},
+		{
+			name:   "bad 1",
+			fnames: []string{"test0", "test1", "test2"},
+			close:  []int{0}, // want error
+		},
+		{
+			name:   "bad 2",
+			fnames: []string{"test0", "test1", "test2"},
+			close:  []int{1},
+		},
+		{
+			name:   "bad 3",
+			fnames: []string{"test0", "test1", "test2"},
+			close:  []int{2},
+		},
+		{
+			name:   "bad 1,3",
+			fnames: []string{"test0", "test1", "test2"},
+			close:  []int{1, 2},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			dir := t.TempDir()
+
+			files := make([]*os.File, 0, 3)
+			for _, fname := range tt.fnames {
+				f, _ := os.OpenFile(filepath.Join(dir, fname), os.O_CREATE|os.O_RDWR, 0)
+				files = append(files, f)
+			}
+
+			for _, i := range tt.close {
+				files[i].Close()
+			}
+
+			err := closeFiles(files...)
+
+			// Check returned error
+			if len(tt.close) == 0 {
+				// Want no error
+				if err != nil {
+					t.Errorf("get %v, want nil", err)
+				}
+			} else {
+				// Want error
+				if err == nil {
+					t.Errorf("get nil, want error")
+				} else {
+					// Check we return the first error
+					fname := fmt.Sprintf("test%d", tt.close[0])
+					if !strings.Contains(err.Error(), fname) {
+						t.Errorf("error %v not contains %q", err, fname)
+					}
+				}
+			}
+
+			// Check that all files were closed
+			for _, f := range files {
+				err := f.Close()
+				if err == nil {
+					t.Errorf("file %q not close", f.Name())
+				}
+			}
+		})
+	}
+}
+
 func Test_nextSplit(t *testing.T) {
 	tests := []struct {
 		fname     string