fix(gatsby-plugin-sharp): don't write to same temporary time multiple…

… times at a same time (#12927)

Memoization key for tracedSVG was not entirely correct and we were trying to write to temporary file multiple times. This was causing those temporary files to become corrupted in dependencies of `potrace` were crashing while trying to read those corrupted input files (there was a lot of concurrency, so that's why those crashes were very random - sometimes timing was bad and we would end up . This _should_ fix memoization key and avoid writing to same file more than once - there is also extra memoization layer just for that, so if general memoization key for `traceSVG` function fails, there is another memoization that checks just temporary file location.

This was happening most likely when there were copies of same image in multiple places (as memoization key was using `absolutePath` before).

I've moved some code around (mostly because I needed to export some trace SVG related functions for added tests). I'll try to mark places in removed code that did change.

related issue #8301

packages/gatsby-plugin-sharp/src/__tests__/trace-svg.js

@@ -0,0 +1,217 @@
+jest.mock(`sharp`, () => {
+  const sharp = path => {
+    const pipeline = {
+      rotate: () => pipeline,
+      resize: () => pipeline,
+      png: () => pipeline,
+      jpeg: () => pipeline,
+      toFile: (_, cb) => cb(),
+    }
+    return pipeline
+  }
+
+  sharp.simd = () => {}
+
+  return sharp
+})
+
+jest.mock(`potrace`, () => {
+  return {
+    trace: (_, _2, cb) => cb(null, ``),
+    Potrace: {
+      TURNPOLICY_MAJORITY: `wat`,
+    },
+  }
+})
+
+const path = require(`path`)
+
+const traceSVGHelpers = require(`../trace-svg`)
+
+const notMemoizedtraceSVG = jest.spyOn(traceSVGHelpers, `notMemoizedtraceSVG`)
+const notMemoizedPrepareTraceSVGInputFile = jest.spyOn(
+  traceSVGHelpers,
+  `notMemoizedPrepareTraceSVGInputFile`
+)
+// note that we started spying on not memoized functions first
+// now we recreate memoized functions that will use function we just started
+// spying on
+traceSVGHelpers.createMemoizedFunctions()
+const memoizedTraceSVG = jest.spyOn(traceSVGHelpers, `memoizedTraceSVG`)
+const memoizedPrepareTraceSVGInputFile = jest.spyOn(
+  traceSVGHelpers,
+  `memoizedPrepareTraceSVGInputFile`
+)
+
+const { traceSVG } = require(`../`)
+
+function getFileObject(absolutePath, name = path.parse(absolutePath).name) {
+  return {
+    id: `${absolutePath} absPath of file`,
+    name: name,
+    absolutePath,
+    extension: `png`,
+    internal: {
+      contentDigest: `1234`,
+    },
+  }
+}
+
+describe(`traceSVG memoization`, () => {
+  const file = getFileObject(path.join(__dirname, `images/test.png`))
+  const copyOfFile = getFileObject(path.join(__dirname, `images/test-copy.png`))
+  const differentFile = getFileObject(
+    path.join(__dirname, `images/different.png`)
+  )
+  differentFile.internal.contentDigest = `4321`
+
+  beforeEach(() => {
+    traceSVGHelpers.clearMemoizeCaches()
+    memoizedTraceSVG.mockClear()
+    notMemoizedtraceSVG.mockClear()
+    memoizedPrepareTraceSVGInputFile.mockClear()
+    notMemoizedPrepareTraceSVGInputFile.mockClear()
+  })
+
+  it(`Baseline`, async () => {
+    await traceSVG({
+      file,
+    })
+
+    expect(memoizedTraceSVG).toBeCalledTimes(1)
+    expect(notMemoizedtraceSVG).toBeCalledTimes(1)
+    expect(memoizedPrepareTraceSVGInputFile).toBeCalledTimes(1)
+    expect(notMemoizedPrepareTraceSVGInputFile).toBeCalledTimes(1)
+  })
+
+  it(`Is memoizing results for same args`, async () => {
+    await traceSVG({
+      file,
+    })
+
+    await traceSVG({
+      file,
+    })
+
+    expect(memoizedTraceSVG).toBeCalledTimes(2)
+    expect(notMemoizedtraceSVG).toBeCalledTimes(1)
+    expect(memoizedPrepareTraceSVGInputFile).toBeCalledTimes(1)
+    expect(notMemoizedPrepareTraceSVGInputFile).toBeCalledTimes(1)
+  })
+
+  it(`Is calling functions with same input file when params change`, async () => {
+    await traceSVG({
+      file,
+      args: {
+        color: `red`,
+      },
+      fileArgs: {
+        width: 400,
+      },
+    })
+    await traceSVG({
+      file,
+      args: {
+        color: `blue`,
+      },
+      fileArgs: {
+        width: 400,
+      },
+    })
+    await traceSVG({
+      file,
+      args: {
+        color: `red`,
+      },
+      fileArgs: {
+        width: 200,
+      },
+    })
+    await traceSVG({
+      file,
+      args: {
+        color: `blue`,
+      },
+      fileArgs: {
+        width: 200,
+      },
+    })
+    await traceSVG({
+      file: differentFile,
+      args: {
+        color: `red`,
+      },
+      fileArgs: {
+        width: 400,
+      },
+    })
+
+    expect(memoizedTraceSVG).toBeCalledTimes(5)
+    expect(notMemoizedtraceSVG).toBeCalledTimes(5)
+    expect(memoizedPrepareTraceSVGInputFile).toBeCalledTimes(5)
+    // trace svg should be actually created just 3 times
+    // because it's affected just by `fileArgs`, and not `args`
+    // this makes sure we don't try to write to same input file multiple times
+    expect(notMemoizedPrepareTraceSVGInputFile).toBeCalledTimes(3)
+    expect(notMemoizedPrepareTraceSVGInputFile).toHaveBeenNthCalledWith(
+      1,
+      expect.objectContaining({
+        file,
+        options: expect.objectContaining({
+          width: 400,
+        }),
+      })
+    )
+    expect(notMemoizedPrepareTraceSVGInputFile).toHaveBeenNthCalledWith(
+      2,
+      expect.objectContaining({
+        file,
+        options: expect.objectContaining({
+          width: 200,
+        }),
+      })
+    )
+    expect(notMemoizedPrepareTraceSVGInputFile).toHaveBeenNthCalledWith(
+      3,
+      expect.objectContaining({
+        file: differentFile,
+        options: expect.objectContaining({
+          width: 400,
+        }),
+      })
+    )
+
+    const usedTmpFilePaths = notMemoizedPrepareTraceSVGInputFile.mock.calls.map(
+      args => args[0].tmpFilePath
+    )
+
+    // tmpFilePath was always unique
+    expect(usedTmpFilePaths.length).toBe(new Set(usedTmpFilePaths).size)
+  })
+
+  it(`Use memoized results for file copies`, async () => {
+    await traceSVG({
+      file,
+      args: {
+        color: `red`,
+      },
+      fileArgs: {
+        width: 400,
+      },
+    })
+    await traceSVG({
+      file: copyOfFile,
+      args: {
+        color: `red`,
+      },
+      fileArgs: {
+        width: 400,
+      },
+    })
+
+    expect(memoizedTraceSVG).toBeCalledTimes(2)
+    expect(notMemoizedtraceSVG).toBeCalledTimes(1)
+    expect(memoizedPrepareTraceSVGInputFile).toBeCalledTimes(1)
+    expect(notMemoizedPrepareTraceSVGInputFile).toBeCalledTimes(1)
+  })
+})

packages/gatsby-plugin-sharp/src/gatsby-node.js

@@ -1,9 +1,11 @@
 const {
   setBoundActionCreators,
-  setPluginOptions,
   // queue: jobQueue,
   // reportError,
 } = require(`./index`)
+
+const { setPluginOptions } = require(`./plugin-options`)
+
 // const { scheduleJob } = require(`./scheduler`)
 // let normalizedOptions = {}