Console.Unix: also reset terminal state when process terminates due to an unhandled exception.

[tmds]

Fixes #110502.

@adamsitnik @jkotas ptal.

[tmds]

I manually checked the behavior with this program:

if (args.Length > 0 && args[0] == "throw")
{
    Console.ReadLine();

    // Intentional crash
    int[] arr = new int[1];
    arr[100] = 10;
}
else
{
    Process.Start(Environment.ProcessPath!, [ Assembly.GetExecutingAssembly().Location, "throw"]).WaitForExit();
}

I don't think there's a good way to test this in System.Console.Manual.Tests because the .NET process will re-configure the terminal when the child exits and when using Console.ReadLine after that.

[adamsitnik]

Overall the changes LGTM, but I would prefer @jkotas to sign of the part that registers for the AppDomain.CurrentDomain.UnhandledException event as I don't know all the pros and cons of doing so.

[adamsitnik]

I think that we should do that before the s_initialized is set to true (so setting s_initialized to true is the last thing that is part of the initialization logic)

[tmds]

I put it here intentionally because we won't start modifying the terminal until we've reached this point.
At this location we're sure to only add the delegate once.

I'm fine with moving it higher up, though this location should also be fine.

@adamsitnik can you confirm you'd still like me to move this higher up?

[adamsitnik]

We won't start modifying because there is a lock, but in theory once the flag is set to true some other thread can assume everything is initialized and it can be used.

runtime/src/libraries/System.Console/src/System/ConsolePal.Unix.cs

Lines 868 to 874 in 5abf582

	internal static void EnsureConsoleInitialized()
	{
	if (!s_initialized)
	{
	EnsureInitializedCore(); // factored out for inlinability
	}
	}

I just like to have all initialization flags set to true when all the initialization logic is finished. I won't block the PR without it, but I think that in the future somebody can add some new initialization logic after your new logic and then we are going to be in trouble if we miss that in a code review.

[jkotas]

theory once the flag is set to true some other thread can assume everything is initialized and it can be used.

Does it mean that registering after s_initialized is set increases chances that the app will terminate without resetting the terminal?

[adamsitnik]

Does it mean that registering after s_initialized is set increases chances that the app will terminate without resetting the terminal?

Based on the logic for AppDomain.CurrentDomain I believe it's possible to be so (it seems to be non-trivial and can take some time, so in meantime something can use the console and then somehow crash the app).

runtime/src/libraries/System.Private.CoreLib/src/System/AppDomain.cs

Lines 31 to 43 in 5abf582

	public static AppDomain CurrentDomain
	{
	get
	{
	// Create AppDomain instance only once external code asks for it. AppDomain brings lots of unnecessary
	// dependencies into minimal trimmed app via ToString method.
	if (s_domain == null)
	{
	Interlocked.CompareExchange(ref s_domain, new AppDomain(), null);
	}
	return s_domain;
	}
	}

But the chances are very low.

[adamsitnik]

BTW this comment made me realize that simplest "Hello World" app size after trimming may grow with this change. I am still not against it, just something to keep in mind.

[jkotas]

the chances are very low.

I agree that the chances are low, but this ordering is needlessly increasing them.

simplest "Hello World" app size after trimming may grow with this change

This is true for a lot of PRs.

[adamsitnik]

This is true for a lot of PRs.

I know, but in case of Hello World there are some benchmarks that some people look at when comparing tech stacks. I just wanted to point it out since IIRC we may be having some automation that tracks such regressions. So then the person looking at it will find my comment and close it as "by design".

[tmds]

fyi, I explored what the implementation would need to implement it in a native signal handler. The challenge is that runtime will reset to the default SIGABRT handler (in PROCAbort by calling SEHCleanupSignals) and a handler that was added later for Console that chains back to the runtime handler is not called.

[adamsitnik]

@tmds big thanks for explaining why we have not relied on SIGABRT!

[jkotas]

LGTM modulo feedback

[adamsitnik]

LGTM, thank you for your help @tmds !

[adamsitnik]

/ba-g the timeout in the Windows job is unrelated