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

child_process.spawn parity between windows and posix #10302

Closed
papercuptech opened this issue Dec 16, 2016 · 13 comments
Closed

child_process.spawn parity between windows and posix #10302

papercuptech opened this issue Dec 16, 2016 · 13 comments
Labels
child_process Issues and PRs related to the child_process subsystem. windows Issues and PRs related to the Windows platform.

Comments

@papercuptech
Copy link

papercuptech commented Dec 16, 2016

  • Version: 7.2.0 (all?)
  • Platform: Windows
  • Subsystem: Child Process

On Posix:

test file:

#!/usr/bin/env bash
echo "hello"
$ chmod 755 test
node
> var cp = require('child_process')
undefined
> cp.spawnSync('test')
'hello\n'
>

On Windows:

test.cmd file:

echo "hello"
$ echo %PATHEXT%
.COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC
node
> var cp = require('child_process')
undefined
> cp.spawnSync('test')
null
> cp.spawnSync('test.cmd')
null
> cp.spawnSync('test', {shell:true})
'hello\n'

Windows uses PATHEXT, ASSOC, and FTYPE to achieve something similar to Posix using execute permission and shebangs.

On Windows, spawn() is not giving Windows a chance to query PATHEXT, ASSOC, FTYPE information to run test.cmd as an executable.

To fix, on Windows, spawn() should always launch using CMD, when {shell:undefined} (i.e. set it to true when undefined), so that CMD can correctly handle PATHEXT, ASSOC, and FTYPE configuration. This does mean real, binary executables won't be launched directly but this should have no perceivable impact on launch time or behavior. The benefit is increased parity between Posix and Windows behavior.

@papercuptech papercuptech changed the title fs.spawn parity between windows and posix child_process.spawn parity between windows and posix Dec 16, 2016
@bnoordhuis
Copy link
Member

Don't exec/execFile and their sync counterparts work for you? They do what you request: run the command through the command processor first.

@papercuptech
Copy link
Author

They don't :(

$ node
> var cp = require('child_process')
undefined
> cp.execSync('test').toString()
'\r\cC:\\Users\\John\\src\\test>echo Hello \r\nHello\r\n'
> cp.execFileSync('test')
Error: spawnSync test ENOENT

Passing {shell: true} to spawn() works.

But the issue isn't about how I can get it to work, rather it's a difference in behavior that effects others who already are using spawn() for whatever reason, and where we're wanting to 'hook' the executable they're calling, by using a script with the same base name.

On Posix, this is very easy; just make a script with the same name and chmod 755. done!

On Windows, they have to change their spawn call to {shell: true}, which is unreasonable as we're trying to make it transparent.

@mscdex mscdex added the child_process Issues and PRs related to the child_process subsystem. label Dec 16, 2016
@papercuptech
Copy link
Author

The other thing is CMD will also correctly search PATH, which I forgot to mention.

@cjihrig
Copy link
Contributor

cjihrig commented Dec 16, 2016

Aren't you observing the documented behavior from https://nodejs.org/api/child_process.html#child_process_spawning_bat_and_cmd_files_on_windows?

@papercuptech
Copy link
Author

papercuptech commented Dec 16, 2016

Yes. And that actually applies to not just .bat or .cmd, but any extension in PATHEXT.

However, I think the point of this issue is being misunderstood.

There's a difference in behavior between Posix and Windows when there doesn't need to be.

It would be ideal if they behaved the same when possible.

Because they don't we can't support our desired use case.

Is there some technical reason why, on Windows, spawn() can't just set {shell: true} when it's undefined?

@papercuptech
Copy link
Author

papercuptech commented Dec 16, 2016

Having a file whose extension is in PATHEXT on Windows, is conceptually similar to setting execute permission on a file in Posix.

Using ASSOC and FTYPE to map an extension to an executable on Windows, is conceptually similar to writing #!/some/executable in a script on Posix.

I believe the kernel in Posix is reading the #! to determine how to execute the file. On Windows the same basic logic, except using PATHEXT, ASSOC, and FTYPE configurations, happens through CMD and not the kernel.

I can appreciate being able to technically state spawn is explicit in it's reliance on the kernel vs a shell, but I think in practice in this case it's a distinction that doesn't help normalize the platform, which is one of the nicest things about using node.

I've not found anything that breaks when launching some .exe or .com directly, vs them launching though cmd.exe, which would be the effect of spawn() defaulting shell=true on Windows.

I'm sure I'm missing something?

@gucong3000
Copy link

@papercuptech
I wrote a patch to let Windows support the functionality you want
https://github.com/gucong3000/exec-extra

@papercuptech
Copy link
Author

@gucong3000 Thank you.

However, that's not quite what I need for my use case. I can't change anything in the node program that's calling spawn() because it's not our program, I can only change the environment of the machine during installation of our utility.

I make sure our 'someprog.cmd' is higher in up in PATH, so (assuming spawn() effectively behaved the same on Posix as Windows), when the program calls spawn('someprog'), it would actually be launching our script, which then does's its thing before calling the 'real' someprog.

On Posix this works as expected, on Windows it does not.

@bnoordhuis @cjihrig Is this issue likely to be looked at or addressed?

@sam-github
Copy link
Contributor

It would be ideal if they behaved the same when possible.

Agreed.

Is there some technical reason why, on Windows, spawn() can't just set {shell: true} when it's undefined?

Because it causes spawn to not behave the same on Windows and Unix! It creates an intermediary process, so the process you think you are spawning as your child is actually your grand child.

For applications that do not care if they have an intermediate process, they can set {shell: true} on Windows and Unix, and get consistent behaviour.

Making shell be true by default is a pretty breaking change.

It would make more sense to have completely different APIs (exec vs. spawn already exists, however, wherein exec uses shell by default, and spawn does not).

This should be fixed in libuv, @joshgav and @digitalinfinity, by libuv doing the registry lookup, and directly spawning the correct interpreter so there is no intermediate child process.

@digitalinfinity
Copy link
Contributor

Thanks @sam-github - I think @ianwjhalliday was planning on taking a crack at this once he was back from vacation.

@papercuptech
Copy link
Author

This should be fixed in libuv, @joshgav and @digitalinfinity, by libuv doing the registry lookup, and directly spawning the correct interpreter so there is no intermediate child process.

This is the ideal for sure.

Making shell be true by default is a pretty breaking change.

Yeaahh.. Understood actual process would be grand child, but purely from node's API, and this is just out of curiosity, what would a caller actually see as a change in behavior?

It would make more sense to have completely different APIs

Hmmm... Not a fan of yet another api here; three should be plenty? If fixed in uv, then no need, correct?

@sam-github
Copy link
Contributor

spawn("cat").pid would not be the process ID of "cat", it would be the process ID of "cmd", so when the caller then tried to interact with that process ID... it would not be the process it thinks it is. On Linux, for example, if you sent a signal to your child, it would arrive at the wrong process, of if you on any platform report the PID of your child in a log, and support people do some looking, they won't find the right process.

@papercuptech
Copy link
Author

Doh! of course.

Updating uv seems the best place, except it would have to mimic something Windows is doing. Probably not a big deal in this case, but if Windows implementation changed in this area, uv would have to follow, and it would seem implementation would have to search PATH, look for any files with PATHEXT extensions (assuming command arg to spawn was extension-less), lookup ext-to-filetype in registry (which I think might not be as straightforward as one would think) and then filetype-to-realexe. All do-able for sure but maybe a minor hassle.

Just wondering: Only on Windows, and only when shell === undefined, to then launch through cmd, and if there's a way to detect the 'real' process is the grand child, to return it's PID, and whatever else of it, so caller has no idea, and CMD (Windows) can still deal with resolving all the above? Might still need be done in uv, unless uv exposes enough to node to figure this out for itself?

Just an idea.

Regardless, it would be nice if spawn() worked the same in this case, and it does seem something in, or with help of, uv could make that happen without having to add a forth child_process method :).

Thanks

@refack refack added the windows Issues and PRs related to the Windows platform. label Jul 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

No branches or pull requests

8 participants