From 9a5e278ffe7dde5302c538a4adef38e4554c0e09 Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Tue, 4 May 2021 15:11:15 -0600 Subject: [PATCH] Recover ReentrantSemaphore after JTF throws an exception We already had a try/finally block, but it assumed JTF.RunAsync itself wouldn't throw an exception. In some cases it *does* throw an exception (`OutOfMemoryException` for example) and it's critical to avoiding deadlocks later on that we exit the semaphore we entered in this case so we do not deadlock later. Fixes #843 --- .../ReentrantSemaphore.cs | 220 +++++++++--------- 1 file changed, 110 insertions(+), 110 deletions(-) diff --git a/src/Microsoft.VisualStudio.Threading/ReentrantSemaphore.cs b/src/Microsoft.VisualStudio.Threading/ReentrantSemaphore.cs index 36cce6b31..62927d425 100644 --- a/src/Microsoft.VisualStudio.Threading/ReentrantSemaphore.cs +++ b/src/Microsoft.VisualStudio.Threading/ReentrantSemaphore.cs @@ -352,9 +352,9 @@ public override async Task ExecuteAsync(Func operation, CancellationToken } } - await this.ExecuteCoreAsync(async delegate + try { - try + await this.ExecuteCoreAsync(async delegate { if (this.IsJoinableTaskAware(out JoinableTaskFactory? joinableTaskFactory, out _)) { @@ -376,12 +376,12 @@ public override async Task ExecuteAsync(Func operation, CancellationToken } await operation().ConfigureAwaitRunInline(); - } - finally - { - DisposeReleaserNoThrow(releaser); - } - }); + }); + } + finally + { + DisposeReleaserNoThrow(releaser); + } } /// @@ -416,9 +416,9 @@ public override async ValueTask ExecuteAsync(Func> operation, } } - return await this.ExecuteCoreAsync(async delegate + try { - try + return await this.ExecuteCoreAsync(async delegate { if (this.IsJoinableTaskAware(out JoinableTaskFactory? joinableTaskFactory, out _)) { @@ -440,12 +440,12 @@ public override async ValueTask ExecuteAsync(Func> operation, } return await operation().ConfigureAwait(true); - } - finally - { - DisposeReleaserNoThrow(releaser); - } - }); + }); + } + finally + { + DisposeReleaserNoThrow(releaser); + } } } @@ -515,9 +515,9 @@ public override async Task ExecuteAsync(Func operation, CancellationToken } } - await this.ExecuteCoreAsync(async delegate + try { - try + await this.ExecuteCoreAsync(async delegate { if (this.IsJoinableTaskAware(out JoinableTaskFactory? joinableTaskFactory, out _)) { @@ -540,19 +540,19 @@ public override async Task ExecuteAsync(Func operation, CancellationToken this.reentrancyDetection.Value = ownedBox = new StrongBox(true); await operation().ConfigureAwaitRunInline(); - } - finally + }); + } + finally + { + // Make it clear to any forks of our ExecutionContexxt that the semaphore is no longer owned. + // Null check incase the switch to UI thread was cancelled. + if (ownedBox is object) { - // Make it clear to any forks of our ExecutionContexxt that the semaphore is no longer owned. - // Null check incase the switch to UI thread was cancelled. - if (ownedBox is object) - { - ownedBox.Value = false; - } - - DisposeReleaserNoThrow(releaser); + ownedBox.Value = false; } - }); + + DisposeReleaserNoThrow(releaser); + } } /// @@ -594,9 +594,9 @@ public override async ValueTask ExecuteAsync(Func> operation, } } - return await this.ExecuteCoreAsync(async delegate + try { - try + return await this.ExecuteCoreAsync(async delegate { if (this.IsJoinableTaskAware(out JoinableTaskFactory? joinableTaskFactory, out _)) { @@ -619,19 +619,19 @@ public override async ValueTask ExecuteAsync(Func> operation, this.reentrancyDetection.Value = ownedBox = new StrongBox(true); return await operation().ConfigureAwait(true); - } - finally + }); + } + finally + { + // Make it clear to any forks of our ExecutionContexxt that the semaphore is no longer owned. + // Null check incase the switch to UI thread was cancelled. + if (ownedBox is object) { - // Make it clear to any forks of our ExecutionContexxt that the semaphore is no longer owned. - // Null check incase the switch to UI thread was cancelled. - if (ownedBox is object) - { - ownedBox.Value = false; - } - - DisposeReleaserNoThrow(releaser); + ownedBox.Value = false; } - }); + + DisposeReleaserNoThrow(releaser); + } } /// @@ -725,11 +725,11 @@ public override async Task ExecuteAsync(Func operation, CancellationToken releaser = default; } - await this.ExecuteCoreAsync(async delegate + bool pushed = false; + var pushedReleaser = new StrongBox(releaser); + try { - bool pushed = false; - var pushedReleaser = new StrongBox(releaser); - try + await this.ExecuteCoreAsync(async delegate { if (this.IsJoinableTaskAware(out JoinableTaskFactory? joinableTaskFactory, out _)) { @@ -760,33 +760,33 @@ public override async Task ExecuteAsync(Func operation, CancellationToken } await operation().ConfigureAwaitRunInline(); - } - finally + }); + } + finally + { + try { - try + if (pushed) { - if (pushed) + lock (reentrantStack) { - lock (reentrantStack) + StrongBox? poppedReleaser = reentrantStack.Pop(); + if (!object.ReferenceEquals(poppedReleaser, pushedReleaser)) { - StrongBox? poppedReleaser = reentrantStack.Pop(); - if (!object.ReferenceEquals(poppedReleaser, pushedReleaser)) - { - // When the semaphore faults, we will drain and throw for awaiting tasks one by one. - this.faulted = true; + // When the semaphore faults, we will drain and throw for awaiting tasks one by one. + this.faulted = true; #pragma warning disable CA2219 // Do not raise exceptions in finally clauses - throw Verify.FailOperation(Strings.SemaphoreStackNestingViolated, ReentrancyMode.Stack); + throw Verify.FailOperation(Strings.SemaphoreStackNestingViolated, ReentrancyMode.Stack); #pragma warning restore CA2219 // Do not raise exceptions in finally clauses - } } } } - finally - { - DisposeReleaserNoThrow(releaser); - } } - }); + finally + { + DisposeReleaserNoThrow(releaser); + } + } } /// @@ -841,11 +841,11 @@ public override async ValueTask ExecuteAsync(Func> operation, releaser = default; } - return await this.ExecuteCoreAsync(async delegate + bool pushed = false; + var pushedReleaser = new StrongBox(releaser); + try { - bool pushed = false; - var pushedReleaser = new StrongBox(releaser); - try + return await this.ExecuteCoreAsync(async delegate { if (this.IsJoinableTaskAware(out JoinableTaskFactory? joinableTaskFactory, out _)) { @@ -876,33 +876,33 @@ public override async ValueTask ExecuteAsync(Func> operation, } return await operation().ConfigureAwait(true); - } - finally + }); + } + finally + { + try { - try + if (pushed) { - if (pushed) + lock (reentrantStack) { - lock (reentrantStack) + StrongBox? poppedReleaser = reentrantStack.Pop(); + if (!object.ReferenceEquals(poppedReleaser, pushedReleaser)) { - StrongBox? poppedReleaser = reentrantStack.Pop(); - if (!object.ReferenceEquals(poppedReleaser, pushedReleaser)) - { - // When the semaphore faults, we will drain and throw for awaiting tasks one by one. - this.faulted = true; + // When the semaphore faults, we will drain and throw for awaiting tasks one by one. + this.faulted = true; #pragma warning disable CA2219 // Do not raise exceptions in finally clauses - throw Verify.FailOperation(Strings.SemaphoreStackNestingViolated, ReentrancyMode.Stack); + throw Verify.FailOperation(Strings.SemaphoreStackNestingViolated, ReentrancyMode.Stack); #pragma warning restore CA2219 // Do not raise exceptions in finally clauses - } } } } - finally - { - DisposeReleaserNoThrow(releaser); - } } - }); + finally + { + DisposeReleaserNoThrow(releaser); + } + } } /// @@ -990,10 +990,10 @@ public override async Task ExecuteAsync(Func operation, CancellationToken releaser = default; } - await this.ExecuteCoreAsync(async delegate + bool pushed = false; + try { - bool pushed = false; - try + await this.ExecuteCoreAsync(async delegate { if (this.IsJoinableTaskAware(out JoinableTaskFactory? joinableTaskFactory, out _)) { @@ -1022,20 +1022,20 @@ public override async Task ExecuteAsync(Func operation, CancellationToken } await operation().ConfigureAwaitRunInline(); - } - finally + }); + } + finally + { + if (pushed) { - if (pushed) + lock (reentrantStack) { - lock (reentrantStack) - { - releaser = reentrantStack.Pop(); - } + releaser = reentrantStack.Pop(); } - - DisposeReleaserNoThrow(releaser); } - }); + + DisposeReleaserNoThrow(releaser); + } } /// @@ -1086,10 +1086,10 @@ public override async ValueTask ExecuteAsync(Func> operation, releaser = default; } - return await this.ExecuteCoreAsync(async delegate + bool pushed = false; + try { - bool pushed = false; - try + return await this.ExecuteCoreAsync(async delegate { if (this.IsJoinableTaskAware(out JoinableTaskFactory? joinableTaskFactory, out _)) { @@ -1118,20 +1118,20 @@ public override async ValueTask ExecuteAsync(Func> operation, } return await operation().ConfigureAwait(true); - } - finally + }); + } + finally + { + if (pushed) { - if (pushed) + lock (reentrantStack) { - lock (reentrantStack) - { - releaser = reentrantStack.Pop(); - } + releaser = reentrantStack.Pop(); } - - DisposeReleaserNoThrow(releaser); } - }); + + DisposeReleaserNoThrow(releaser); + } } ///