Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(gatsby-plugin-sharp): don't write to same temporary file multiple times at a same time #12927

Merged
merged 2 commits into from
Apr 9, 2019

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Mar 28, 2019

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


const memoizedTraceSVG = _.memoize(
notMemoizedtraceSVG,
({ file, args }) => `${file.absolutePath}${JSON.stringify(args)}`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was changed to

({ file, args, fileArgs }) => `${file.internal.contentDigest}${JSON.stringify(args)}${JSON.stringify(fileArgs)}`

pipeline.toFile(tmpFilePath, (err, info) => {
resolve()
})
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generation of temporary input file was refactored into separate function notMemoizedPrepareTraceSVGInputFile and added memoization on top that use tmpFilePath as key

@tylermenezes
Copy link
Contributor

(This doesn't fully fix #8301 -- there are a bunch of other reasons you could get that error.)

@pieh
Copy link
Contributor Author

pieh commented Mar 29, 2019

@tylermenezes can you expand on that? I know you can get this in bunch of different ways, but what are other things we can fix (if you have that context).

I scanned through comments in that issue and there was lot of guesses, but not a lot of actual details.

I've also added throw if for some reason sharp throws when generating this temporary image file, so this causing problem would be potentially eliminated.

I tried using .jpeg is input file and it worked for me locally.

@tylermenezes
Copy link
Contributor

Basically "media files potrace supports" is a subset of "media files". I'm still not totally sure why my original test case wasn't supported (maybe it was because it also had a colon in the name), but I troubleshooted this as a fix and it didn't solve it.

This is a useful fix for many people, I just think it's premature to call the original issue really fixed, when the error message is so generic that literally any problem will cause it.

@pieh
Copy link
Contributor Author

pieh commented Mar 29, 2019

Basically "media files potrace supports" is a subset of "media files". I'm still not totally sure why my original test case wasn't supported (maybe it was because it also had a colon in the name), but I troubleshooted this as a fix and it didn't solve it.

Can you share your reproduction test case? I'm not sure if this is about .jpeg extension, but I did try using those even before the fix and it was working, so it might more file specific?

This is a useful fix for many people, I just think it's premature to call the original issue really fixed, when the error message is so generic that literally any problem will cause it.

Sure, no argument here, I'll change my PR description to not autoclose linked issue, and will post message there so people update and report back.

@pieh pieh changed the title fix(gatsby-plugin-sharp): should fix cannot read bitmap of undefined fix(gatsby-plugin-sharp): don't write to same temporary time multiple times at a same time Mar 29, 2019
@DSchau
Copy link
Contributor

DSchau commented Mar 29, 2019

@tylermenezes just to be clear, you tried this fix out with your example--and still running into errors?

I know we asked in the original thread, but what would be most helpful here is if we could access that reproduction. Feel free to DM me on twitter if any secret details could be shared. We haven't really had a reproducible example to craft this fix--so anything towards that end will ensure we're able to fix each occurrence of this issue.

Thank you!

@DSchau DSchau added bot: merge on green Gatsbot will merge these PRs automatically when all tests passes status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response labels Apr 9, 2019
@DSchau
Copy link
Contributor

DSchau commented Apr 9, 2019

@tylermenezes a gentle reminder that if you could provide any more detail as to why this fix doesn't work for you (and showing with a reproduction or even more information) that would be very much appreciated.

We think this does fix the issue for most (all?), so if you have evidence to the contrary we'd love to ensure we do fix it for all.

@tylermenezes
Copy link
Contributor

You're welcome to close it if you're convinced it will fix the issue. I didn't save any images that caused errors so I'm not able to provide the same reproduction I had last time.

Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests are going to give me nightmares 😱 Just kidding!

This looks great--let's get it merged in, and we'll monitor errors and issues to see if we see this thing continue to pop up.

@DSchau DSchau merged commit 5e254e2 into gatsbyjs:master Apr 9, 2019
@pieh pieh deleted the potrace-stuff branch April 23, 2019 12:10
@CanRau CanRau changed the title fix(gatsby-plugin-sharp): don't write to same temporary time multiple times at a same time fix(gatsby-plugin-sharp): don't write to same temporary file multiple times at a same time Apr 30, 2019
@marcosilvestroni
Copy link

Hi, Maybe It could help to find an other usecase, I've got the same problem here while I'm running in develop and then try to build the project, and vice versa. I have to clean the directrories to make it works or remove the tracedSVG from my gasby-node.js query while generating pages

@isAlmogK
Copy link

Has this been fixed I'm getting the same issue?

@fraserisland
Copy link
Contributor

fraserisland commented Jan 26, 2021

I'm getting this issue when querying for tracedSVG's, i'm getting different errors depending on the file type. It would be great to find a solution, especially with how beneficial tracedSVG's can be in lighthouse v6 scores against blur-up.

  • PNG's work with no errors.
  • WebP images will crash the builds with: TypeError: Cannot read property 'bitmap' of undefined at Potrace._processLoadedImage.
  • JPEG's throw this error: "VipsJpeg: Premature end of JPEG file\nVipsJpeg: out of order read at line 224\n", however don't crash the build.
"gatsby-source-contentful": "4.4.1",
"gatsby-transformer-sharp": "2.10.1",
"gatsby-plugin-sharp": "2.12.2",
"gatsby": "2.30.3",
"gatsby-image": "2.9.0",

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants