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 ignores PATHEXT on Windows #6671

Closed
timdp opened this issue May 10, 2016 · 16 comments
Closed

child_process.spawn ignores PATHEXT on Windows #6671

timdp opened this issue May 10, 2016 · 16 comments
Labels
child_process Issues and PRs related to the child_process subsystem. feature request Issues that request new features to be added to Node.js. windows Issues and PRs related to the Windows platform.

Comments

@timdp
Copy link
Contributor

timdp commented May 10, 2016

Originally reported as nodejs/node-v0.x-archive#2318.

TLDR: The workaround is to use cross-spawn (or cross-spawn-async).

@mscdex mscdex added child_process Issues and PRs related to the child_process subsystem. windows Issues and PRs related to the Windows platform. feature request Issues that request new features to be added to Node.js. labels May 10, 2016
@cjihrig
Copy link
Contributor

cjihrig commented May 13, 2016

Does the shell option to spawn() do what you need?

@timdp
Copy link
Contributor Author

timdp commented May 13, 2016

@cjihrig I'm guessing it would work, but wouldn't native support for PATHEXT be better? Otherwise, you could also claim that we'd always need to specify the .exe suffix on Windows, since otherwise, the first argument wouldn't be the exact name of the executable either.

@cjihrig
Copy link
Contributor

cjihrig commented May 13, 2016

I'm not a Windows user, so forgive me if I'm wrong. Isn't PATHEXT an environment variable used by cmd.exe? I wouldn't expect it to supported unless you're running the command in a shell.

@timdp
Copy link
Contributor Author

timdp commented May 13, 2016

On Windows, everything has an extension, but you can use child_process.spawn to spawn foo.exe by just passing in 'foo'. However, bar.cmd or baz.com can only be spawned by specifying the full filename. Since .exe is part of PATHEXT, I just assumed that it was more of a system-wide thing and expected Node to support it just like PATH itself. Correct me if I'm wrong.

@cjihrig
Copy link
Contributor

cjihrig commented May 13, 2016

It looks like libuv does this when looking up the file, so I'd expect 'foo', 'foo.com', and 'foo.exe' to all work (although judging from your comment, .com doesn't). It looks like CreateProcess() only handles .com and .exe, and anything else would require a shell.

@sam-github
Copy link
Contributor

.com, .exe, and .bat, anything else (such as .cmd) requires a shell

^--- my understanding

@timdp
Copy link
Contributor Author

timdp commented May 13, 2016

My bad, .com probably works then.

But if .cmd requires a shell, does that mean a bash script would behave the same way on *NIX? Given that a lot of apps install themselves as bash-based wrappers, that'd probably cause a lot of breakage.

But ultimately, I guess this is a libuv issue then?

@bnoordhuis
Copy link
Member

But if .cmd requires a shell, does that mean a bash script would behave the same way on *NIX?

Yes, but UNIX kernels support shebangs; on Windows, that's handled by cmd.exe.

Rule of thumb: pass { shell: true } if you don't know what kind of program you're executing.

@timdp
Copy link
Contributor Author

timdp commented May 14, 2016

How about a {shell: 'auto'} that looks at the extension on Windows? You know it's bound to end up in an npm module anyway.

@bnoordhuis
Copy link
Member

My question would be 'why'?

@timdp
Copy link
Contributor Author

timdp commented May 14, 2016

Because you don't want to spawn a shell if you don't have to. Forking Spawning is expensive.

@bnoordhuis
Copy link
Member

Performance claims should be backed up by numbers, else they're just assertions.

Aside: there is no concept of forking in the Windows process model.

@timdp
Copy link
Contributor Author

timdp commented May 14, 2016

const spawn = require('child_process').spawn

const shell = (process.argv[2] === 'true')

let i = 0

const spawnOne = () => new Promise((resolve, reject) => {
  spawn('node', ['-v'], {shell})
    .on('close', resolve)
    .on('error', reject)
})

const next = () => Promise.resolve().then(() => {
  return (i++ < 1000) ? spawnOne().then(next) : null
})

const started = Date.now()
next()
  .then(() => {
    const ended = Date.now()
    const total = ended - started
    console.log(total)
  })
  .catch((err) => {
    console.error(err.stack)
  })
Tim@CP C:\src\spawn-test
> node index true
45848

Tim@CP C:\src\spawn-test
> node index
16961

Tim@CP C:\src\spawn-test
> node index true
33879

Tim@CP C:\src\spawn-test
> node index
17076

@bnoordhuis
Copy link
Member

I guess process spawning on Windows is as slow as they say it is.

You're welcome to follow up with a pull request. I can't promise it gets merged but the numbers make a compelling case.

@timdp
Copy link
Contributor Author

timdp commented May 16, 2016

I'll see what I can do.

@bnoordhuis
Copy link
Member

Seeing how #6784 was rejected, I'll close this issue.

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. feature request Issues that request new features to be added to Node.js. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

No branches or pull requests

5 participants