resource: Fix multi-threaded image processing issue

When doing something like this with the same image from a partial used in, say, both the home page and the single page:

```bash
{{ with $img }}
{{ $big := .Fill "1024x512 top" }}
{{ $small := $big.Resize "512x" }}
{{ end }}
```

There would be timing issues making Hugo in some cases try to process the same image with the same instructions in parallel.

You would experience errors of type:

```bash
png: invalid format: not enough pixel data
```

This commit works around that by adding a mutex per image. This should also improve the performance, sligthly, as it avoids duplicate work.

The current workaround before this fix is to always operate on the original:

```bash
{{ with $img }}
{{ $big := .Fill "1024x512 top" }}
{{ $small := .Fill "512x256 top" }}
{{ end }}
```

Fixes #4404

diff --git a/resource/image.go b/resource/image.go
index 1914b3c044e0498e478622f94787ec4443c4661a..208a0e9fb0f43df4c71e76fc111f01b74c01e3e9 100644 (file)
--- a/resource/image.go
+++ b/resource/image.go
@@ -112,6 +112,9 @@ type Image struct {
 
        copyToDestinationInit sync.Once
 
+       // Lock used when creating alternate versions of this image.
+       createMu sync.Mutex
+
        imaging *Imaging
 
        hash string

diff --git a/resource/image_cache.go b/resource/image_cache.go
index 5720fb62322c7748d8d320ff4cc3c9e5be45c4a0..250155db1a7441bd85bfa75a5e0cb7bd8c25e0dc 100644 (file)
--- a/resource/image_cache.go
+++ b/resource/image_cache.go
@@ -28,7 +28,8 @@ type imageCache struct {
        absCacheDir   string
        pathSpec      *helpers.PathSpec
        mu            sync.RWMutex
-       store         map[string]*Image
+
+       store map[string]*Image
 }
 
 func (c *imageCache) isInCache(key string) bool {
@@ -69,6 +70,11 @@ func (c *imageCache) getOrCreate(
        }
 
        // Now look in the file cache.
+       // Multiple Go routines can invoke same operation on the same image, so
+       // we need to make sure this is serialized per source image.
+       parent.createMu.Lock()
+       defer parent.createMu.Unlock()
+
        cacheFilename := filepath.Join(c.absCacheDir, key)
 
        // The definition of this counter is not that we have processed that amount

diff --git a/resource/image_test.go b/resource/image_test.go
index de706b0ac1d86af007ec8b1745ca65c940db8d34..e981a208fb1d46f9ffa4669251833a083f37b3f0 100644 (file)
--- a/resource/image_test.go
+++ b/resource/image_test.go
@@ -15,8 +15,12 @@ package resource
 
 import (
        "fmt"
+       "math/rand"
+       "strconv"
        "testing"
 
+       "sync"
+
        "github.com/stretchr/testify/require"
 )
 
@@ -141,6 +145,51 @@ func TestImageTransformLongFilename(t *testing.T) {
        assert.Equal("/a/_hu59e56ffff1bc1d8d122b1403d34e039f_90587_c876768085288f41211f768147ba2647.jpg", resized.RelPermalink())
 }
 
+func TestImageTransformConcurrent(t *testing.T) {
+
+       var wg sync.WaitGroup
+
+       assert := require.New(t)
+
+       spec := newTestResourceOsFs(assert)
+
+       image := fetchImageForSpec(spec, assert, "sunset.jpg")
+
+       for i := 0; i < 4; i++ {
+               wg.Add(1)
+               go func(id int) {
+                       defer wg.Done()
+                       for j := 0; j < 5; j++ {
+                               img := image
+                               for k := 0; k < 2; k++ {
+                                       r1, err := img.Resize(fmt.Sprintf("%dx", id-k))
+                                       if err != nil {
+                                               t.Fatal(err)
+                                       }
+
+                                       if r1.Width() != id-k {
+                                               t.Fatalf("Width: %d:%d", r1.Width(), j)
+                                       }
+
+                                       r2, err := r1.Resize(fmt.Sprintf("%dx", id-k-1))
+                                       if err != nil {
+                                               t.Fatal(err)
+                                       }
+
+                                       _, err = r2.decodeSource()
+                                       if err != nil {
+                                               t.Fatal("Err decode:", err)
+                                       }
+
+                                       img = r1
+                               }
+                       }
+               }(i + 20)
+       }
+
+       wg.Wait()
+}
+
 func TestDecodeImaging(t *testing.T) {
        assert := require.New(t)
        m := map[string]interface{}{
@@ -208,3 +257,22 @@ func TestImageWithMetadata(t *testing.T) {
        assert.Equal("Sunset #1", resized.Name())
 
 }
+
+func BenchmarkResizeParallel(b *testing.B) {
+       assert := require.New(b)
+       img := fetchSunset(assert)
+
+       b.RunParallel(func(pb *testing.PB) {
+               for pb.Next() {
+                       w := rand.Intn(10) + 10
+                       resized, err := img.Resize(strconv.Itoa(w) + "x")
+                       if err != nil {
+                               b.Fatal(err)
+                       }
+                       _, err = resized.Resize(strconv.Itoa(w-1) + "x")
+                       if err != nil {
+                               b.Fatal(err)
+                       }
+               }
+       })
+}

diff --git a/resource/testhelpers_test.go b/resource/testhelpers_test.go
index 2b543ab641f28d00eb20fa25fc41503f5aa6512f..7f6d4f3076db757fe4fe0e20d2717693b14c575f 100644 (file)
--- a/resource/testhelpers_test.go
+++ b/resource/testhelpers_test.go
@@ -6,8 +6,11 @@ import (
 
        "image"
        "io"
+       "io/ioutil"
        "os"
        "path"
+       "runtime"
+       "strings"
 
        "github.com/gohugoio/hugo/helpers"
        "github.com/gohugoio/hugo/hugofs"
@@ -45,17 +48,53 @@ func newTestResourceSpecForBaseURL(assert *require.Assertions, baseURL string) *
        return spec
 }
 
+func newTestResourceOsFs(assert *require.Assertions) *Spec {
+       cfg := viper.New()
+       cfg.Set("baseURL", "https://example.com")
+
+       workDir, err := ioutil.TempDir("", "hugores")
+
+       if runtime.GOOS == "darwin" && !strings.HasPrefix(workDir, "/private") {
+               // To get the entry folder in line with the rest. This its a little bit
+               // mysterious, but so be it.
+               workDir = "/private" + workDir
+       }
+
+       contentDir := "base"
+       cfg.Set("workingDir", workDir)
+       cfg.Set("contentDir", contentDir)
+       cfg.Set("resourceDir", filepath.Join(workDir, "res"))
+
+       fs := hugofs.NewFrom(hugofs.Os, cfg)
+       fs.Destination = &afero.MemMapFs{}
+
+       s, err := helpers.NewPathSpec(fs, cfg)
+
+       assert.NoError(err)
+
+       spec, err := NewSpec(s, media.DefaultTypes)
+       assert.NoError(err)
+       return spec
+
+}
+
 func fetchSunset(assert *require.Assertions) *Image {
        return fetchImage(assert, "sunset.jpg")
 }
 
 func fetchImage(assert *require.Assertions, name string) *Image {
+       spec := newTestResourceSpec(assert)
+       return fetchImageForSpec(spec, assert, name)
+}
+
+func fetchImageForSpec(spec *Spec, assert *require.Assertions, name string) *Image {
        src, err := os.Open("testdata/" + name)
        assert.NoError(err)
 
-       spec := newTestResourceSpec(assert)
+       workingDir := spec.Cfg.GetString("workingDir")
+       f := filepath.Join(workingDir, name)
 
-       out, err := spec.Fs.Source.Create("/b/" + name)
+       out, err := spec.Fs.Source.Create(f)
        assert.NoError(err)
        _, err = io.Copy(out, src)
        out.Close()
@@ -66,11 +105,10 @@ func fetchImage(assert *require.Assertions, name string) *Image {
                return path.Join("/a", s)
        }
 
-       r, err := spec.NewResourceFromFilename(factory, "/public", "/b/"+name, name)
+       r, err := spec.NewResourceFromFilename(factory, "/public", f, name)
        assert.NoError(err)
        assert.IsType(&Image{}, r)
        return r.(*Image)
-
 }
 
 func assertFileCache(assert *require.Assertions, fs *hugofs.Fs, filename string, width, height int) {