zlib: warn before crash on invalid internals usage #16657
Conversation
addaleax
added
npm
nodejs-github-bot
added
c++
I think it should stay, since the combination of |
|
@MylesBorins I have no idea when we should remove it, but no, there’s no rush – this won’t show up if you’re using a broken version. |
src/node_zlib.cc
Outdated
| if (args.Length() == 5) { | ||
| fprintf(stderr, | ||
| "WARNING: You are likely using a version of node-tar or npm that " | ||
| "uses the internals of Node.js in an invalid way.\nPlease use " |
There was a problem hiding this comment.
Micro-nit (totally not blocking): s/uses the internals of Node.js in an invalid way/is incompatible with this version of Node.js/
There was a problem hiding this comment.
I think I prefer the current wording because it gives more of a hint on why the error is occurring. That the versions are incompatible kind of follows from the rest of the message, right?
There was a problem hiding this comment.
I guess I'm trying to avoid "uses the internals of Node.js in an invalid way" as it seems likely to be seen as throwing a bit of shade at npm. But I may be overthinking it.
Beyond that, my thinking was that the user needs to know the versions are incompatible, and why (or whose responsible for the situation) are secondary.
All that said, I'm fine with the current wording.
src/node_zlib.cc
Outdated
| "uses the internals of Node.js in an invalid way.\nPlease use " | ||
| "either the version of npm that is bundled with Node.js, or " | ||
| "a version of npm (> 5.5.1 or < 5.4.0) or node-tar (> 4.0.1) " | ||
| "that is compatible with Node 9 and above.\n"); |
|
Is this intended to be more-or-less permanent or something that gets removed in the foreseeable future? No objections either way. Just trying to be reasonably informed. |
|
@Trott I would expect this to be removed at some point, but that can probably be just about whenever. |
There was a problem hiding this comment.
LGTM but for my understanding, this is because tar/minizlib calls process.binding('zlib')? It's graceful-fs all over!
I looked at minizlib/index.js and I can't for the life of me figure out why it does that. It should be able to get along just fine through the public API.
Yup. The reason is that they’re trying to avoid the overhead that comes with the streams API, which is reasonable but really something we should be providing from core. |
|
CI: https://ci.nodejs.org/job/node-test-commit/13692/ This should be ready. |
|
Landed in 0300565 |
PR-URL: #16657 Refs: #16649 Refs: #14161 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: #16657 Refs: #16649 Refs: #14161 Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>
|
I've landed this on 9.x-staging to ensure it comes in the next release. |
|
@addaleax I've added dont-land-on labels for v6.x and v8.x as they are unaffected by the zlib refactor. please lmk if I am mistaken |
I realize this is a pretty unusual patch but it might make sense to do this given that a lot of people running into #16649 might not understand what is happening or how to fix it.
Refs: #16649
Refs: #14161
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
zlib