refactoring process tests + adding lock on run function

app/converter/process/process.go

@@ -5,6 +5,7 @@ import (
 	"bytes"
 	"fmt"
 	"os/exec"
+	"sync"
 	"text/template"
 	"time"
 
@@ -12,11 +13,60 @@ import (
 	gfile "github.com/thecodingmachine/gotenberg/app/converter/file"
 )
 
-var commandsConfig *config.CommandsConfig
+type runner struct {
+	mu             sync.Mutex
+	commandsConfig *config.CommandsConfig
+}
+
+var forest = &runner{}
+
+type commandTimeoutError struct {
+	command string
+	timeout int
+}
+
+func (e *commandTimeoutError) Error() string {
+	return fmt.Sprintf("The command '%s' has reached the %d second(s) timeout", e.command, e.timeout)
+}
+
+// run runs the given command. If timeout is reached or
+// something bad happened, returns an error.
+func (r *runner) run(command string, timeout int) error {
+	r.mu.Lock()
+	defer r.mu.Unlock()
+
+	cmd := exec.Command("/bin/sh", "-c", command)
+	if err := cmd.Start(); err != nil {
+		return err
+	}
+
+	done := make(chan error, 1)
+	go func() {
+		done <- cmd.Wait()
+	}()
+
+	// wait for the process to finish or kill it after a timeout.
+	select {
+	case <-time.After(time.Duration(timeout) * time.Second):
+		if err := cmd.Process.Kill(); err != nil {
+			return err
+		}
+		return &commandTimeoutError{
+			command: command,
+			timeout: timeout,
+		}
+	case err := <-done:
+		if err != nil {
+			return err
+		}
+
+		return nil
+	}
+}
 
 // Load loads the commands configuration coming from the application configuration.
 func Load(config *config.CommandsConfig) {
-	commandsConfig = config
+	forest.commandsConfig = config
 }
 
 // conversionData will be applied to the data-driven templates of conversions commands.
@@ -47,16 +97,16 @@ func Unconv(workingDir string, file *gfile.File) (string, error) {
 
 	switch file.Type {
 	case gfile.MarkdownType:
-		cmdTimeout = commandsConfig.Markdown.Timeout
+		cmdTimeout = forest.commandsConfig.Markdown.Timeout
-		cmdTemplate = commandsConfig.Markdown.Template
+		cmdTemplate = forest.commandsConfig.Markdown.Template
 		break
 	case gfile.HTMLType:
-		cmdTimeout = commandsConfig.HTML.Timeout
+		cmdTimeout = forest.commandsConfig.HTML.Timeout
-		cmdTemplate = commandsConfig.HTML.Template
+		cmdTemplate = forest.commandsConfig.HTML.Template
 		break
 	case gfile.OfficeType:
-		cmdTimeout = commandsConfig.Office.Timeout
+		cmdTimeout = forest.commandsConfig.Office.Timeout
-		cmdTemplate = commandsConfig.Office.Template
+		cmdTemplate = forest.commandsConfig.Office.Template
 		break
 	default:
 		return "", &impossibleConversionError{}
@@ -67,7 +117,7 @@ func Unconv(workingDir string, file *gfile.File) (string, error) {
 		return "", err
 	}
 
-	err := run(data.String(), cmdTimeout)
+	err := forest.run(data.String(), cmdTimeout)
 	if err != nil {
 		return "", err
 	}
@@ -88,59 +138,18 @@ func Merge(workingDir string, filesPaths []string) (string, error) {
 		ResultFilePath: gfile.MakeFilePath(workingDir, ".pdf"),
 	}
 
-	cmdTimeout := commandsConfig.Merge.Timeout
+	cmdTimeout := forest.commandsConfig.Merge.Timeout
-	cmdTemplate := commandsConfig.Merge.Template
+	cmdTemplate := forest.commandsConfig.Merge.Template
 
 	var data bytes.Buffer
 	if err := cmdTemplate.Execute(&data, cmdData); err != nil {
 		return "", err
 	}
 
-	err := run(data.String(), cmdTimeout)
+	err := forest.run(data.String(), cmdTimeout)
 	if err != nil {
 		return "", err
 	}
 
 	return cmdData.ResultFilePath, nil
 }
-
-type commandTimeoutError struct {
-	command string
-	timeout int
-}
-
-func (e *commandTimeoutError) Error() string {
-	return fmt.Sprintf("The command '%s' has reached the %d second(s) timeout", e.command, e.timeout)
-}
-
-// run runs the given command. If timeout is reached or
-// something bad happened, returns an error.
-func run(command string, timeout int) error {
-	cmd := exec.Command("/bin/sh", "-c", command)
-	if err := cmd.Start(); err != nil {
-		return err
-	}
-
-	done := make(chan error, 1)
-	go func() {
-		done <- cmd.Wait()
-	}()
-
-	// wait for the process to finish or kill it after a timeout.
-	select {
-	case <-time.After(time.Duration(timeout) * time.Second):
-		if err := cmd.Process.Kill(); err != nil {
-			return err
-		}
-		return &commandTimeoutError{
-			command: command,
-			timeout: timeout,
-		}
-	case err := <-done:
-		if err != nil {
-			return err
-		}
-
-		return nil
-	}
-}

app/converter/process/process_test.go

@@ -22,100 +22,111 @@ func makeFile(workingDir string, fileName string) *gfile.File {
 	return f
 }
 
+func loadCommandConfigs(configurationFilePath string) {
+	path, _ := filepath.Abs(configurationFilePath)
+	c, _ := config.NewAppConfig(path)
+	Load(c.CommandsConfig)
+}
+
 func TestLoad(t *testing.T) {
 	path, _ := filepath.Abs("../../../_tests/configurations/gotenberg.yml")
 	c, _ := config.NewAppConfig(path)
 	Load(c.CommandsConfig)
 
-	if c.CommandsConfig != commandsConfig {
+	if c.CommandsConfig != forest.commandsConfig {
-		t.Error("Commands configuration should have loaded correctly!")
+		t.Error("Commands configuration should have been loaded correctly")
+	}
+}
+
+func TestRun(t *testing.T) {
+	var cmd string
+
+	// case 1: uses a simple command.
+	cmd = "echo Hello world"
+	if err := forest.run(cmd, 30); err != nil {
+		t.Errorf("Command '%s' should have worked", cmd)
+	}
+
+	// case 2: uses a simple command but with an unsuitable timeout.
+	cmd = "sleep 5"
+	if err := forest.run(cmd, 0); err == nil {
+		t.Errorf("Command '%s' should not have worked", cmd)
+	}
+
+	// case 3: uses a broken command.
+	cmd = "helloworld"
+	if err := forest.run(cmd, 30); err == nil {
+		t.Errorf("Command '%s' should not have worked", cmd)
 	}
 }
 
 func TestUnconv(t *testing.T) {
-	path, _ := filepath.Abs("../../../_tests/configurations/gotenberg.yml")
+	var file *gfile.File
-	c, _ := config.NewAppConfig(path)
-	Load(c.CommandsConfig)
 
 	workingDir := "test"
 	os.Mkdir(workingDir, 0666)
 
+	loadCommandConfigs("../../../_tests/configurations/gotenberg.yml")
+
 	// case 1: uses an HTML file type.
-	if _, err := Unconv(workingDir, makeFile(workingDir, "file.html")); err != nil {
+	file = makeFile(workingDir, "file.html")
-		t.Error("HTML conversion to PDF should have worked!")
+	if _, err := Unconv(workingDir, file); err != nil {
+		t.Errorf("Converting '%s' to PDF should have worked", file.Path)
 	}
 
 	// case 2: uses an Office file type.
-	if _, err := Unconv(workingDir, makeFile(workingDir, "file.docx")); err != nil {
+	file = makeFile(workingDir, "file.docx")
-		t.Error("Office conversion to PDF should have worked!")
+	if _, err := Unconv(workingDir, file); err != nil {
+		t.Errorf("Converting '%s' to PDF should have worked", file.Path)
 	}
 
 	// case 3: uses a PDF file type.
-	if _, err := Unconv(workingDir, makeFile(workingDir, "file.pdf")); err == nil {
+	file = makeFile(workingDir, "file.pdf")
-		t.Error("PDF conversion to PDF should not have worked!")
+	if _, err := Unconv(workingDir, file); err == nil {
+		t.Errorf("Converting '%s' to PDF should not have worked", file.Path)
 	}
 
+	loadCommandConfigs("../../../_tests/configurations/timeout-gotenberg.yml")
+
 	// case 4: uses a command with an unsuitable timeout.
-	path, _ = filepath.Abs("../../../_tests/configurations/timeout-gotenberg.yml")
+	file = makeFile(workingDir, "file.docx")
-	c, _ = config.NewAppConfig(path)
-	Load(c.CommandsConfig)
 	if _, err := Unconv(workingDir, makeFile(workingDir, "file.docx")); err == nil {
-		t.Error("Office conversion to PDF should have reached timeout!")
+		t.Errorf("Converting '%s' to PDF should have reached timeout", file.Path)
 	}
 
 	os.RemoveAll(workingDir)
 }
 
 func TestMerge(t *testing.T) {
-	path, _ := filepath.Abs("../../../_tests/configurations/gotenberg.yml")
-	c, _ := config.NewAppConfig(path)
-	Load(c.CommandsConfig)
-
 	workingDir := "test"
 	os.Mkdir(workingDir, 0666)
 
+	loadCommandConfigs("../../../_tests/configurations/gotenberg.yml")
+
 	var filesPaths []string
-	path, _ = filepath.Abs("../../../_tests/file.pdf")
+	path, _ := filepath.Abs("../../../_tests/file.pdf")
 	filesPaths = append(filesPaths, path)
 	filesPaths = append(filesPaths, path)
 
 	// case 1: simple merge.
 	if _, err := Merge(workingDir, filesPaths); err != nil {
-		t.Error("Merge should have worked!")
+		t.Error("Merge should have worked")
 	}
 
+	loadCommandConfigs("../../../_tests/configurations/timeout-gotenberg.yml")
+
 	// case 2: uses a command with an unsuitable timeout.
-	path, _ = filepath.Abs("../../../_tests/configurations/timeout-gotenberg.yml")
-	c, _ = config.NewAppConfig(path)
-	Load(c.CommandsConfig)
 	if _, err := Merge(workingDir, filesPaths); err == nil {
-		t.Error("Merge should have reached timeout!")
+		t.Error("Merge should have reached timeout")
 	}
 
 	os.RemoveAll(workingDir)
 }
 
-func TestRun(t *testing.T) {
-	// case 1: uses a simple command.
-	if err := run("echo Hello world", 30); err != nil {
-		t.Error("Command should have worked!")
-	}
-
-	// case 2: uses a simple command but with an unsuitable timeout.
-	if err := run("sleep 5", 0); err == nil {
-		t.Error("Command should not have worked!")
-	}
-
-	// case 3: uses a broken command.
-	if err := run("helloworld", 30); err == nil {
-		t.Error("Command should not have worked!")
-	}
-}
-
 func TestImpossibleConversionError(t *testing.T) {
 	err := &impossibleConversionError{}
 	if err.Error() != impossibleConversionErrorMessage {
-		t.Errorf("Error returned a wrong message: got %s want %s", err.Error(), impossibleConversionErrorMessage)
+		t.Errorf("Error returned a wrong message: got '%s' want '%s'", err.Error(), impossibleConversionErrorMessage)
 	}
 }
 
@@ -124,10 +135,8 @@ func TestCommandTimeoutError(t *testing.T) {
 		command: "echo hello",
 		timeout: 30,
 	}
-
 	expected := fmt.Sprintf("The command '%s' has reached the %d second(s) timeout", err.command, err.timeout)
-
 	if err.Error() != expected {
-		t.Errorf("Error returned a wrong message: got %s want %s", err.Error(), expected)
+		t.Errorf("Error returned a wrong message: got '%s' want '%s'", err.Error(), expected)
 	}
 }