Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 2 commits into from
Jan 13, 2025
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7bb253a
Console.Unix: also reset terminal state when process terminates due t…
tmds Jan 10, 2025
24f175f
PR feedback.
tmds Jan 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions ...ries/Common/src/Interop/Unix/System.Native/Interop.InitializeTerminalAndSignalHandling.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,9 @@ internal static partial class Sys

[LibraryImport(Libraries.SystemNative, EntryPoint = "SystemNative_SetKeypadXmit", StringMarshalling = StringMarshalling.Utf8)]
internal static partial void SetKeypadXmit(SafeFileHandle terminalHandle, string terminfoString);

[LibraryImport(Libraries.SystemNative, EntryPoint = "SystemNative_UninitializeTerminal")]
[return: MarshalAs(UnmanagedType.Bool)]
tmds marked this conversation as resolved.
Show resolved Hide resolved
internal static partial void UninitializeTerminal();
}
}
4 changes: 4 additions & 0 deletions src/libraries/System.Console/src/System/ConsolePal.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -924,6 +924,10 @@ private static unsafe void EnsureInitializedCore()

// Mark us as initialized
s_initialized = true;

// InitializeTerminalAndSignalHandling will reset the terminal on a normal exit.
// This also resets it for termination due to an unhandled exception.
AppDomain.CurrentDomain.UnhandledException += delegate { Interop.Sys.UninitializeTerminal(); };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

tmds marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Expand Down
1 change: 1 addition & 0 deletions src/native/libs/System.Native/entrypoints.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ static const Entry s_sysNative[] =
DllImportEntry(SystemNative_GetWindowSize)
DllImportEntry(SystemNative_IsATty)
DllImportEntry(SystemNative_InitializeTerminalAndSignalHandling)
DllImportEntry(SystemNative_UninitializeTerminal)
DllImportEntry(SystemNative_SetKeypadXmit)
DllImportEntry(SystemNative_GetControlCharacters)
DllImportEntry(SystemNative_StdinReady)
Expand Down
9 changes: 7 additions & 2 deletions src/native/libs/System.Native/pal_console.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,8 +194,8 @@ static bool ConfigureTerminal(bool signalForBreak, bool forChild, uint8_t minCha

void UninitializeTerminal(void)
{
// This method is called on SIGQUIT/SIGINT from the signal dispatching thread
// and on atexit.
// This method is called on SIGQUIT/SIGINT from the signal dispatching thread,
// on atexit, and for AppDomain.UnhandledException.

if (pthread_mutex_lock(&g_lock) == 0)
{
Expand Down Expand Up @@ -473,3 +473,8 @@ int32_t SystemNative_InitializeTerminalAndSignalHandling(void)

return initialized;
}

void SystemNative_UninitializeTerminal(void)
{
UninitializeTerminal();
}
5 changes: 5 additions & 0 deletions src/native/libs/System.Native/pal_console.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,11 @@ PALEXPORT int32_t SystemNative_GetSignalForBreak(void);
*/
PALEXPORT int32_t SystemNative_SetSignalForBreak(int32_t signalForBreak);

/**
* Resets the terminal to the state it was in before calling InitializeTerminalAndSignalHandling.
*/
PALEXPORT void SystemNative_UninitializeTerminal(void);

typedef enum
{
Interrupt = 0,
Expand Down
3 changes: 3 additions & 0 deletions src/native/libs/System.Native/pal_console_wasi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,3 +117,6 @@
{
return true;
}

void SystemNative_UninitializeTerminalAndSignalHandling(void)

Check failure on line 121 in src/native/libs/System.Native/pal_console_wasi.c

View check run for this annotation

Azure Pipelines / runtime (Build wasi-wasm linux Release WasmBuildTests)

src/native/libs/System.Native/pal_console_wasi.c#L121 

src/native/libs/System.Native/pal_console_wasi.c(121,6): error GF7F7EAD1: (NETCORE_ENGINEERING_TELEMETRY=Build) no previous prototype for function 'SystemNative_UninitializeTerminalAndSignalHandling' [-Wmissing-prototypes]

Check failure on line 121 in src/native/libs/System.Native/pal_console_wasi.c

View check run for this annotation

Azure Pipelines / runtime (Build wasi-wasm linux Release LibraryTests_Smoke)

src/native/libs/System.Native/pal_console_wasi.c#L121 

src/native/libs/System.Native/pal_console_wasi.c(121,6): error GBFE5B470: (NETCORE_ENGINEERING_TELEMETRY=Build) no previous prototype for function 'SystemNative_UninitializeTerminalAndSignalHandling' [-Wmissing-prototypes]

Check failure on line 121 in src/native/libs/System.Native/pal_console_wasi.c

View check run for this annotation

Azure Pipelines / runtime

src/native/libs/System.Native/pal_console_wasi.c#L121 

src/native/libs/System.Native/pal_console_wasi.c(121,6): error GBFE5B470: (NETCORE_ENGINEERING_TELEMETRY=Build) no previous prototype for function 'SystemNative_UninitializeTerminalAndSignalHandling' [-Wmissing-prototypes]

Check failure on line 121 in src/native/libs/System.Native/pal_console_wasi.c

View check run for this annotation

Azure Pipelines / runtime

src/native/libs/System.Native/pal_console_wasi.c#L121 

src/native/libs/System.Native/pal_console_wasi.c(121,6): error GF7F7EAD1: (NETCORE_ENGINEERING_TELEMETRY=Build) no previous prototype for function 'SystemNative_UninitializeTerminalAndSignalHandling' [-Wmissing-prototypes]
tmds marked this conversation as resolved.
Show resolved Hide resolved
{ }
Loading