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.

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

tool_doswin: Restore original console settings on CTRL signal #6226

Closed
wants to merge 1 commit into from

Conversation

@jay
Copy link
Member

@jay jay commented Nov 19, 2020

  • Move Windows terminal init code from tool_main to tool_doswin.

  • Restore the original console settings on CTRL+C and CTRL+BREAK.

Background: On Windows the curl tool changes the console settings to
enable virtual terminal processing (eg color output) if supported
(ie Win 10). The original settings are restored on exit but prior to
this change were not restored in the case of the CTRL signals.

Closes #xxxx


@bitcrazed can you take a look at this please. It was my understanding from #3008 that if VT was set on the console then it needed to be restored on exit. Currently that doesn't happen on CTRL+C / CTRL+BREAK signals, so I made some changes in this PR to address that.

However.. after that I did some empirical testing in Windows 10 20H2 that makes me wonder if restoring the console's original vt setting is necessary in any case, since the system appears to do that already. Take for example the C program below that was run twice from the same command prompt console and turns on VT without shutting it off again:

foo\test
VT processing is off
VT processing is on

foo\test
VT processing is off
VT processing is on
#ifndef ENABLE_VIRTUAL_TERMINAL_PROCESSING
#define ENABLE_VIRTUAL_TERMINAL_PROCESSING 0x0004
#endif

HANDLE hStdOut = INVALID_HANDLE_VALUE;
DWORD dwOutputMode = 0;

#define ABORT(x) do { \
  printf("#x failed.\n"); \
  return 1; \
} while(0)

#define SHOWVT() do { \
  printf("VT processing is %s\n", \
    (dwOutputMode & ENABLE_VIRTUAL_TERMINAL_PROCESSING) ? "on" : "off"); \
} while(0)

int main(int argc, char *argv[])
{

  hStdOut = GetStdHandle(STD_OUTPUT_HANDLE);
  if(hStdOut == INVALID_HANDLE_VALUE)
    ABORT("GetStdHandle");

  if(!GetConsoleMode(hStdOut,&dwOutputMode))
    ABORT("GetConsoleMode");
  
  SHOWVT();

  if(!SetConsoleMode(hStdOut, (dwOutputMode |
                               ENABLE_VIRTUAL_TERMINAL_PROCESSING)))
    ABORT("SetConsoleMode");

  if(!GetConsoleMode(hStdOut,&dwOutputMode))
    ABORT("GetConsoleMode");
  
  SHOWVT();

  return 0;
}

Ref: https://docs.microsoft.com/en-us/windows/console/ctrl-c-and-ctrl-break-signals

@jay jay added the Windows label Nov 19, 2020

static void restore_terminal(void)
{
if(InterlockedExchange(&TerminalSettings.valid, (LONG)FALSE))
Copy link
Contributor

@bitcrazed bitcrazed Dec 11, 2020

Perhaps I am missing something, but I am interested in why InterlockedExchange() is being used here (and below) when setting the value for a struct which (as far as I can see) will only be called from the main thread, once, during startup.

Copy link
Member Author

@jay jay Dec 11, 2020

restore_terminal can be called from the signal handler which the system calls from a separate thread

Copy link
Contributor

@bitcrazed bitcrazed Dec 12, 2020

Ah, got it. Thanks. Didn't realize that.

/* The signal handler is set before attempting to change the console mode
because otherwise a signal would not be caught after the change but
before the handler was installed. */
(void)InterlockedExchange(&TerminalSettings.valid, (LONG)TRUE);
Copy link
Contributor

@bitcrazed bitcrazed Dec 11, 2020

See question above re. use of InterlockedExchange() here too,

@jay
Copy link
Member Author

@jay jay commented Jan 9, 2021

@bitcrazed could you please address whether it's necessary to restore the console's vt setting on process termination, as mentioned in the first post

@bitcrazed
Copy link
Contributor

@bitcrazed bitcrazed commented Jan 9, 2021

Apologies for my delayed reply. It's been a little nuts at work getting all the balls rolling after Xmas. I'll double-check and get back to you all.

@bitcrazed
Copy link
Contributor

@bitcrazed bitcrazed commented Jan 10, 2021

Okay, I've confirmed with the team:

Console modes are currently global state. While the team is aiming to eventually make modes per-process, that work has yet to land.

So command-line apps should save/restore Console modes on entry/exit.

@jay
Copy link
Member Author

@jay jay commented Jan 10, 2021

Console modes are currently global state.

Thanks. If the VT state global why is it resetting on process termination (as seen in the first post)?

@bitcrazed
Copy link
Contributor

@bitcrazed bitcrazed commented Jan 11, 2021

For back-compat reasons, Console and Terminal handle modes differently. And depending on what shell you're using, you may see VT mode being enabled or disabled by default.

For example, if you run your ConsoleMode sample in PowerShell, Windows PowerShell, or Cmd in the (legacy) Console, VT is off by default and is rest on process exit. But when using Terminal, VT mode is on by default. Other shell/terminal combo's may choose different options.

So, the best approach is to be a good command-line citizen and save/restore mode state on entry/exit so that your app knows what state it's in and considerately cleans up after itself.

image

- Move Windows terminal init code from tool_main to tool_doswin.

- Restore the original console settings on CTRL+C and CTRL+BREAK.

Background: On Windows the curl tool changes the console settings to
enable virtual terminal processing (eg color output) if supported
(ie Win 10). The original settings are restored on exit but prior to
this change were not restored in the case of the CTRL signals.

VT default behavior varies; refer to the discussion in curl#6226.

Assisted-by: Rich Turner

Closes #xxxx
@jay jay force-pushed the fix_console_restore branch from 1e77a0b to 626e49b Jan 11, 2021
@jay
Copy link
Member Author

@jay jay commented Jan 11, 2021

So, the best approach is to be a good command-line citizen and save/restore mode state on entry/exit so that your app knows what state it's in and considerately cleans up after itself.

Got it. Thanks again.

@bitcrazed
Copy link
Contributor

@bitcrazed bitcrazed commented Jan 11, 2021

No problem - thanks for making these improvements - let me know if you need any info/assistance 😁

@jay jay closed this in 3831043 Jan 12, 2021
@jay jay deleted the fix_console_restore branch Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants