Skip to content

[release/6.0] Console.Unix: fix missing terminal configuration on SIGINT/SIGQUIT/SGCONT. #64985

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

Jump to bottom
Merged
carlossanlop merged 1 commit into from
Aug 11, 2022
Merged

[release/6.0] Console.Unix: fix missing terminal configuration on SIGINT/SIGQUIT/SGCONT. #64985

carlossanlop merged 1 commit into from
Aug 11, 2022

Conversation

@github-actions
Copy link
Contributor

@github-actions github-actions bot commented Feb 8, 2022 •
edited by danmoseley
Loading

Backport of #64200 to release/6.0

/cc @adamsitnik @tmds

Customer Impact

Customer reported that if a console app is waiting for keyboard input via Console.Read, and the user quits the app with Ctrl-C, the terminal will be left in a broken state, and typing will not be displayed.

This is due to a regression introduced in .NET 6 where we fail to re-enable input echoing on Ctrl+C.

Testing

Manually verified that this fix solves the problem.

Risk

Very low, as this change has been in main (7.0) for half year and no regressions or bugs were reported so far.

@tmds
Console.Unix: fix missing terminal configuration on SIGINT/SIGQUIT/SI…
ebd62f2 
…GCONT.

The introduction of the managed API for signal handling (PosixSignal)
inadvertently caused terminal configuration to no longer be performed
when no managed handlers are registered.

This adds back the unconditional registration for SIGINT/SIGQUIT/SIGCONT.

The missing registrations can cause the terminal to stop echoing when an
application terminates on Ctrl-C.
@ghost ghost added the area-System.Net label Feb 8, 2022
@ghost
Copy link

ghost commented Feb 8, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #64200 to release/6.0

/cc @adamsitnik @tmds

Customer Impact

Testing

Risk

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Net
Milestone: -

@ghost
Copy link

ghost commented Feb 8, 2022

Tagging subscribers to this area: @dotnet/area-system-console
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #64200 to release/6.0

/cc @adamsitnik @tmds

Customer Impact

Testing

Risk

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Console, area-System.Net
Milestone: -

@akoeplinger akoeplinger added the Servicing-consider Issue for next servicing release review label Mar 11, 2022
@akoeplinger akoeplinger added this to the 6.0.x milestone Mar 11, 2022
@akoeplinger
Copy link
Member

akoeplinger commented Mar 11, 2022 •
edited
Loading

@ericstj @adamsitnik looks like this was missed, probably because it wasn't in an 6.0.x milestone and/or Servicing-consider label. I've did that now.

@ericstj ericstj removed the Servicing-consider Issue for next servicing release review label Mar 11, 2022
@ericstj
Copy link
Member

ericstj commented Mar 11, 2022 •
edited
Loading

servicing-consider should not be added until the template is filled out and the PR is approved by at least one reviewer. I reached out yesterday to the folks that are responsible for this and they let me know it was not ready for this release. There's still time today if they want it. cc @adamsitnik @jeffhandley

@carlossanlop
Copy link
Contributor

carlossanlop commented Mar 11, 2022 •
edited
Loading

@akoeplinger the template has not yet been filled. I don't think we will be able to get this merged, since the deadline for the next servicing release is today, and it has not yet been approved by Tactics. Have you sent an email to Tactics?

@carlossanlop carlossanlop added Servicing-consider Issue for next servicing release review and removed Servicing-consider Issue for next servicing release review labels Mar 11, 2022
@akoeplinger
Copy link
Member

akoeplinger commented Mar 11, 2022

@carlossanlop I have not since I'm not driving this PR, I merely noticed this was sitting around for a month while I was going through the list of 6.0 PRs.

@carlossanlop
Copy link
Contributor

carlossanlop commented Mar 11, 2022

Ah, my apologies @akoeplinger 😄

@jeffhandley
Copy link
Member

jeffhandley commented Mar 11, 2022

Thanks all. Due to several disruptions, we were not able to follow-through on getting this ready for consideration/approval. It'll have to carry over to the next servicing release.

@ViktorHofer
Copy link
Member

ViktorHofer commented Apr 19, 2022

@jeffhandley is now a good time to consider this PR for the next servicing release, given that time passed? Just asking as I noticed that this PR is lying around.

@tmds
Copy link
Member

tmds commented Apr 25, 2022

imo we are good to include this.

@carlossanlop
Copy link
Contributor

carlossanlop commented May 3, 2022

@jeffhandley @jozkee @adamsitnik May 16th is the due date for Code Complete for June servicing. Do we want this to go through Tactics?

@danmoseley
Copy link
Member

danmoseley commented Jun 14, 2022

closing this as it's static. if someone wants to champion - please recreate, with the usual template.

@danmoseley danmoseley closed this Jun 14, 2022
@danmoseley danmoseley deleted the backport/pr-64200-to-release/6.0 branch June 14, 2022 02:57
@tmds
Copy link
Member

tmds commented Jun 14, 2022

I run into this issue, so I believe others do to.
It's annoying when the terminal stops echoing.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 14, 2022
@adamsitnik adamsitnik restored the backport/pr-64200-to-release/6.0 branch July 18, 2022 14:30
@adamsitnik adamsitnik added the Servicing-consider Issue for next servicing release review label Jul 18, 2022
@adamsitnik adamsitnik self-assigned this Jul 18, 2022
@adamsitnik adamsitnik reopened this Jul 18, 2022
@dotnet dotnet unlocked this conversation Jul 18, 2022
@adamsitnik
Copy link
Member

adamsitnik commented Jul 18, 2022

I am going to champion this issue. I've filled the template and sent an email. This is really annoying bug BTW ;)

@adamsitnik adamsitnik mentioned this pull request Jul 18, 2022
Closed
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jul 19, 2022
@rbhanda rbhanda modified the milestones: 6.0.x, 6.0.9 Jul 19, 2022
carlossanlop
carlossanlop approved these changes Aug 11, 2022
View reviewed changes
Copy link
Contributor

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

Tactics approved.
Correct milestone applied.
CI failure is unrelated.
No OOB package authoring needed since this is native code.
Ready to merge.

@carlossanlop carlossanlop merged commit 12e90e7 into release/6.0 Aug 11, 2022
@carlossanlop carlossanlop deleted the backport/pr-64200-to-release/6.0 branch August 11, 2022 17:45
@adamsitnik adamsitnik mentioned this pull request Sep 6, 2022
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Sep 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@adamsitnik adamsitnik adamsitnik approved these changes

+1 more reviewer

@carlossanlop carlossanlop carlossanlop approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@adamsitnik adamsitnik

Labels

area-System.Console Servicing-approved Approved for servicing release

Projects

None yet

Milestone

6.0.9

Development

Successfully merging this pull request may close these issues.