improving lock mechanism for office conversion: now check if context has timeout when trying to acquire the lock + office printer test

internal/pkg/printer/merge.go

@@ -34,13 +34,13 @@ func NewMergePrinter(logger xlog.Logger, fpaths []string, opts MergePrinterOptio
 
 func (p mergePrinter) Print(destination string) error {
 	const op string = "printer.mergePrinter.Print"
-	logOptions(p.logger, p.opts)
 	/*
 		context.Context may be providen from
 		an officePrinter which needs to merge
 		its result files.
 	*/
 	if p.ctx == nil {
+		logOptions(p.logger, p.opts)
 		ctx, cancel := xcontext.WithTimeout(p.logger, p.opts.WaitTimeout)
 		defer cancel()
 		p.ctx = ctx

internal/pkg/printer/office.go

@@ -5,7 +5,6 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
-	"sync"
 
 	"github.com/thecodingmachine/gotenberg/internal/pkg/xcontext"
 	"github.com/thecodingmachine/gotenberg/internal/pkg/xerror"
@@ -63,6 +62,7 @@ func (p officePrinter) Print(destination string) error {
 			return nil
 		}
 		m := mergePrinter{
+			logger: p.logger,
 			ctx:    ctx,
 			fpaths: fpaths,
 		}
@@ -78,15 +78,10 @@ func (p officePrinter) Print(destination string) error {
 }
 
 // nolint: gochecknoglobals
-var mu sync.Mutex
+var lock = make(chan struct{}, 1)
 
 func unoconv(ctx context.Context, logger xlog.Logger, fpath, destination string, opts OfficePrinterOptions) error {
 	const op string = "printer.unoconv"
-	// TODO check if timeout while waiting for the lock.
-	logger.DebugOp(op, "waiting lock to be released...")
-	mu.Lock()
-	defer mu.Unlock()
-	logger.DebugOp(op, "lock released")
 	resolver := func() error {
 		args := []string{
 			"--format",
@@ -108,10 +103,23 @@ func unoconv(ctx context.Context, logger xlog.Logger, fpath, destination string,
 		xexec.LogBeforeExecute(logger, cmd)
 		return cmd.Run()
 	}
-	if err := resolver(); err != nil {
+	logger.DebugOp(op, "waiting lock to be acquired...")
-		return xerror.New(op, err)
+	select {
+	case lock <- struct{}{}:
+		// lock acquired.
+		logger.DebugOp(op, "lock acquired")
+		if err := resolver(); err != nil {
+			<-lock // we release the lock.
+			return xerror.New(op, err)
+		}
+		<-lock // we release the lock.
+		return nil
+	case <-ctx.Done():
+		// failed to acquire lock before
+		// deadline.
+		logger.DebugOp(op, "failed to acquire lock before context.Context deadline")
+		return xerror.New(op, ctx.Err())
 	}
-	return nil
 }
 
 // Compile-time checks to ensure type implements desired interfaces.

internal/pkg/printer/office_test.go

@@ -0,0 +1,59 @@
+package printer
+
+import (
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/thecodingmachine/gotenberg/internal/pkg/xerror"
+	"github.com/thecodingmachine/gotenberg/internal/pkg/xlog"
+	"github.com/thecodingmachine/gotenberg/test/internalpkg/printertest"
+	"github.com/thecodingmachine/gotenberg/test/internalpkg/xerrortest"
+	"github.com/thecodingmachine/gotenberg/test/internalpkg/xlogtest"
+)
+
+func TestOfficePrinter(t *testing.T) {
+	var (
+		logger xlog.Logger = xlogtest.DebugLogger()
+		fpaths []string    = printertest.OfficeFpaths(t)
+		opts   OfficePrinterOptions
+		dest   string
+		p      Printer
+		err    error
+	)
+	// default options.
+	opts = OfficePrinterOptions{
+		WaitTimeout: 10.0,
+		Landscape:   false,
+	}
+	p = NewOfficePrinter(logger, fpaths, opts)
+	dest = printertest.GenerateDestination()
+	err = p.Print(dest)
+	assert.Nil(t, err)
+	err = os.RemoveAll(dest)
+	assert.Nil(t, err)
+	// options with landscape.
+	opts = OfficePrinterOptions{
+		WaitTimeout: 10.0,
+		Landscape:   true,
+	}
+	p = NewOfficePrinter(logger, fpaths, opts)
+	dest = printertest.GenerateDestination()
+	err = p.Print(dest)
+	assert.Nil(t, err)
+	err = os.RemoveAll(dest)
+	assert.Nil(t, err)
+	// should not be OK as context.Context
+	// should timeout.
+	opts = OfficePrinterOptions{
+		WaitTimeout: 1.0,
+		Landscape:   true,
+	}
+	p = NewOfficePrinter(logger, fpaths, opts)
+	dest = printertest.GenerateDestination()
+	err = p.Print(dest)
+	xerrortest.AssertError(t, err)
+	assert.Equal(t, xerror.TimeoutCode, xerror.Code(err))
+	err = os.RemoveAll(dest)
+	assert.Nil(t, err)
+}

test/internalpkg/printertest/doc.go

@@ -0,0 +1,6 @@
+/*
+Package printertest contains useful
+functions for tests related
+to printer package.
+*/
+package printertest

test/internalpkg/printertest/printertest.go

@@ -0,0 +1,35 @@
+package printertest
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"github.com/thecodingmachine/gotenberg/internal/pkg/xrand"
+)
+
+const testdataDirectoryPath string = "/gotenberg/tests/test/testdata"
+
+// GenerateDestination simply generates
+// a path for a resulting PDF file.
+func GenerateDestination() string {
+	return fmt.Sprintf("/tmp/%s.pdf", xrand.Get())
+}
+
+// OfficeFpaths return the paths
+// of the Office documents used in tests.
+func OfficeFpaths(t *testing.T) []string {
+	return []string{
+		fpath(t, "office", "document.docx"),
+		fpath(t, "office", "document.rtf"),
+		fpath(t, "office", "document.txt"),
+	}
+}
+
+func fpath(t *testing.T, kind, filename string) string {
+	require.NotEmpty(t, kind)
+	require.NotEmpty(t, filename)
+	fpath := fmt.Sprintf("%s/%s/%s", testdataDirectoryPath, kind, filename)
+	require.FileExists(t, fpath)
+	return fpath
+}