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

Lock and Unlock in conhost should decouple Ctrl+C dispatch and use smarter handling #2141

Open
miniksa opened this issue Jul 29, 2019 · 3 comments
Labels
Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase
Milestone

Comments

@miniksa
Copy link
Member

miniksa commented Jul 29, 2019

The Lock and Unlock procedures in Conhost.exe are fraught with error.

For one, we are using intricate details of how many recursive entries there are on the lock to count when it is fully unlocked and dispatch Ctrl+C events. We're not supposed to be dependent on the lock count at all.

Additionally, there's multiple layers at which the locks can be processed, some of which DO dispatch the Ctrl+C events on the last unlock and some of which do NOT.

Finally, the lock/unlock procedure is bad in that it is easy to not unlock something since it's not in any sort of smart RAII-type object. This is sort of mitigated by wil::scope_exit in many places, but that function is also discouraged when you can use something better.

This issue encompasses rooting around and finding a better overall way to do all of this.

It does NOT encompass more granular locks than already exist.

@miniksa miniksa added Product-Conhost For issues in the Console codebase Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Issue-Task It's a feature request, but it doesn't really need a major design. labels Jul 29, 2019
@miniksa miniksa self-assigned this Jul 29, 2019
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jul 29, 2019
@zadjii-msft
Copy link
Member

where's the "crying inside" reaction when you need it

@miniksa
Copy link
Member Author

miniksa commented Jul 29, 2019

🤪

@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jul 29, 2019
@DHowett-MSFT
Copy link
Contributor

Yanking triage ;P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase
Projects
None yet
Development

No branches or pull requests

3 participants