Node: CHECK fails when using `natives` on node >= 8
- Version:
>=8 - Platform: All
- Subsystem:
internal/async_hooks
The Problem
require('natives').require('child_process')
Running the above code causes a CHECK to fail resulting in node crashing. This can not be caught user side and for Electron applications such as VS Code there is no way for them to catch a bad module doing this before the process crashes.
This CHECK should probably be done JS side and throw a JS land error if JS is the cause. We can leave the CHECK in for those doing bad things in native land but for the average joe just running JS we shouldn't being crashing on them IMO. Happy to put together a patch for this if folks are OK with the "throw an error in JS" strategy.
Disclaimer
I am aware that natives is questionable / not really support. However IMO the crash shouldn't happen. As I mention above it should throw a JS side error or just handle it 😄
MarshallOfSound
All 13 comments
I do not see anything wrong here as these are for internal use only. It would be better to instead ask about possible changes to public APIs/behavior to achieve whatever it is you're trying to do.
mscdex
on 9 Apr 2018
It would be better to instead ask about possible changes to public APIs/behavior to achieve whatever it is you're trying to do.
I'm not trying to do anything, but a lot of users are 😄 Over on Electron we've been getting a lot of reports of this crash, we can trace it back but the fix / patch should probably come back to here.
In an ideal world, natives wouldn't be doing the stuff it's doing (executing internal nodejs logic) but it is, and I think it's safe to say tens of thousands of people are using it right now. For instance, quite a few of VS Code's plugins have dependencies on natives. Regardless of whether this is an appropriate use of the API, I think there is minimal cost in node.js to stop the userland crash.
MarshallOfSound
on 9 Apr 2018
IMO we should not start supporting use of node internals. We started hiding a lot of things for a reason. The moment we start making the changes suggested here, in my mind we begin implicitly support third party use of node internals.
Either natives module users should ask about whatever missing functionality that is leading them to use natives in the first place or they should deal with breakage if/when it happens. The readme for natives even makes this warning known. If people ignore that warning, that is their fault IMO.
mscdex
on 9 Apr 2018
See https://github.com/nodejs/node/issues/19786#issuecomment-379041131, https://github.com/nodejs/node/issues/19786#issuecomment-379053976 and https://github.com/nodejs/node/pull/19398#issuecomment-377505368.
To summarize: known issue, will probably be fixed (@addaleax maintains natives) and stop using it now because it'll break forever sooner or later - probably sooner.
Not much else to say so I'll close this out.
bnoordhuis
on 9 Apr 2018
@bnoordhuis @mscdex can this at least be reconsidered to be changed to a runtime error in JS instead of a native crash that brings down an entire process of node.js when hit? The reality is that there are tons of node module that can still trigger this and a simple try/catch won't help to avoid this from happening.
bpasero
on 9 Apr 2018
@bpasero there is no JavaScript sitting between the call and the c++, and we want to make sure we're operating on the type we think we're operating on. bottom line these packages were always doing something bad and knowingly opted out of node's compatability
devsnek
on 9 Apr 2018
@MarshallOfSound @bpasero natives will never be properly supported.
It _was created_ for the sake of doing unsupported things to Node.js internals. Once users do that, they are on their own (meaning that all stability/semver/error handling gurantees are void). That's written in the natives readme, btw.
Anyone using natives should stop doing that and in case if there is no other way — file a request for the missing API that they are trying to achieve.
ChALkeR
on 9 Apr 2018
in my mind we begin implicitly support third party use of node internals.
I would argue that isn't the case, I'm 100% ok with these node internals throwing all kinds of errors when natives tries to use them. Throw whatever you want. My main problem is the crash. In a Node.JS world this is OK, you're node process will crash and you'll figure out why, and stop it crashing. In an Electron world this is pretty bad, because it means that just by using a node module that could be a transitive optional dependency of a plug in (VS Code example) your desktop app could crash. I'm not asking that we start supporting node internals, just that we prevent crashes caused by running JS (what is effectively happening here).
The readme for natives even makes this warning known. If people ignore that warning, that is their fault IMO.
The thing is, due to the way npm works 99.99% of people who use natives, don't even realise they are using it. It just becomes part of their module tree, something like the incredibly popular graceful-fs@<=3.0.0 depended on natives for years and there are still hundreds of modules _(citation needed)_ out there today that depends on things that depend on natives. As I mentioned above, I don't want node to start supporting this use case. I agree it's a bad idea and shouldn't be supported, I just don't think that doing it should cause a crash. Especially when it's so easily preventable.
Anyone using natives should stop doing that and in case if there is no other way — file a request for the missing API that they are trying to achieve.
This is a wonderful idea in theory, but people getting this crash probably don't have any idea natives is causing it, we only tracked it down recently. The best outcome I see for this is a JS error being thrown when this happens, telling you that bad things are happening and it's probably "natives" fault.
MarshallOfSound
on 9 Apr 2018
@MarshallOfSound
The best outcome I see for this is a JS error being thrown when this happens
You don't understand. natives was created to work-around the Node.js API (which includes the safety checks) and to get access to internals that are lower-level than the safety checks.
don't even realise they are using it
natives logs a warning at installation time.
They only way where these could be fixed on case-by-case basis is on the natives side, but maintaining it would be a nightmare.
ChALkeR
on 9 Apr 2018
@addaleax What do you think of emitting a deprecation warning at runtime from natives side if Node.js version is 10.0 or above? That kinda fits semver imo.
ChALkeR
on 9 Apr 2018
@ChALkeR Tbh, I’d like to wait until gulp@4 is out. But once that happens, yes, I’m all for it. :)
addaleax
on 16 Apr 2018
@addaleax Technically, gulp@4 is out for several months and seems working just fine.
ChALkeR
on 16 Apr 2018
I meant, when it’s no longer a pre-release. @phated has indicated that there is still a lot of work to be done around things like documentation, and I don’t really want to push users towards underdocumented software either…
addaleax
on 16 Apr 2018
Related issues
fanjunzhi
·
3Comments
Brekmister
·
3Comments
akdor1154
·
3Comments
danielstaleiny
·
3Comments
filipesilvaa
·
3Comments
Most helpful comment
IMO we should not start supporting use of node internals. We started hiding a lot of things for a reason. The moment we start making the changes suggested here, in my mind we begin implicitly support third party use of node internals.
Either
nativesmodule users should ask about whatever missing functionality that is leading them to usenativesin the first place or they should deal with breakage if/when it happens. The readme fornativeseven makes this warning known. If people ignore that warning, that is their fault IMO.