To clarify:

CREATE_NO_WINDOW does not appear compatible with inherited console handles (no console, and hence console handles, get created). MoarVM showed a regression post v1.15.0 because it uses inherited stdio handles by default, and -always- sets UV_PROCESS_WINDOWS_HIDE. The problem was not immediately obvious on nodejs because it defaults to stdio pipes, but indeed a deadlock can be shown with the right parameters:
node -e "const cp = require('child_process'); { const bat = cp.spawn('cmd.exe', ['/k', 'dir'], { stdio: 'inherit', detached: false, windowsHide: true }); }"

In the case of MoarVM this PR fixes the regression introduced in b21c1f9

In the case of nodejs this PR should bring back the console 'blip' but only if stdio: 'inherit' is used, with the possible caveat (bonus) that it no longer deadlocks.

@bzoz do you have any thoughts on this?

Sorry, was on vacations.

This sounds reasonable, since it solves regression in libuv. @ugexe could you add a test for this?

@bzoz @cjihrig I'm not sure how to write a test for this ( at this low of a level anyway ). Specifically the last step mentioned below:

  /* process setup to test */
  options.stdio[0].flags = UV_IGNORE;
  options.stdio[1].flags = UV_INHERIT_FD;
  options.stdio[1].data.fd = 1;
  options.stdio[2].flags = UV_INHERIT_FD;
  options.stdio[2].data.fd = 2;
  options.stdio_count = 3;
  options.flags = UV_PROCESS_WINDOWS_HIDE;

and then:

• spawn process -- expected to write '1' to stdout and '2' to stderr.

• check that stdout contains '1' and stderr eq '2' ( both would be empty before this PR )

I'm not sure how to write a test for this ( at this low of a level anyway )

IIRC, I saw somewhere that you can make Node.js deadlock with this bug. Am I remembering correctly? If so, maybe a PR to Node adding a test case would be better than nothing. We're in the process of trying to get a CI job that tests libuv commits in Node.

@cjihrig

node -e "const cp = require('child_process'); { const bat = cp.spawn('cmd.exe', ['/k', 'dir'], { stdio: 'inherit', detached: false, windowsHide: true }); }"

Change windowsHide to false and it no longer deadlocks. But note that this is probably counting on some other node bug; cmd.exe up/down history list no longer works after running that with windowsHide: false until pressing Cntrl+C, possibly indicating an open handle? Either way I'm not sure it's a good test.

Technically tests exist as part of the Perl 6 spectests, but unfortunately we don't yet have the tooling/infrastructure to apply a build system our ( MoarVM ) dependencies.

But note that this is probably counting on some other node bug; cmd.exe up/down history list no longer works after running that with windowsHide: false until pressing Cntrl+C, possibly indicating an open handle?

I'm not a Windows user, but /k executes dir and then gives you back a command prompt (which doesn't exit). My guess would be that the up/down key presses are being sent to the child process that is still running, and pressing Ctrl+C kills it, bringing you back to your original command prompt, with working up/down history.

The Node issue is here nodejs/node#17824

Just gave it a spin. The "deadlock" is because of /k, if you change it to /c it exits correctly. Nonetheless, dir output is lost, so there is a genuine bug here.

Also, if you redirect stdout to a pipe or file it works, so this is a tty specific thing. I don't know if we can reliably test this in CI. I'm compiling Node with this change on my box right now, I'll test manually if this fixes that issue.

Anyhow, libuv CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/698/

I had to rebase this on top of libuv/v1.x to make it work with Node. One of the Node tests is failing:

C:\...\node>Release\node.exe test\parallel\test-tty-get-color-depth.js
tty.js:83
    throw new errors.SystemError(ctx);
    ^

Error [ERR_SYSTEM_ERROR]: bad file descriptor: EBADF [uv_tty_init]
    at new WriteStream (tty.js:83:11)
    at Object.<anonymous> (C:\...\node\test\parallel\test-tty-get-color-depth.js:31:21)
    at Module._compile (module.js:666:30)
    at Object.Module._extensions..js (module.js:677:10)
    at Module.load (module.js:578:32)
    at tryModuleLoad (module.js:518:12)
    at Function.Module._load (module.js:510:3)
    at Function.Module.runMain (module.js:707:10)
    at startup (bootstrap_node.js:191:16)
    at bootstrap_node.js:683:3

This PR fixes the issue in Node, CI failures are unrelated and the failing test is currently also failing on master. So, this LGTM.

Thanks for the help digging into this

Sorry, forgot about this. I'll land this tomorrow.

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/709/

Landed in 48ba92c

U-oh, I've messed up - the commit credits me as the author instead of @ugexe.

Fixing that is beyond my git-fu, @libuv/collaborators can you help out?

@bzoz, yeah will do that

Landed in 491848a with the correct author.

Thanks!