[rush] Fix issue where package.json script hangs when accessing STDIN on Windows

[octogonz]

Summary

The problem I encountered:

1. clone [api-extractor] Fix an issue where destructured parameters were handled incorrectly #5559 on Windows
2. rush install && rush test from cmd.exe

The process hangs while building api-documenter-scenarios, which uses a Heft "run-script-plugin" to invoke runScenarios.ts which runs node.exe api-documenter/lib/start.js. The actual hang is somewhere in API Documenter, but if you simply add console.log(process.stdin.isTTY) to the top of api-documenter/lib/start.js it will hang on that line forever.

Details

Presumably something goes wrong during lazy initialization of the process.stdin property, which probably is a deep Node.js bug on Windows. Windows TTY/streams have unusual semantics, and the Node maintainers don't put as much effort into Windows oddities. I found several GitHub issues about STDIN hangs that were opened and later closed without any code change.

A simple way to avoid the hang is to change this code:

rushstack/build-tests/api-documenter-scenarios/src/runScenarios.ts

Lines 46 to 59 in be05af7

	const childProcess: ChildProcess = Executable.spawn(
	process.argv0,
	[
	'node_modules/@microsoft/api-documenter/lib/start',
	'generate',
	`--input-folder`,
	`temp/etc/${scenarioFolderName}`,
	'--output-folder',
	`temp/etc/${scenarioFolderName}/markdown`
	],
	{
	stdio: 'inherit'
	}
	);

...to use stdio: ['ignore', 'inherit', 'inherit'].

But I did not make that fix, because it seems fundamentally unreasonable: What's wrong with inheriting STDIN? And why does it only repro when invoked by Rush?

After a bunch of debugging, I realized Rush is doing something wrong:

When spawning a child process, in the case where it binds STDOUT/STDERR to pipe to redirect the output to the stream collator, Rush also seems to bind STDIN to pipe. BUT Rush never actually attaches any handlers to the STDIN stream, nor does it close it properly. This doesn't justify the Node.js hang when accessing stdin.isTTY, but it does raise the possibility of a hang if the child process tries to read STDIN and expects the stream to get closed later, since it's not a TTY.

Looking around in Utilities.ts, there seem to be other copy+pastes of this same mistake, however I didn't have time to analyze them deeply. If someone else wants to suggest other fixes, that would be very welcome.

How it was tested

Hang repros without the fix. Build does not hang with this fix.

[dmichon-msft]

We should ABSOLUTELY not be inheriting stdin, that would imply that anything the user types would get unexpectedly forwarded to some arbitrary set of child processes.

[dmichon-msft]

In fact, in general, the else case that inherits stdin is also not a sensible default; the caller should have to expressly specify if they want stdin, otherwise we should default to ignore.

[octogonz]

What you are saying certainly makes sense whenrush build is running shell scripts and piping their output to the summarizer. The problem underlying this PR is that we have many code paths that run scripts, but no rigorous indication of which scenarios should accept STDIN.

Tracing the callers of _executeLifecycleCommandInternal():

scenario that runs lifecycle script	should accept STDIN?	ILifecycleCommandOptions.handleOutput
"hook" scripts in rush.json	maybe? originally they were for telemetry only, but what about postRushx?	!printEventHooksOutputToConsole from experiments.json
"global" Rush custom commands	yes	false
rushx	yes	false
ShellOperationRunner for phased builds	no; @dmichon-msft I think this is what you are focusing on	true
IPCOperationRunner	no? this is used to invoke Heft for multi-project watch mode, so I guess interactive commands don't make sense there?	true

I'm not even sure handleOutput should be what determines this; look at its docs:

export interface ILifecycleCommandOptions {
  /**
   * If true, suppress the process's output, but if there is a nonzero exit code then print stderr
   */
  handleOutput: boolean;

Maybe it should be renamed to handleOutputAndIgnoreStdin?

[octogonz]

Your suggestion was this:

- const stdio: child_process.StdioOptions = handleOutput ? [0, 'pipe', 'pipe'] : [0, 1, 2];
+ const stdio: child_process.StdioOptions = handleOutput ? ['ignore', 'pipe', 'pipe'] : [0, 1, 2];

It definitely breaks less table rows. The main impact would be Rush global commands, but I suppose that was already broken with pipe?

[octogonz]

I updated the branch with this change.

[octogonz]

Observations from our discussion:

1. We should audit the Rush Stack code base: When spawning processes, we should never specify pipe without actually accessing it (attaching event handlers and ensuring the pipe is closed). If it's not being accessed, use ignore instead.
2. Wherever a command spawns subprocesses, we should clearly specify whether that user scenario supports STDIN (rushx do-thing can ask the user questions, or allow rushx do-thing < input.txt).
3. David's opinion was that perhaps STDIN should be NOT supported by default; this certainly makes sense for CI, although it might be counterintuitive for other cases like Rush hooks.
4. The name of ILifecycleCommandOptions.handleOutput could be improved.