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

Revert "child_process: change windowsHide default to true" #24034

Closed
wants to merge 2 commits into
base: master
from

Conversation

@Trott
Member

Trott commented Nov 2, 2018

This reverts commit 420d8af.

I'm opening this not because I'm convinced the commit needs to be reverted, but to hopefully elicit a prompt conversation and decision about whether or not it should be reverted.

Refs: #21316 (comment) and several subsequent comments.

I'm particularly interested in what embedders, especially Electron folks, think. @MarshallOfSound has already weighed in that they don't believe a revert is warranted, but also expressed it as a personal opinion (so, I assume, not speaking on behalf of Electron). Would love to hear from @codebytere or @zeke or @groundwater or someone, either to get a consensus of personal opinions or else to get a semi-official "Electron prefers revert/no-revert".

@nodejs/embedders

IIUC, this is only affecting developers and not end users. Doing this out of an abundance of caution and in case I"m wrong about that.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@Trott Trott referenced this pull request Nov 2, 2018

Merged

child_process: change windowsHide default to true #21316

4 of 4 tasks complete

@refack refack added the windows label Nov 2, 2018

@Trott

This comment has been minimized.

@bzoz

This comment has been minimized.

Contributor

bzoz commented Nov 2, 2018

FWIW this was added because it was requested by electron in libuv/libuv#1878

@ryzokuken

This comment has been minimized.

Member

ryzokuken commented Nov 2, 2018

@bzoz totally! You'd have to agree though that this did kinda bamboozle Electron. I mean, it was intended to hide phantom console windows from popping up, GUI windows were never in the equation (why would we even want that?).

That said, it's a problem that could be easily solved on Electron's end so this might not be necessary.

@refack

This comment has been minimized.

Member

refack commented Nov 2, 2018

I think the actual problem that was experienced by Electron was fixed in:
libuv/libuv#1558
and
libuv/libuv#1659
IIUC it was that even when windowsHide was set to true a console window would flash and disappear.

@ryzokuken

This comment has been minimized.

Member

ryzokuken commented Nov 2, 2018

@refack well, that'll be great news in favor of reverting.

I'll let y'all know what the consensus is on this. That'll have to wait until Monday though.

@refack

This comment has been minimized.

Member

refack commented Nov 2, 2018

This has some WIN32 minutia behind it. And I'm writing the following with some approximations, and only with my own understanding.
The Windows native binary format (PE) has in it's header a field named subsystem, which is commonly either console or windows. Only windows executables can interact with the user, so console binaries are hosted inside conhost.exe which is Windows' analog to a terminal emulator:
image
Node.js for Windows is compiled as a console app so for it to interact with the user it also runs through conhost:
image

So essentially you can only directly spawn console applications applications from node without creating a new Window. Also if you want to spawn a "detached" process (one that's life-cycle is independent of its parent) it has to have it's own window. That window can be "hidden", but it's there.


For reference, the most similar "native" APIs are

(other runtimes such as ActiveState Perl or cpython , have a similar API with the default to hidden=false)

@refack

This comment has been minimized.

Member

refack commented Nov 2, 2018

@yodurr or @bitcrazed could you help me correct any mistakes in the above ^^^^

@refack

refack approved these changes Nov 2, 2018

@codebytere

This comment has been minimized.

Member

codebytere commented Nov 2, 2018

Electron can work around this, so we'd be +0 on a revert, i would say!

@Trott

This comment has been minimized.

Member

Trott commented Nov 2, 2018

Explicitly pinging the author and approvers of the commit potentially being reverted: @cjihrig @TimothyGu @bzoz @apapirovski @trivikr @addaleax @ryzokuken @BridgeAR

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Nov 2, 2018

In a perfect world, I think defaulting to hiding the window is the correct behavior. This doesn't impact me personally, so I'm fine with deferring to the people that are impacted. I will say though that I was under the impression that only console windows would be impacted by the change.

@Trott

This comment has been minimized.

Member

Trott commented Nov 8, 2018

FWIW this was added because it was requested by electron in libuv/libuv#1878

Talked a bit with @codebytere tonight and the upshot seems to be that a change in libuv was requested, but that this change in Node.js was not quite what they wanted. They can live with this, but do have a slight preference to it being reverted.

@bzoz

bzoz approved these changes Nov 8, 2018

@Trott

This comment has been minimized.

Member

Trott commented Nov 9, 2018

Approvals on this from two Electron team members and two Windows-centric collaborators. I'm going to land on master in 24 hours if there's no objections.

Since this is reverting a semver-major, landing on v11.x-staging may require some TSC buy-in so: ping @nodejs/tsc for concerns/objections. I discussed this briefly with @ofrobots yesterday and he was in favor of reverting on 11.x rather than waiting for more people to rely on the new behavior. I concur, so that's two of us....

@sindresorhus

This comment has been minimized.

sindresorhus commented Nov 9, 2018

It seems like everyone agrees that hiding is the best default for users, so I'm surprised you're making Node.js worse for end-users just because of an embedder. Electron can work around this. If this is merged, every usage of childProcess.spawn() will have to include this option. I strongly oppose this change.

@refack

This comment has been minimized.

Member

refack commented Nov 9, 2018

(h/t @refack for patiently filling in many gaps in my understanding/knowledge here)

I gave my perspective, and what I know from my own experience. AFAIK it's a tricky issue even for Windows experts with 110K hits in Google and 13K hits in SO. So I'm still hoping for more Windows people to chime in.

cjihrig added a commit to libuv/libuv that referenced this pull request Nov 10, 2018

win: support more fine-grained windows hiding
Added UV_PROCESS_WINDOWS_HIDE_CONSOLE and
UV_PROCESS_WINDOWS_HIDE_GUI for specifying if console or GUI
subprocess windows are to be hidden.

Refs: nodejs/node#24034
PR-URL: #2073
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Trott

This comment has been minimized.

Member

Trott commented Nov 12, 2018

We've had 3 days of quiet on this. I'm going to land it some time soon (probably within the next 24 hours, certainly 48), so if someone has a formal objection, now would be the time to make it.

Pinging @nodejs/chakracore on the grounds that they might have a good idea about expected behavior etc. as well as the accuracy and completeness of #24034 (comment) and other things.

@Trott

This comment has been minimized.

Member

Trott commented Nov 12, 2018

Last CI run was all green, but it was also 10 days ago, and a lot of code has landed since then. So just to be super extra special careful, here's another CI: https://ci.nodejs.org/job/node-test-pull-request/18563/

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Nov 12, 2018

Wait was this actually not desirable?

@Trott

This comment has been minimized.

Member

Trott commented Nov 13, 2018

@Trott

This comment has been minimized.

Member

Trott commented Nov 13, 2018

Landed in ad6ead3

@Trott Trott closed this Nov 13, 2018

Trott added a commit to Trott/io.js that referenced this pull request Nov 13, 2018

Revert "child_process: change windowsHide default to true"
This reverts commit 420d8af.

PR-URL: nodejs#24034
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

@refack refack added the v11.x label Nov 13, 2018

BridgeAR added a commit that referenced this pull request Nov 14, 2018

2018-11-15, Version 11.2.0 (Current)
Notable changes:

* child_process:
  * All child processes will again open up a new Window on Windows by
    default. [#24034](#24034)
* deps:
  * A new and fast experimental HTTP parser (`llhttp`) is now supported.
    [#24059](#24059)
* Added new collaborators:
  * [oyyd](https://github.com/oyyd) - Ouyang Yadong.
    [#24300](#24300)
  * [psmarshall](https://github.com/psmarshall) - Peter Marshall.
    [#24170](#24170)
  * [shisama](https://github.com/shisama) - Masashi Hirano.
    [#24136](#24136)

@BridgeAR BridgeAR referenced this pull request Nov 14, 2018

Merged

Release proposal: v11.2 #24350

BridgeAR added a commit that referenced this pull request Nov 14, 2018

Revert "child_process: change windowsHide default to true"
This reverts commit 420d8af.

PR-URL: #24034
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

@targos targos added this to Closed PRs in v11.x Nov 14, 2018

BridgeAR added a commit that referenced this pull request Nov 14, 2018

2018-11-15, Version 11.2.0 (Current)
Notable changes:

* child_process:
  * All child processes will again open up a new Window on Windows by
    default. [#24034](#24034)
* deps:
  * A new and fast experimental HTTP parser (`llhttp`) is now supported.
    [#24059](#24059)
* **Windows**
  * A crashing process will now show the names of stack frames if the
    node.pdb file is available.
    [#23822](#23822)
* Added new collaborators:
  * [oyyd](https://github.com/oyyd) - Ouyang Yadong.
    [#24300](#24300)
  * [psmarshall](https://github.com/psmarshall) - Peter Marshall.
    [#24170](#24170)
  * [shisama](https://github.com/shisama) - Masashi Hirano.
    [#24136](#24136)

kiyomizumia added a commit to kiyomizumia/node that referenced this pull request Nov 15, 2018

Revert "child_process: change windowsHide default to true"
This reverts commit 420d8af.

PR-URL: nodejs#24034
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

BridgeAR added a commit that referenced this pull request Nov 15, 2018

Revert "child_process: change windowsHide default to true"
This reverts commit 420d8af.

PR-URL: #24034
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Ujjwal Sharma <usharma1998@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>

BridgeAR added a commit that referenced this pull request Nov 15, 2018

2018-11-15, Version 11.2.0 (Current)
Notable changes:

* deps:
  * A new experimental HTTP parser (`llhttp`) is now supported.
    [#24059](#24059)
* util:
  * The `util.inspect` `depth` option default is reset to 2.
    [#24326](#24326)
* Windows
  * A crashing process will now show the names of stack frames if the
    node.pdb file is available.
    [#23822](#23822)
  * Continued effort to improve the installer's new stage that installs
    native build tools.
    [#23987](#23987),
    [#24348](#24348)
  * child_process:
    * On Windows the `windowsHide` option default was restored to
      `false`. This means `detached` child processes and GUI apps will
      once again start in a new window.
      [#24034](https.//github.com//pull/24034)
* Added new collaborators:
  * [oyyd](https://github.com/oyyd) - Ouyang Yadong.
    [#24300](#24300)
  * [psmarshall](https://github.com/psmarshall) - Peter Marshall.
    [#24170](#24170)
  * [shisama](https://github.com/shisama) - Masashi Hirano.
    [#24136](#24136)

BridgeAR added a commit that referenced this pull request Nov 15, 2018

2018-11-15, Version 11.2.0 (Current)
Notable changes:

* deps:
  * A new experimental HTTP parser (`llhttp`) is now supported.
    [#24059](#24059)
* util:
  * The `util.inspect` `depth` option default is reset to 2.
    [#24326](#24326)
* timers:
  * Fixed an issue that could cause setTimeout to stop working as
    expected.
    [#24322](#24322)
* Windows
  * A crashing process will now show the names of stack frames if the
    node.pdb file is available.
    [#23822](#23822)
  * Continued effort to improve the installer's new stage that installs
    native build tools.
    [#23987](#23987),
    [#24348](#24348)
  * child_process:
    * On Windows the `windowsHide` option default was restored to
      `false`. This means `detached` child processes and GUI apps will
      once again start in a new window.
      [#24034](https.//github.com//pull/24034)
* Added new collaborators:
  * [oyyd](https://github.com/oyyd) - Ouyang Yadong.
    [#24300](#24300)
  * [psmarshall](https://github.com/psmarshall) - Peter Marshall.
    [#24170](#24170)
  * [shisama](https://github.com/shisama) - Masashi Hirano.
    [#24136](#24136)

BridgeAR added a commit that referenced this pull request Nov 15, 2018

2018-11-15, Version 11.2.0 (Current)
Notable changes:

* deps:
  * A new experimental HTTP parser (`llhttp`) is now supported.
    #24059
* timers:
  * Fixed an issue that could cause setTimeout to stop working as
    expected. #24322
* util:
  * The `util.inspect` `depth` option default is reset to 2.
    #24326
* Windows
  * A crashing process will now show the names of stack frames if the
    node.pdb file is available.
    #23822
  * Continued effort to improve the installer's new stage that installs
    native build tools.
    #23987,
    #24348
  * child_process:
    * On Windows the `windowsHide` option default was restored to
      `false`. This means `detached` child processes and GUI apps will
      once again start in a new window.
      #24034
* Added new collaborators:
  * [oyyd](https://github.com/oyyd) - Ouyang Yadong.
    #24300
  * [psmarshall](https://github.com/psmarshall) - Peter Marshall.
    #24170
  * [shisama](https://github.com/shisama) - Masashi Hirano.
    #24136

BridgeAR added a commit that referenced this pull request Nov 15, 2018

2018-11-15, Version 11.2.0 (Current)
Notable changes:

* deps:
  * A new experimental HTTP parser (`llhttp`) is now supported.
    #24059
* timers:
  * Fixed an issue that could cause setTimeout to stop working as
    expected. #24322
* Windows
  * A crashing process will now show the names of stack frames if the
    node.pdb file is available.
    #23822
  * Continued effort to improve the installer's new stage that installs
    native build tools.
    #23987,
    #24348
  * child_process:
    * On Windows the `windowsHide` option default was restored to
      `false`. This means `detached` child processes and GUI apps will
      once again start in a new window.
      #24034
* Added new collaborators:
  * [oyyd](https://github.com/oyyd) - Ouyang Yadong.
    #24300
  * [psmarshall](https://github.com/psmarshall) - Peter Marshall.
    #24170
  * [shisama](https://github.com/shisama) - Masashi Hirano.
    #24136

targos added a commit that referenced this pull request Nov 15, 2018

2018-11-15, Version 11.2.0 (Current)
Notable changes:

* deps:
  * A new experimental HTTP parser (`llhttp`) is now supported.
    #24059
* timers:
  * Fixed an issue that could cause setTimeout to stop working as
    expected. #24322
* Windows
  * A crashing process will now show the names of stack frames if the
    node.pdb file is available.
    #23822
  * Continued effort to improve the installer's new stage that installs
    native build tools.
    #23987,
    #24348
  * child_process:
    * On Windows the `windowsHide` option default was restored to
      `false`. This means `detached` child processes and GUI apps will
      once again start in a new window.
      #24034
* Added new collaborators:
  * [oyyd](https://github.com/oyyd) - Ouyang Yadong.
    #24300
  * [psmarshall](https://github.com/psmarshall) - Peter Marshall.
    #24170
  * [shisama](https://github.com/shisama) - Masashi Hirano.
    #24136

targos added a commit that referenced this pull request Nov 15, 2018

2018-11-15, Version 11.2.0 (Current)
Notable changes:

* deps:
  * A new experimental HTTP parser (`llhttp`) is now supported.
    #24059
* timers:
  * Fixed an issue that could cause setTimeout to stop working as
    expected. #24322
* Windows
  * A crashing process will now show the names of stack frames if the
    node.pdb file is available.
    #23822
  * Continued effort to improve the installer's new stage that installs
    native build tools.
    #23987,
    #24348
  * child_process:
    * On Windows the `windowsHide` option default was restored to
      `false`. This means `detached` child processes and GUI apps will
      once again start in a new window.
      #24034
* Added new collaborators:
  * [oyyd](https://github.com/oyyd) - Ouyang Yadong.
    #24300
  * [psmarshall](https://github.com/psmarshall) - Peter Marshall.
    #24170
  * [shisama](https://github.com/shisama) - Masashi Hirano.
    #24136

targos added a commit that referenced this pull request Nov 15, 2018

2018-11-15, Version 11.2.0 (Current)
Notable changes:

* deps:
  * A new experimental HTTP parser (`llhttp`) is now supported.
    #24059
* timers:
  * Fixed an issue that could cause setTimeout to stop working as
    expected. #24322
* Windows
  * A crashing process will now show the names of stack frames if the
    node.pdb file is available.
    #23822
  * Continued effort to improve the installer's new stage that installs
    native build tools.
    #23987,
    #24348
  * child_process:
    * On Windows the `windowsHide` option default was restored to
      `false`. This means `detached` child processes and GUI apps will
      once again start in a new window.
      #24034
* Added new collaborators:
  * [oyyd](https://github.com/oyyd) - Ouyang Yadong.
    #24300
  * [psmarshall](https://github.com/psmarshall) - Peter Marshall.
    #24170
  * [shisama](https://github.com/shisama) - Masashi Hirano.
    #24136

PR-URL: #24350
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment