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

Fix spawn of cmd and bat files on Windows #15217

Closed
atrauzzi opened this issue Sep 6, 2017 · 19 comments
Closed

Fix spawn of cmd and bat files on Windows #15217

atrauzzi opened this issue Sep 6, 2017 · 19 comments
Labels
child_process Issues and PRs related to the child_process subsystem. windows Issues and PRs related to the Windows platform.

Comments

@atrauzzi
Copy link

atrauzzi commented Sep 6, 2017

This ticket is specifically in reference to this: https://nodejs.org/api/child_process.html#child_process_spawning_bat_and_cmd_files_on_windows

I've put together a comparison as an example, basically it uses edgejs to spawn a command that's based off of a cmd file (electron): https://gist.github.com/atrauzzi/bbcc171b57a7ea276a2e6bf6419c7e51

The point of the comparison is to show that it is possible to run cmd and bat files without requiring a console window open the whole time. It would just be a matter of figuring out how .net is doing it and plugging into that same mechanism when running on Windows vs however nodejs is currently doing things.

But I really think this issue is long overdue to be addressed.

cc. #556

@mscdex mscdex added child_process Issues and PRs related to the child_process subsystem. windows Issues and PRs related to the Windows platform. labels Sep 6, 2017
@mscdex
Copy link
Contributor

mscdex commented Sep 6, 2017

For some reason, the process module in nodejs imposes that it be run in a console window.

I think this snippet from the C# code you referenced pretty much says it all:

WindowStyle = System.Diagnostics.ProcessWindowStyle.Hidden

A console window is still being created. So C# is really doing the same thing, except it is removing the window from the taskbar.

@atrauzzi
Copy link
Author

atrauzzi commented Sep 6, 2017

Nice, that probably bodes well for a fix then because it means that nodejs isn't doing anything wrong in how it spawns, it's just not fully abstracted over this Windows-specific nuance?

@bnoordhuis
Copy link
Member

Libuv supports it (the UV_PROCESS_WINDOWS_HIDE flag to uv_spawn()) but node.js hasn't exposed it so far.

@atrauzzi
Copy link
Author

atrauzzi commented Sep 6, 2017

Cool, is this something that can be exposed as another option to spawn and exec? Without claiming to be a whiz with C++ or nodejs internals, conceptually this sounds pretty low risk. And I mean, this is an issue that's been spoken about since at least 2015 -- pretty low hanging fruit for such a high impact.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 10, 2017

@atrauzzi if you follow the logic of windowsVerbatimArguments, you should be able to implement something similar for this flag.

@atrauzzi
Copy link
Author

I suspect it would make more sense for someone familiar with C++ dev to implement this.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 13, 2017

@atrauzzi I've opened #15380, which exposes UV_PROCESS_WINDOWS_HIDE as windowsHide in the child_process methods. I don't have a Windows box on hand to test with at the moment, so could you or another Windows user see if that fixes the problem.

@bzoz
Copy link
Contributor

bzoz commented Sep 14, 2017

I've tested #15380 but it looks like it does not solve the issue. But I'm also not confident that I've tested the right thing.

@atrauzzi could you provide a small example of how to reproduce the console popping? It would be great if it would be in pure Node.js, but since this looks related to the PM2 internals, an example that uses PM2 will work for now.

@bnoordhuis
Copy link
Member

Ping @atrauzzi.

@atrauzzi
Copy link
Author

atrauzzi commented Sep 21, 2017

I put together a quick example as part of this ticket's initial description if that's enough?

@bzoz
Copy link
Contributor

bzoz commented Sep 21, 2017

@atrauzzi
Copy link
Author

Yeah.

@refack
Copy link
Contributor

refack commented Sep 21, 2017

How serendipitous 😄 this is what's been stopping me from using this in #14042

@bzoz
Copy link
Contributor

bzoz commented Sep 22, 2017

@atrauzzi the example looks like workaround the issue using edge-js. Could you provide some code that demonstrates the console window popping in and out?

@atrauzzi
Copy link
Author

atrauzzi commented Sep 22, 2017

You can play with this module and flip the flags around in the node-native spawn:

https://github.com/atrauzzi/gerty/blob/master/src/Spawn.ts#L15-L35

I can't remember off the top of my head at the moment what the correct combination is. But I believe there's a combination that results in a window popping open when it really shouldn't be.

@bzoz
Copy link
Contributor

bzoz commented Sep 22, 2017

Well, an exact combination would be very helpful 😉. I'm not even sure what am I looking for, the more detailed instructions you provide the easier it will be for me to find the issue and fix it.

@bzoz
Copy link
Contributor

bzoz commented Sep 25, 2017

After libuv PR it still looks like it does not work as intended.

I've included libuv/libuv#1558 and #15380 in my Node build. I've included windowsHide: true in the spawn call (ForkMode.js:94). Since MSDN says using detached: true disables flag from libuv/libuv#1558, I've tied this two times with and without detached: true (with pm2 update in the middle).

I've created an empty file test.js. And I called: node bin/pm2 start test.js. Once in a while a console window would pop in for a split-second.

cjihrig added a commit to cjihrig/node that referenced this issue Oct 16, 2017
This commit exposes the UV_PROCESS_WINDOWS_HIDE flag in Node
as a windowsHide option to the child_process methods. The
option is only applicable to Windows, and is ignored elsewhere.

Fixes: nodejs#15217
PR-URL: nodejs#15380
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit to ayojs/ayo that referenced this issue Oct 18, 2017
This commit exposes the UV_PROCESS_WINDOWS_HIDE flag in Node
as a windowsHide option to the child_process methods. The
option is only applicable to Windows, and is ignored elsewhere.

Fixes: nodejs/node#15217
PR-URL: nodejs/node#15380
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
cjihrig added a commit to cjihrig/node that referenced this issue Oct 24, 2017
This commit exposes the UV_PROCESS_WINDOWS_HIDE flag in Node
as a windowsHide option to the child_process methods. The
option is only applicable to Windows, and is ignored elsewhere.

Fixes: nodejs#15217
PR-URL: nodejs#15380
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Oct 24, 2017
This commit exposes the UV_PROCESS_WINDOWS_HIDE flag in Node
as a windowsHide option to the child_process methods. The
option is only applicable to Windows, and is ignored elsewhere.

Backport-PR-URL: #16425
Fixes: #15217
PR-URL: #15380
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@atrauzzi
Copy link
Author

atrauzzi commented Nov 6, 2017

Sorry, been very busy lately, but I just wanted to catch up -- Will the fix(es?) planned allow me to spawn a process with a shell, but without a window accompanying it? My main case here is that I need to have access to the default shell environment of the current user when spawning.

@toddwong
Copy link
Contributor

Will cluster.fork honor this windowsHide eventually? It calls child_process.fork without this option for now I think.

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

7 participants