terminal icon indicating copy to clipboard operation
terminal copied to clipboard

Published 20 hours ago • verified publisher icon microsoft

Add recursive_ticket_lock and use it for Terminal

Open lhecker opened this issue 3 years ago • 1 comments
trafficstars

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 avatar Aug 15 '22 21:08 lhecker

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.

ghost avatar Sep 14 '22 14:09 ghost

@msftbot make sure @DHowett signs off on this

carlos-zamora avatar Oct 17 '22 17:10 carlos-zamora

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".

ghost avatar Oct 17 '22 17:10 ghost

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

DHowett avatar Oct 31 '22 20:10 DHowett

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.

lhecker avatar Oct 31 '22 21:10 lhecker

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

Handy links:

ghost avatar Jan 24 '23 18:01 ghost