Misc improvements (#41)

* Only set -common_args option if there are additional args to be set
* Fix typo
* Wait for the exiftool command to exit when closing
* Run tests that call NewExiftool() in parallel
* Also run CI on pull requests
* Don't block waiting for exiftool to exit

.github/workflows/ci.yml

@@ -1,5 +1,7 @@
 name: go-exiftool-ci
-on: push
+on:
+  - push
+  - pull_request
 jobs:
   build:
     runs-on: ubuntu-latest
@@ -19,4 +21,4 @@ jobs:
       - name: Tests
         run: go test -coverprofile=coverage.txt -covermode=atomic ./...
       - name: Coverage publication
-        run: bash <(curl -s https://codecov.io/bash)
+        run: bash <(curl -s https://codecov.io/bash)

exiftool.go

@@ -4,21 +4,24 @@ import (
 	"bufio"
 	"bytes"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"os"
 	"os/exec"
 	"sync"
-
-	"errors"
+	"time"
 )
 
 var executeArg = "-execute"
-var initArgs = []string{"-stay_open", "True", "-@", "-", "-common_args"}
+var initArgs = []string{"-stay_open", "True", "-@", "-"}
 var extractArgs = []string{"-j"}
 var closeArgs = []string{"-stay_open", "False", executeArg}
 var readyTokenLen = len(readyToken)
 
+// WaitTimeout specifies the duration to wait for exiftool to exit when closing before timing out
+var WaitTimeout = time.Second
+
 // ErrNotExist is a sentinel error for non existing file
 var ErrNotExist = errors.New("file does not exist")
 
@@ -33,6 +36,7 @@ type Exiftool struct {
 	bufferMaxSize   int
 	extraInitArgs   []string
 	exiftoolBinPath string
+	cmd             *exec.Cmd
 }
 
 // NewExiftool instanciates a new Exiftool with configuration functions. If anything went
@@ -48,16 +52,21 @@ func NewExiftool(opts ...func(*Exiftool) error) (*Exiftool, error) {
 		}
 	}
 
-	args := append(initArgs, e.extraInitArgs...)
-	cmd := exec.Command(e.exiftoolBinPath, args...)
+	args := append([]string(nil), initArgs...)
+	if len(e.extraInitArgs) > 0 {
+		args = append(args, "-common_args")
+		args = append(args, e.extraInitArgs...)
+	}
+
+	e.cmd = exec.Command(e.exiftoolBinPath, args...)
 	r, w := io.Pipe()
 	e.stdMergedOut = r
 
-	cmd.Stdout = w
-	cmd.Stderr = w
+	e.cmd.Stdout = w
+	e.cmd.Stderr = w
 
 	var err error
-	if e.stdin, err = cmd.StdinPipe(); err != nil {
+	if e.stdin, err = e.cmd.StdinPipe(); err != nil {
 		return nil, fmt.Errorf("error when piping stdin: %w", err)
 	}
 
@@ -67,8 +76,8 @@ func NewExiftool(opts ...func(*Exiftool) error) (*Exiftool, error) {
 	}
 	e.scanMergedOut.Split(splitReadyToken)
 
-	if err = cmd.Start(); err != nil {
-		return nil, fmt.Errorf("error when executing commande: %w", err)
+	if err = e.cmd.Start(); err != nil {
+		return nil, fmt.Errorf("error when executing command: %w", err)
 	}
 
 	return &e, nil
@@ -95,6 +104,24 @@ func (e *Exiftool) Close() error {
 		errs = append(errs, fmt.Errorf("error while closing stdin: %w", err))
 	}
 
+	ch := make(chan struct{})
+	go func() {
+		if e.cmd != nil {
+			if err := e.cmd.Wait(); err != nil {
+				errs = append(errs, fmt.Errorf("error while waiting for exiftool to exit: %w", err))
+			}
+		}
+		ch <- struct{}{}
+		close(ch)
+	}()
+
+	// Wait for wait to finish or timeout
+	select {
+	case <- ch:
+	case <- time.After(WaitTimeout):
+		errs = append(errs, errors.New("Timed out waiting for exiftool to exit"))
+	}
+
 	if len(errs) > 0 {
 		return fmt.Errorf("error while closing exiftool: %v", errs)
 	}
@@ -229,4 +256,4 @@ func SetExiftoolBinaryPath(p string) func(*Exiftool) error {
 		e.exiftoolBinPath = p
 		return nil
 	}
-}
+}

exiftool_test.go

@@ -11,13 +11,17 @@ import (
 )
 
 func TestNewExiftoolEmpty(t *testing.T) {
+	t.Parallel()
+
 	e, err := NewExiftool()
 	assert.Nil(t, err)
 
 	defer e.Close()
 }
 
 func TestNewExifToolOptOk(t *testing.T) {
+	t.Parallel()
+
 	var exec1, exec2 bool
 
 	f1 := func(*Exiftool) error {
@@ -40,6 +44,8 @@ func TestNewExifToolOptOk(t *testing.T) {
 }
 
 func TestNewExifToolOptKo(t *testing.T) {
+	t.Parallel()
+
 	f := func(*Exiftool) error {
 		return fmt.Errorf("err")
 	}
@@ -61,6 +67,8 @@ func TestSingleExtract(t *testing.T) {
 	for _, tc := range tcs {
 		tc := tc // Pin variable
 		t.Run(tc.tcID, func(t *testing.T) {
+			t.Parallel()
+
 			e, err := NewExiftool()
 			assert.Nilf(t, err, "error not nil: %v", err)
 			defer e.Close()
@@ -75,6 +83,8 @@ func TestSingleExtract(t *testing.T) {
 }
 
 func TestMultiExtract(t *testing.T) {
+	t.Parallel()
+
 	e, err := NewExiftool()
 
 	assert.Nilf(t, err, "error not nil: %v", err)
@@ -168,6 +178,16 @@ func TestCloseErrorOnStdout(t *testing.T) {
 	assert.True(t, wClosed)
 }
 
+func TestCloseExifToolNominal(t *testing.T) {
+	t.Parallel()
+
+	e, err := NewExiftool()
+
+	assert.Nil(t, err)
+	assert.Nil(t, e.Close())
+}
+
+
 type readWriteCloserMock struct {
 	writeInt int
 	writeErr error
@@ -191,6 +211,8 @@ func (e readWriteCloserMock) Close() error {
 }
 
 func TestBuffer(t *testing.T) {
+	t.Parallel()
+
 	e, err := NewExiftool()
 	assert.Nil(t, err)
 	defer e.Close()
@@ -204,6 +226,8 @@ func TestBuffer(t *testing.T) {
 }
 
 func TestNewExifTool_WithBuffer(t *testing.T) {
+	t.Parallel()
+
 	buf := make([]byte, 128*1000)
 	e, err := NewExiftool(Buffer(buf, 64*1000))
 	assert.Nil(t, err)
@@ -215,6 +239,8 @@ func TestNewExifTool_WithBuffer(t *testing.T) {
 }
 
 func TestCharset(t *testing.T) {
+	t.Parallel()
+
 	e, err := NewExiftool()
 	assert.Nil(t, err)
 	defer e.Close()
@@ -227,6 +253,8 @@ func TestCharset(t *testing.T) {
 }
 
 func TestNewExifTool_WithCharset(t *testing.T) {
+	t.Parallel()
+
 	e, err := NewExiftool(Charset("filename=utf8"))
 	assert.Nil(t, err)
 	defer e.Close()
@@ -237,6 +265,8 @@ func TestNewExifTool_WithCharset(t *testing.T) {
 }
 
 func TestNoPrintConversion(t *testing.T) {
+	t.Parallel()
+
 	e, err := NewExiftool(NoPrintConversion())
 	assert.Nil(t, err)
 	defer e.Close()
@@ -256,6 +286,8 @@ func TestNoPrintConversion(t *testing.T) {
 }
 
 func TestExtractEmbedded(t *testing.T) {
+	t.Parallel()
+
 	eWithout, err := NewExiftool()
 	assert.Nil(t, err)
 	defer eWithout.Close()
@@ -277,6 +309,8 @@ func TestExtractEmbedded(t *testing.T) {
 }
 
 func TestExtractAllBinaryMetadata(t *testing.T) {
+	t.Parallel()
+
 	eWithout, err := NewExiftool()
 	assert.Nil(t, err)
 	defer eWithout.Close()
@@ -299,6 +333,8 @@ func TestExtractAllBinaryMetadata(t *testing.T) {
 }
 
 func TestSetExiftoolBinaryPath(t *testing.T) {
+	t.Parallel()
+
 	// default
 	eDefault, err := NewExiftool()
 	assert.Nil(t, err)
@@ -322,4 +358,4 @@ func TestSetExiftoolBinaryPath(t *testing.T) {
 	_, err = NewExiftool(SetExiftoolBinaryPath("/non/existing/path"))
 	assert.NotNil(t, err)
 
-}
+}