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

Jump to bottom

Add recursive_ticket_lock and use it for Terminal #13746

Merged
4 commits merged into from Oct 31, 2022
Merged

Add recursive_ticket_lock and use it for Terminal #13746

4 commits merged into from Oct 31, 2022

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Aug 15, 2022 •
edited

My hope with this commit is to make our code more robust against accidental
recursive locking, as well as making it easier to write code with confidence,
with only a slight performance trade-off.

Validation Steps Performed

  • Playing Pac Man music through MIDI ✅
  • Windows Terminal runs happily ever after ✅

@lhecker lhecker added the Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. label Aug 15, 2022
@lhecker lhecker force-pushed the dev/lhecker/recursive-ticket-lock branch from fe7479b to 5be3b6d Compare August 15, 2022 21:53
DHowett
DHowett reviewed Aug 15, 2022
View reviewed changes
src/inc/til/ticket_lock.h Outdated Show resolved Hide resolved
src/inc/til/ticket_lock.h Outdated Show resolved Hide resolved
src/inc/til/ticket_lock.h Outdated Show resolved Hide resolved
@lhecker lhecker marked this pull request as ready for review August 24, 2022 01:38
zadjii-msft
zadjii-msft approved these changes Aug 25, 2022
View reviewed changes
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Seems fine to me, but maybe we hold this till we fork off 1.16 / 1.17?

src/cascadia/TerminalControl/ControlCore.cpp Outdated
terminalLock.unlock();
{
// We then unlock the terminal, so the UI doesn't hang while we're busy.
const auto suspension = _terminal->SuspendLock();
Copy link
Member

Choose a reason for hiding this comment

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

So basically, this will release the lock on the terminal lock, but then wait to take the lock again as soon as suspension goes out of scope?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep exactly. It's like an inversion of std::lock_guard. It's basically an std::unlock_guard.

@lhecker lhecker added this to the Terminal v1.17 milestone Aug 25, 2022
@lhecker lhecker added the Needs-Second It's a PR that needs another sign-off label Sep 13, 2022
@lhecker lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 14, 2022
@ghost
Copy link

ghost commented Sep 14, 2022

Hello @lhecker!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@carlos-zamora
Copy link
Member

@msftbot make sure @DHowett signs off on this

@ghost
Copy link

ghost commented Oct 17, 2022

Hello @carlos-zamora!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I'll only merge this pull request if it's approved by @DHowett

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

carlos-zamora added a commit that referenced this pull request Oct 25, 2022
@carlos-zamora
PRE-MERGE #13746 Add recursive_ticket_lock and use it for Terminal
5f9fd41
@DHowett
Copy link
Member

DHowett commented Oct 31, 2022

Have you re-tested this with the new MIDI changes? Sorry for the delay in my review lol

@lhecker
Copy link
Member Author

lhecker commented Oct 31, 2022 •
edited

Have you re-tested this with the new MIDI changes? Sorry for the delay in my review lol

I can do that, but I believe that it's not necessary. The MIDI changes work independent of our console lock mechanism and if this PR (or the other one) changed that, well... that would be quite a bug. 😅
It will work on Windows 8.1, because it only depends on WaitOnAddress which is supported on Windows 8. The performance cost is likely practically negligible, as the run to run variance during benchmarking was significantly larger than the average runtime cost of this lock.

@ghost ghost merged commit b4d37d8 into main Oct 31, 2022
@ghost ghost deleted the dev/lhecker/recursive-ticket-lock branch October 31, 2022 23:08
@ghost
Copy link

ghost commented Jan 24, 2023

🎉Windows Terminal Preview v1.17.1023 has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DHowett DHowett DHowett approved these changes

@zadjii-msft zadjii-msft zadjii-msft approved these changes

@PankajBhojwani PankajBhojwani Awaiting requested review from PankajBhojwani

@carlos-zamora carlos-zamora Awaiting requested review from carlos-zamora

Assignees

@DHowett DHowett

Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. AutoMerge Marked for automatic merge by the bot when requirements are met Needs-Second It's a PR that needs another sign-off
Projects
None yet
Milestone
Terminal v1.17
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@lhecker @carlos-zamora @DHowett @zadjii-msft