fix(compile): panic when compiled with --no-terminal flag

[JasperVanEsveld]

This works towards fixing #27730 and #21091.

When compiled with the --no-terminal flag under windows there are some issues related to stdin, stdout and stderr.
This pull request fixes the panic when during bootstrap the terminal type is requested.
Previously it would return the error if the terminal was not valid, instead it now returns UNKNOWN.

The dummy stdin that is than provided instantly pushes null for some reason.
However this tries to do a debugLog which is not possible yet, as we are still initiating.
I don't see the use for this push, so I removed the line.
If it is needed for something please let me know.

This PR does not fully fix the issue yet, as there is a similar problem in deno_core's isTerminal op, see this pull request.

[JasperVanEsveld]

Can some one please review this one? 🙏
@ry merged a related PR in deno-core, if this one is also merged the issues can be closed.

Not sure if/who I should mention to get more eyes on it, or just have patience...

[bartlomieju]

Why is this change needed? Seems quite risky without any tests attached

[JasperVanEsveld]

This change was needed because it used debugLog somewhere in _streams.mjs.
But that is not yet possible because the debug environment is not yet initialized which causes it to panic.
However, I will need to check again if this is needed because #28855 got merged.

I didn't understand what the line was for but looking at the Node API it is supposed to end the stream when you push null. I am not entirely sure if this is needed, stdin is normally not instantly ended.
I don't think it is needed to end the stream for compatibility reasons with Node, as I don't think there is a similar feature for node.

In case the stdin stream does need to end, the push needs to be called after initializeDebugEnv(nodeDebug). To do that you would again have to check that the stream is a dummy stdin.

deno/ext/node/polyfills/process.ts

Lines 999 to 1025 in 84b192e

	const newStdin = initStdin();
	if (newStdin) {
	stdin = process.stdin = newStdin;
	}
	// Replace stdout/stderr if they are not terminals
	if (!io.stdout.isTerminal()) {
	/** https://nodejs.org/api/process.html#process_process_stdout */
	stdout = process.stdout = createWritableStdioStream(
	io.stdout,
	"stdout",
	);
	}
	if (!io.stderr.isTerminal()) {
	/** https://nodejs.org/api/process.html#process_process_stderr */
	stderr = process.stderr = createWritableStdioStream(
	io.stderr,
	"stderr",
	);
	}
	arch = arch_();
	platform = isWindows ? "win32" : Deno.build.os;
	pid = Deno.pid;
	initializeDebugEnv(nodeDebug);

Let me first check if debugLog is still called

[dsherret]

Is this change related to the issue in this PR? Can we just merge the other change in here?

[bartlomieju]

I agree with David, it probably should be added in another PR (if it's needed at all)

[JasperVanEsveld]

It was related to the issue before, as this was one of the points where it would panic when running without a terminal.
Just checked and it no longer calls debugLog or at least no longer panics so this is not required anymore! 👍

[bartlomieju]

@JasperVanEsveld thanks for the PR, is there a test we could add that exercises this change?

[JasperVanEsveld]

@bartlomieju
A test I can think of is to check if HandleType::Unknown is returned when it uses an OpState that throws an error.
Which should happen if it is given an empty OpState.

I tried to do this but the compiler tells me op_node_guess_handle_type takes 0 arguments.
Is there an easy way to test opcode functions, so functions that use #[op2]?

I tried something like this:

#[cfg(test)]
mod tests {
  use deno_core::OpState;
  use super::*;

  #[test]
  fn guesshandletype_emptystate() {
    let empty_state = OpState::new(None, None);
    let actual = op_node_guess_handle_type(empty_state, 0);
    assert_eq!(actual, HandleType::Unknown);
  }
}

[dsherret]

LGTM. Thanks!

[JasperVanEsveld]

@bartlomieju @dsherret
I think I accidentally used the wrong executable before, because the new releases still panics.
The stdin.push(null); line still causes a panic, delaying the initialization of stdin until after debug environment is initialized fixes this issue. Meaning that stdin.push(null); does not have to be removed.

#29144 does exactly that.
Please take a look when either of you have the time