-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
"Check failed: result.second" in Node 14.1 via Sharp 0.24 #2196
Comments
Thanks for the notification Aaron, I wasn't aware of this. Based on those discussions I would guess many native addons are going to break with Node.js 14, even those using N-API, as sharp v0.25.0+ does. Comments on the original PR at nodejs/node#30782 also suggest N-API won't protect against this. It's unclear to me right now exactly how to work around this, e.g. copying memory before creating a Buffer to ensure every Buffer has a unique location will have performance implications. |
If you're able to provide some code to reproduce this that would be very useful as I've not yet been able to do so. The |
So far I've only seen it on alpine. Hopefully I'll have repro script soon. |
Was finally able to repro on Mac, but it takes quite a while. Current code is quite complex, so will take some time to narrow down to minimal repro.
|
Thanks, the upstream Node.js PR nodejs/node#33321 should fix the race condition we're seeing. |
I could repro this issue in 14.2 in under a minute reliably. 14.3 has been solid in 10 mins of identical load. There seems to be doubt that it's resolved all cases, but so far so good for sharp. nodejs/node#33321 |
I was hitting this randomly, and not able to reproduce on a specific input, suggesting it is something like a race condition. Downgrading to node@12 fixed the problem for me. Will upgrade to node 14.3 when its released on brew. |
That's great news. From what I can tell the remaining problems being discussed do not affect sharp/libvips as it does not create multiple Buffer instances pointing to the same memory. |
Not sure if this has been resolved in Sharp 0.25+, but wanted to make aware this is apparently a breaking change in node. nodejs/node#32463 nodejs/node#31061
Are you able to provide a minimal, standalone code sample, without other dependencies, that demonstrates this problem? Not at this time as it was only reproducible in production so it had to be rolled back quickly.
Are you able to provide a sample image that helps explain the problem? Not at this time. Will investigate further.
What is the output of running
npx envinfo --binaries --system
? Don't have since was forced to do fast rollback.Will update if I'm able to reproduce outside of production environment.
The text was updated successfully, but these errors were encountered: