Breaking Change Request: Don't restore terminal state on exit

[sortie]

Proposal

We propose to make the Dart VM stop automatically restoring the terminal settings upon exit. A proposed implementation is at https://dart-review.googlesource.com/c/sdk/+/190484.

Programs that set Stdin.echoMode or Stdin.lineMode must now manually restore the terminal settings upon exit if they wish.

Rationale

The automatic terminal setting restoration happens unconditionally and can lead to undesirable behavior e.g. when the program does not care about terminal settings and is sharing a process group with a program that does care. E.g. if dart's output is piped into less(1), then there is a race condition where dart might see the raw terminal settings set by less(1), and if the dart VM exits after less(1) has exited, then it will restore the raw terminal settings, leaving the user with a seemingly defective shell with echo disabled. This race condition can be reproduced using:

cat > yes.dart << EOF
main() {
  for (int i = 0; i < 1000000; i++) {
    print("yes");
  }
}
EOF
stty; (sleep 1 && dart yes.dart) | less; stty; stty sane

The user will end up with a shell with echo behavior disabled. The stty command shows the current terminal settings, where the difference can be seen, and stty sane fixes the settings before returning to the shell prompt. The stty sane call can be omitted to see the defective shell prompt.

dart:io already provides all the functionality for programs to correctly implement the restoration behavior themselves. This class of terminal setting corruption bugs are eliminated by shifting the burden onto the programs that use interactive input and change these settings. This design matches that of other programming languages.

Impact

Affected programs won't break for the duration of their run, although the terminal state settings for line mode and echo mode may be unexpected afterwards, especially if the program is terminated by a signal or an unhandled exception. Users of POSIX systems can manually repair their terminal session by running stty echo echonl icanon or stty sane.

Mitigation

Programs that change the Stdin settings lineMode and echoMode are now responsible for restoring the settings upon program exit. E.g. a program disabling echoMode will now need to restore the setting itself and handle exiting by the appropriate signals if desired:

import 'dart:io';
import 'dart:async';

main() {
  bool echoWasEnabled = stdin.echoMode;
  try {
    late StreamSubscription subscription;
    subscription = ProcessSignal.sigint.watch().listen((ProcessSignal signal) {
      stdin.echoMode = echoWasEnabled;
      subscription.cancel();
      Process.killPid(pid, signal); /* Die by the signal. */
    });
    stdin.echoMode = false;
  } finally {
    stdin.echoMode = echoWasEnabled;
  }
}

[Hixie]

fyi @jonahwilliams

[jonahwilliams]

Also fyi @zanderso

We already have the existing hooks to be able to restore terminal state on exit, so the required tool change should be fairly minor. My understanding is that if we get sigkilled we don't get a chance to run these, but then neither does the existing VM code? As long as we don't regress this behavior I don't imagine there would be any problems.

[zanderso]

This was originally done because the way the Dart VM sets the terminal states makes it impossible for a Dart program to explicitly restore the original state after modifying it. So backing this out will re-introduce that issue: #30318. /cc @cbracken.

[cbracken]

I can't remember if we discussed this, but is there a reason echo and echonl couldn't be modelled as separate properties? That's also a breaking change, but gives the developer the ability to correctly restore the original state.

An example where this is horrible is on zsh, which is now the default shell on macOS. Running pub run test or flutter test will reset both echo and echonl to true, when only echo should be true by default. Setting echonl causes an extra newline to be emitted on every shell command, which isn't the end of the world, but is perplexing/annoying.

Running stty echonl in zsh demos the behaviour.

[sortie]

Thanks @zanderso and @cbracken for that valuable context about the #30318 bug. A solution is required in order to prevent this bug from returning if this breaking change is made.

I propose we amend this breaking change proposal with making Stdin.echoMode only control the echo local terminal mode and introducing a new Stdin.echoNewlineMode field to control the echonl local mode on POSIX systems. On Windows, this new field will always be false and throw if enabled since Windows does not have such a mode. This amendment is also a breaking change in its own right, since the addition of a new field can break implementations of the Stdin interface (if any).

Does that resolve your concerns?

I have implemented this proposed behavior in https://dart-review.googlesource.com/c/sdk/+/190484.

cc @lrhn @aam

[cbracken]

@sortie that sounds great to me.

I'm curious if @zanderso remembers why that wasn't the proposed solution the first time; maybe just that it was a breaking change -- or we thought it was likely to be a bigger breaking change than resetting the values on exit?

[zanderso]

I don't think the reason is documented anywhere, and my memory is imperfect, but it was probably a combo of:

1. Avoiding a breaking change,
2. Avoiding thinking about how to handle it on Windows,
3. Following a general principle that the VM should clean up after itself.

On (3), I'm still a little skeptical of this change. It adds the requirement that any program that modifies the echo modes must record them before modifying them, and install signal handlers and on-exit handlers to restore them to their previous state. I suspect that there aren't many command line programs that currently have that. The flutter_tool has the signal and on-exit handlers, but I'm sure it doesn't record the previous state on startup.

But I guess this is a breaking change proposal, so the mitigation would be updating the documentation for Stdin.echoMode etc. to explain the right way to clean up after any modification of the modes.

[franklinyow]

@sortie Do you think this is at the stage for the final approval?

[itsjustkevin]

Is this still something we want to investigate? @Hixie @vsmenon @grouma

[grouma]

This shouldn't have an impact on AngularDart nor do I see the need for it.

[vsmenon]

@sortie - is this something we're still pushing on?

[aam]

I think this is still worth landing. @sortie do you have cycles to see this through?

[aam]

I have updated https://dart-review.googlesource.com/c/sdk/+/190484 so it can land once this breaking change request is approved.

[aam]

Any updates on this from the review meetings? Any objections to fixing currently broken behavior?

[sortie]

This change should be ready to roll out now if approved. It may cause some subtle breakage in some tooling that messes with the terminal settings and expects it to be restored, although I found very few cases when I tried looking last time. It'll probably be easier to find and fix these cases by landing this change, and either doing small fixes to those cases or reverting the change while the fixes are being made.

[zanderso]

Before landing, please test using the flutter tool. In particular, I'm unsure what will happen when the tool is killed by a signal during flutter run.

[aam]

I don't see any adverse effects on flutter tools being killed neither on Windows or on Linux with proposed patch applied: the console shell/window that used to launch flutter tools seems to continue normally after flutter tools is killed.

[cbracken]

Nice. Another test case might be the usecase that caused me to file the original issue. Running with zsh, run flutter test ... and verify it looks good; then do the same but kill it partway through. Ideally, neither case will result in a terminal with a changed echonl setting (evidenced by emitting an extra blank line after every command you type). Note that bash doesn't seem to have the same behaviour in that case.

[aam]

terminating running flutter test [still] seems to result in well-behaving terminal session(verified on linux, windows and, thanks to you, on mac too :-) )

[aam]

@itsjustkevin is there more information needed to make a decision on this?

[a-siva]

ping @mit-mit @vsm, @Hixie, and @grouma for approval

[grouma]

LGTM from an AngularDart perspective.

[itsjustkevin]

@Hixie @mit-mit @vsmenon pinging again for approval.

[mit-mit]

SGTM

[Hixie]

SGTM.

[vsmenon]

@a-siva @brianquinlan - you ok with the dart:io change?

Otherwise, LGTM

[a-siva]

@a-siva @brianquinlan - you ok with the dart:io change?

The changelist has been reviewed and was waiting for this breaking change approval before it could land.

[vsmenon]

lgtm

[itsjustkevin]

Team, marking this breaking change as approved.

[aam]

thank you all, will submit https://dart-review.googlesource.com/c/sdk/+/190484 shortly