Merge pull request #64 from semaphoreci/rl/fix-too-many-open-files

ralopes · web-flow · commit c22f991cb2a3 · 2023-08-16T11:20:25.000-03:00
Fix too many open files error
diff --git a/cmd/compile.go b/cmd/compile.go
@@ -18,7 +18,6 @@ limitations under the License.
 
 import (
 	"encoding/json"
-	"io/ioutil"
 	"os"
 
 	"github.com/semaphoreci/test-results/pkg/cli"
@@ -51,12 +50,14 @@ var compileCmd = &cobra.Command{
 			return err
 		}
 
-		dirPath, err := ioutil.TempDir("", "test-results-*")
+		dirPath, err := os.MkdirTemp("", "test-results-*")
 
 		if err != nil {
 			return err
 		}
 
+		defer os.RemoveAll(dirPath)
+
 		for _, path := range paths {
 			parser, err := cli.FindParser(path, cmd)
 			if err != nil {
@@ -73,7 +74,7 @@ var compileCmd = &cobra.Command{
 				return err
 			}
 
-			tmpFile, err := ioutil.TempFile(dirPath, "result-*.json")
+			tmpFile, err := os.CreateTemp(dirPath, "result-*.json")
 			if err != nil {
 				return err
 			}
@@ -100,8 +101,6 @@ var compileCmd = &cobra.Command{
 			return err
 		}
 
-		defer os.RemoveAll(dirPath)
-
 		return nil
 	},
 }
diff --git a/cmd/gen-pipeline-report.go b/cmd/gen-pipeline-report.go
@@ -18,7 +18,6 @@ limitations under the License.
 
 import (
 	"encoding/json"
-	"io/ioutil"
 	"os"
 	"path"
 
@@ -53,7 +52,7 @@ var genPipelineReportCmd = &cobra.Command{
 		}
 
 		if len(args) == 0 {
-			dir, err = ioutil.TempDir("", "test-results")
+			dir, err = os.MkdirTemp("", "test-results")
 			if err != nil {
 				logger.Error("Creating temporary directory failed %v", err)
 				return err
diff --git a/cmd/publish.go b/cmd/publish.go
@@ -19,7 +19,6 @@ limitations under the License.
 import (
 	"encoding/json"
 	"fmt"
-	"io/ioutil"
 	"os"
 	"path"
 
@@ -50,12 +49,14 @@ var publishCmd = &cobra.Command{
 			return err
 		}
 
-		dirPath, err := ioutil.TempDir("", "test-results-*")
+		dirPath, err := os.MkdirTemp("", "test-results-*")
 
 		if err != nil {
 			return err
 		}
 
+		defer os.RemoveAll(dirPath)
+
 		for _, path := range paths {
 			parser, err := cli.FindParser(path, cmd)
 			if err != nil {
@@ -72,7 +73,7 @@ var publishCmd = &cobra.Command{
 				return err
 			}
 
-			tmpFile, err := ioutil.TempFile(dirPath, "result-*.json")
+			tmpFile, err := os.CreateTemp(dirPath, "result-*.json")
 			if err != nil {
 				return err
 			}
@@ -99,6 +100,8 @@ var publishCmd = &cobra.Command{
 			return err
 		}
 
+		defer os.Remove(fileName)
+
 		_, err = cli.PushArtifacts("job", fileName, path.Join("test-results", "junit.json"), cmd)
 		if err != nil {
 			return err
@@ -150,8 +153,6 @@ var publishCmd = &cobra.Command{
 			}
 		}
 
-		defer os.Remove(fileName)
-
 		return nil
 	},
 }
diff --git a/pkg/cli/cli.go b/pkg/cli/cli.go
@@ -3,8 +3,8 @@ package cli
 import (
 	"encoding/json"
 	"fmt"
+	"io"
 	"io/fs"
-	"io/ioutil"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -196,17 +196,20 @@ func Marshal(testResults parser.Result) ([]byte, error) {
 // WriteToFile saves data to given file
 func WriteToFile(data []byte, path string) (string, error) {
 	file, err := os.Create(path) // #nosec
+	defer file.Close()
 
 	if err != nil {
 		logger.Error("Opening file %s: %v", path, err)
 		return "", err
 	}
+
 	return writeToFile(data, file)
 }
 
 // WriteToTmpFile saves data to temporary file
 func WriteToTmpFile(data []byte) (string, error) {
-	file, err := ioutil.TempFile("", "test-results")
+	file, err := os.CreateTemp("", "test-results")
+	defer file.Close()
 
 	if err != nil {
 		logger.Error("Opening file %s: %v", file.Name(), err)
@@ -225,6 +228,11 @@ func writeToFile(data []byte, file *os.File) (string, error) {
 		return "", err
 	}
 
+	if err = file.Sync(); err != nil {
+		logger.Error("Output file write failed: %v", err)
+		return "", err
+	}
+
 	return file.Name(), nil
 }
 
@@ -385,10 +393,8 @@ func Load(path string) (*parser.Result, error) {
 		return nil, err
 	}
 	defer jsonFile.Close() // #nosec
-	if err != nil {
-		return nil, err
-	}
-	bytes, err := ioutil.ReadAll(jsonFile)
+
+	bytes, err := io.ReadAll(jsonFile)
 	if err != nil {
 		return nil, err
 	}
diff --git a/pkg/cli/cli_test.go b/pkg/cli/cli_test.go
@@ -1,8 +1,10 @@
 package cli_test
 
 import (
+	"encoding/json"
 	"fmt"
-	"io/ioutil"
+	"github.com/semaphoreci/test-results/pkg/parser"
+	"github.com/stretchr/testify/require"
 	"os"
 	"testing"
 
@@ -41,10 +43,22 @@ func Test_LoadFiles(t *testing.T) {
 
 		os.RemoveAll(dirPath)
 	})
+
+	t.Run("with big directory", func(t *testing.T) {
+		dirPath := generateDirWithFilesAndNestedDir(t, 2600, 3)
+		assert.NotEmpty(t, dirPath)
+
+		paths, err := cli.LoadFiles([]string{dirPath}, ".xml")
+		assert.Len(t, paths, 2600, "should return correct number of files")
+		assert.Nil(t, err, "should not throw error")
+
+		os.RemoveAll(dirPath)
+	})
+
 }
 
 func generateFile(t *testing.T) string {
-	filePath, err := ioutil.TempFile("", "file-*.xml")
+	filePath, err := os.CreateTemp("", "file-*.xml")
 	if err != nil {
 		t.Errorf("Failed to create temporary file: %v", err)
 	}
@@ -53,22 +67,116 @@ func generateFile(t *testing.T) string {
 }
 
 func generateDir(t *testing.T) string {
-	dirPath, err := ioutil.TempDir("", "")
+	return generateDirWithFilesAndNestedDir(t, 5, 3)
+}
+
+func generateDirWithFilesAndNestedDir(t *testing.T, fNumber, dirNumber int) string {
+	dirPath, err := os.MkdirTemp("", "")
 	assert.Nil(t, err)
 
-	nestedDir, err := ioutil.TempDir(dirPath, "xml-*")
+	nestedDir, err := os.MkdirTemp(dirPath, "xml-*")
 	assert.Nil(t, err)
 
-	for i := 0; i < 5; i++ {
-		_, err = ioutil.TempFile(nestedDir, "file-*.xml")
+	for i := 0; i < fNumber; i++ {
+		_, err = os.CreateTemp(nestedDir, "file-*.xml")
 		assert.Nil(t, err)
 	}
 
-	nestedDir, _ = ioutil.TempDir(dirPath, "json-*")
-	for i := 0; i < 3; i++ {
-		_, err := ioutil.TempFile(nestedDir, "file-*.json")
+	nestedDir, _ = os.MkdirTemp(dirPath, "json-*")
+	for i := 0; i < dirNumber; i++ {
+		_, err := os.MkdirTemp(nestedDir, "file-*.json")
 		assert.Nil(t, err)
 	}
 
 	return dirPath
 }
+
+func TestWriteToTmpFile(t *testing.T) {
+	tr := parser.TestResults{
+		ID:         "1234",
+		Name:       "Test",
+		Framework:  "JUnit",
+		IsDisabled: false,
+		Suites:     nil,
+		Summary: parser.Summary{
+			Total:    10,
+			Passed:   5,
+			Skipped:  0,
+			Error:    0,
+			Failed:   5,
+			Disabled: 0,
+			Duration: 360,
+		},
+		Status:        "OK",
+		StatusMessage: "Test",
+	}
+	result := parser.Result{TestResults: []parser.TestResults{tr}}
+	jsonData, _ := json.Marshal(&result)
+
+	t.Run("Write to one tmp file", func(t *testing.T) {
+		file, err := cli.WriteToTmpFile(jsonData)
+		assert.NoError(t, err)
+		os.Remove(file)
+	})
+
+	t.Run("Write to three thousand tmp files", func(t *testing.T) {
+		fileNumber := 3000
+		files := make([]string, 0, fileNumber)
+
+		for i := 0; i < fileNumber; i++ {
+			file, err := cli.WriteToTmpFile(jsonData)
+			assert.NoError(t, err)
+
+			files = append(files, file)
+		}
+
+		for _, file := range files {
+			os.Remove(file)
+		}
+	})
+}
+
+func TestWriteToFile(t *testing.T) {
+	tr := parser.TestResults{
+		ID:         "1234",
+		Name:       "Test",
+		Framework:  "JUnit",
+		IsDisabled: false,
+		Suites:     nil,
+		Summary: parser.Summary{
+			Total:    10,
+			Passed:   5,
+			Skipped:  0,
+			Error:    0,
+			Failed:   5,
+			Disabled: 0,
+			Duration: 360,
+		},
+		Status:        "OK",
+		StatusMessage: "Test",
+	}
+	result := parser.Result{TestResults: []parser.TestResults{tr}}
+	jsonData, _ := json.Marshal(&result)
+
+	t.Run("Write to one file", func(t *testing.T) {
+		file, err := cli.WriteToFile(jsonData, "out")
+		assert.NoError(t, err)
+		os.Remove(file)
+	})
+
+	t.Run("Write to three thousand files", func(t *testing.T) {
+		fileNumber := 3000
+		dirPath, err := os.MkdirTemp("", "test-results-*")
+		require.NoError(t, err)
+
+		defer os.RemoveAll(dirPath)
+
+		for i := 0; i < fileNumber; i++ {
+			tmpFile, err := os.CreateTemp(dirPath, "result-*.json")
+			require.NoError(t, err)
+
+			_, err = cli.WriteToFile(jsonData, tmpFile.Name())
+			require.NoError(t, err)
+		}
+	})
+}
diff --git a/pkg/fileloader/fileloader.go b/pkg/fileloader/fileloader.go
@@ -3,7 +3,7 @@ package fileloader
 import (
 	"bytes"
 	"io"
-	"io/ioutil"
+	"os"
 
 	"github.com/semaphoreci/test-results/pkg/logger"
 )
@@ -17,19 +17,23 @@ func Load(path string, reader *bytes.Reader) (*bytes.Reader, bool) {
 
 // Ensure puts reader data into temporary created file.
 func Ensure(reader *bytes.Reader) (fileName string) {
-	file, err := ioutil.TempFile("", "")
+	file, err := os.CreateTemp("", "")
 	if err != nil {
 		panic(err)
 	}
 
+	defer file.Close() // #nosec
+
 	fileName = file.Name()
 
 	_, err = reader.WriteTo(file)
 	if err != nil {
 		panic(err)
 	}
 
-	defer file.Close() // #nosec
+	if err = file.Sync(); err != nil {
+		panic(err)
+	}
 
 	return
 }

Original file line number	Diff line number	Diff line change
`@@ -18,7 +18,6 @@ limitations under the License.`
`18`	`18`
`19`	`19`	`import (`
`20`	`20`	`"encoding/json"`
`21`		`- "io/ioutil"`
`22`	`21`	`"os"`
`23`	`22`
`24`	`23`	`"github.com/semaphoreci/test-results/pkg/cli"`
`@@ -51,12 +50,14 @@ var compileCmd = &cobra.Command{`
`51`	`50`	`return err`
`52`	`51`	`}`
`53`	`52`
`54`		`- dirPath, err := ioutil.TempDir("", "test-results-*")`
	`53`	`+ dirPath, err := os.MkdirTemp("", "test-results-*")`
`55`	`54`
`56`	`55`	`if err != nil {`
`57`	`56`	`return err`
`58`	`57`	`}`
`59`	`58`
	`59`	`+ defer os.RemoveAll(dirPath)`
	`60`	`+`
`60`	`61`	`for _, path := range paths {`
`61`	`62`	`parser, err := cli.FindParser(path, cmd)`
`62`	`63`	`if err != nil {`
`@@ -73,7 +74,7 @@ var compileCmd = &cobra.Command{`
`73`	`74`	`return err`
`74`	`75`	`}`
`75`	`76`
`76`		`- tmpFile, err := ioutil.TempFile(dirPath, "result-*.json")`
	`77`	`+ tmpFile, err := os.CreateTemp(dirPath, "result-*.json")`
`77`	`78`	`if err != nil {`
`78`	`79`	`return err`
`79`	`80`	`}`
`@@ -100,8 +101,6 @@ var compileCmd = &cobra.Command{`
`100`	`101`	`return err`
`101`	`102`	`}`
`102`	`103`
`103`		`- defer os.RemoveAll(dirPath)`
`104`		`-`
`105`	`104`	`return nil`
`106`	`105`	`},`
`107`	`106`	`}`
Original file line number	Diff line number	Diff line change
`@@ -19,7 +19,6 @@ limitations under the License.`
`19`	`19`	`import (`
`20`	`20`	`"encoding/json"`
`21`	`21`	`"fmt"`
`22`		`- "io/ioutil"`
`23`	`22`	`"os"`
`24`	`23`	`"path"`
`25`	`24`
`@@ -50,12 +49,14 @@ var publishCmd = &cobra.Command{`
`50`	`49`	`return err`
`51`	`50`	`}`
`52`	`51`
`53`		`- dirPath, err := ioutil.TempDir("", "test-results-*")`
	`52`	`+ dirPath, err := os.MkdirTemp("", "test-results-*")`
`54`	`53`
`55`	`54`	`if err != nil {`
`56`	`55`	`return err`
`57`	`56`	`}`
`58`	`57`
	`58`	`+ defer os.RemoveAll(dirPath)`
	`59`	`+`
`59`	`60`	`for _, path := range paths {`
`60`	`61`	`parser, err := cli.FindParser(path, cmd)`
`61`	`62`	`if err != nil {`
`@@ -72,7 +73,7 @@ var publishCmd = &cobra.Command{`
`72`	`73`	`return err`
`73`	`74`	`}`
`74`	`75`
`75`		`- tmpFile, err := ioutil.TempFile(dirPath, "result-*.json")`
	`76`	`+ tmpFile, err := os.CreateTemp(dirPath, "result-*.json")`
`76`	`77`	`if err != nil {`
`77`	`78`	`return err`
`78`	`79`	`}`
`@@ -99,6 +100,8 @@ var publishCmd = &cobra.Command{`
`99`	`100`	`return err`
`100`	`101`	`}`
`101`	`102`
	`103`	`+ defer os.Remove(fileName)`
	`104`	`+`
`102`	`105`	`_, err = cli.PushArtifacts("job", fileName, path.Join("test-results", "junit.json"), cmd)`
`103`	`106`	`if err != nil {`
`104`	`107`	`return err`
`@@ -150,8 +153,6 @@ var publishCmd = &cobra.Command{`
`150`	`153`	`}`
`151`	`154`	`}`
`152`	`155`
`153`		`- defer os.Remove(fileName)`
`154`		`-`
`155`	`156`	`return nil`
`156`	`157`	`},`
`157`	`158`	`}`