Repository navigation
process.binding('spawn_sync') is required for spawn-wrap (and thus nyc) to work. #27344
Description
Activity
@coreyfarrell
nycis important, do you have any suggestions as to what core could do to enable spawn-wrap? Would simply making sure that the binding's spawn was available somewhere be a good idea, or can you propose a more obvious API for hooking the underlying spawn? Also, do you know how/if coverage is done by any other coverage tools? I'm wondering ifnychas alternatives, and what they do.Being able to implement code coverage metrics from tests is a pretty important quality of a language run-time, I'm wondering if something more maintainable can be proposed that would help multiple coverage providers.
/to @nodejs/tooling
We hook
require('child_process').spawnby replacingrequire('child_process').ChildProcess.prototype.spawnwith a wrapper function. So I guess that could be a concern sinceChildProcess.prototype.spawnis not documented?spawnSync is what I'm concerned about (sorry my initial post said spawnWrap). Right now
lib/internal/child_process.jsdirectly exportsspawnSync, if it were instead a static function of ChildProcess maybe we could wrap that. Not sure if that's a desirable API, I haven't tested to verify this will work yet as I don't want to try patching node.js without getting feedback (though from reading the code I think it should work). Replacingrequire('child_process').spawnSyncwill not work as we need our function to be called after the public spawnSync sets all the proper options.c8 works by setting
process.env.NODE_V8_COVERAGE, as long as nothing else touches that environment it'll capture all sub-processes. I'm not sure how other coverage systems handle sub-processes. The point of spawn-wrap is to allow nyc to add require hooks to all node.js child processes (and grandchildren, etc). So the ability to runnyc npm testdepends on this as we don't control the way thatnpmspawns processes.One of my hopes about having a
NODE_OPTIONSthat supports--requireis that it could be used to cause js to be loaded into all child node processes in a way that didn't involve monkey patching.Would this be a mechanism you could use? Broadly, it seems to have the same effect as prepending an alternative main js file in order to do pre-setup, as nyc is doing now.
NODE_OPTIONSis used by end users and potentially by other libraries which perform spawns that we might want to capture results of. I think the problem withNODE_OPTIONSis that if something/someone wants to set it, they will replace the value entirely. You'd have situations where child/grandchild processes did not load the nyc hooks and covered silently dropped.In theory a
NODE_REQUIRESvariable could be reliably useful if it were documented that the value should not replaced but instead prepended likePATH. Maybe even an API likeprocess.hookChildren('@babel/register')and document that the variable should not be manually manipulated?For either approach to a hypothetical
nyc/registerwe would probably need to point therequireto the absolute path ofnyc/registerto deal with situations wherenycis globally installed or otherwise in a place that is unresolvable to the target binary.I think the problem with NODE_OPTIONS is that if something/someone wants to set it, they will replace the value entirely
Not necessarily, they could append. It doesn't seem so different from PATH. It can be replaced, or appended to, and all kinds of carnage can occur if its done wrongly.
NODE_OPTIONS isn't set that much by node packages as they spawn children, because args can be put into ARGV more directly. Its mostly used in deployment before calling node or
npm start, and for projects that do set it programmatically (in their unit tests, no less) to be told "that is breaking coverage, please append", doesn't seem so outrageous. After all, its always possible to break coverage. For example, by overwriting the spawn hooks you monkey patch in. Duelling spawn monkey patches seems only marginally less likely than dueling NODE_OPTIONS.Maybe even an API like process.hookChildren('@babel/register') and document that the variable should not be manually manipulated?
An API like that, maybe using a "don't touch this directly"
NODE_PRELOADenv variable that is essentallyNODE_OPTIONSbut only supports--require, seems not so hard to implement, but I'd need to gather a few more use-cases to be able to show its not just anychack.we would probably need to point the require to the absolute path of nyc/register
Yes. I assume you do that already with the startup runner you insert into the ARGV, or a
process.chdir()would beak coverage.One concern I have is escaping in NODE_OPTIONS. I'm considering adding an option to nyc@15 which would allow it to bypass spawn-wrap and prepend NODE_OPTIONS instead. My concern is that I'm not sure what is required to ensure proper/safe escaping of the NODE_OPTIONS value. I'm not sure it's necessarily a security concern but I do want to ensure that nyc works properly even if
process.cwd()contains spaces or quote marks.@coreyfarrell Backslash escapes the next character and text in double quotes is parsed as a single string. E.g.
--arg="foo \"bar\" baz"works as you expect it to.It looks like the option to
--requirecannot be quoted before node.js 12?[cfarrell@lt2 nyc]$ nvm exec 12 ./bin/nyc.js --set-node-options ./mine.js Running node v12.10.0 (npm v6.10.3) Success ----------|---------|----------|---------|---------|------------------- File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s ----------|---------|----------|---------|---------|------------------- All files | 100 | 100 | 100 | 100 | mine.js | 100 | 100 | 100 | 100 | ----------|---------|----------|---------|---------|------------------- [cfarrell@lt2 nyc]$ nvm exec 10 ./bin/nyc.js --set-node-options ./mine.js Running node v10.16.3 (npm v6.9.0) internal/modules/cjs/loader.js:638 throw err; ^ Error: Cannot find module '"/usr/src/npm/nyc/lib/wrap.js"' at Function.Module._resolveFilename (internal/modules/cjs/loader.js:636:15) at Function.Module._load (internal/modules/cjs/loader.js:562:25) at Module.require (internal/modules/cjs/loader.js:692:17) at Module._preloadModules (internal/modules/cjs/loader.js:901:12) at preloadModules (internal/bootstrap/node.js:601:7) at startup (internal/bootstrap/node.js:273:9) at bootstrapNodeJSCore (internal/bootstrap/node.js:622:3) ----------|---------|----------|---------|---------|------------------- File | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s ----------|---------|----------|---------|---------|------------------- All files | 0 | 0 | 0 | 0 | ----------|---------|----------|---------|---------|-------------------From istanbuljs/nyc#1169 - not sure if you have any advice of how to properly escape/quote the path given to the
--requireoption for node.js 8 / 10? Given mypwdof/usr/src/npm/nycthis is effectively initializing the environment with:process.env.NODE_OPTIONS = ' --require="/usr/src/npm/nyc/lib/wrap.js"';
In the above test
./mine.jsis an executable script that just runsconsole.log('Success');.Nevermind, I found #24065 and the linked nearform PR, so I'm using
NODE_PATHfor node.js < 12 when the require path contains a space.I discussed the current PR with @bcoe a few days ago, a concern we have with enabling the
NODE_OPTIONSmethod of istanbuljs/nyc#1169 by default isrequire('child_process').spawn(bin, args, {env: {}})- that child would not get NODE_OPTIONS, NYC_CWD or NYC_CONFIG environmental variables and thus no coverage.Maybe even an API like process.hookChildren('@babel/register') and document that the variable should not be manually manipulated?
An API like that, maybe using a "don't touch this directly" NODE_PRELOAD env variable that is essentally NODE_OPTIONS but only supports --require, seems not so hard to implement, but I'd need to gather a few more use-cases to be able to show its not just a nyc hack.
I've created an experimental node-preload module that in theory could replace
spawn-wrap. I realize this does not solve theprocess.bindingissue as is but I think it could potentially be implemented in node.js core which would eventually eliminate the need for that module. I'd love feedback on the API. CC @bcoe @isaacs @JaKXz.I realize this doesn't answer the question of "not just a nyc hack". In theory this could be used to implement a "all child processes" mode to
@babel/registerbut I don't have absolute use cases.Reacted by Benjamin E. Coe@coreyfarrell @bcoe ... is this still an issue?
@jasnell yes. Although nyc no longer uses
spawn-wrapby default the replacementnode-preloadusesprocess-on-spawnwhich does requireprocess.binding('spawn_sync')to function.How to best structure the API for this along with other core hooks is being investigated by the tooling team.
Since this issue has been open for 2.5 years with no movement, I'll go ahead and close it.
spawn-wrap works by intercepting calls to spawn and spawnSync. spawn is handled by replacing
ChildProcess.prototype.spawn, spawnSync is handled by replacingprocess.bindings('spawn_sync').spawn. With #22160 and #22260 I assume we will need a migration path in the future.CC @isaacs @bcoe