From d8fdffb55268464d54558d6f9cd3874b612dc7c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B8rn=20Erik=20Pedersen?= Date: Tue, 13 Feb 2018 21:45:51 +0100 Subject: [PATCH] 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 --- resource/image.go | 3 ++ resource/image_cache.go | 8 ++++- resource/image_test.go | 68 ++++++++++++++++++++++++++++++++++++ resource/testhelpers_test.go | 46 +++++++++++++++++++++--- 4 files changed, 120 insertions(+), 5 deletions(-) diff --git a/resource/image.go b/resource/image.go index 1914b3c04..208a0e9fb 100644 --- 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 5720fb623..250155db1 100644 --- 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 de706b0ac..e981a208f 100644 --- 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 2b543ab64..7f6d4f307 100644 --- 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) {