From f946f7ef0c3d2b82f04b11917f1cbba6e774bc28 Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Wed, 17 May 2017 18:30:44 -0700 Subject: [PATCH 1/4] Add (failing) test that demonstrates NRE race vulnerability in JTF repros #114 and #115 --- .../JoinableTaskTests.cs | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/src/Microsoft.VisualStudio.Threading.Tests.Shared/JoinableTaskTests.cs b/src/Microsoft.VisualStudio.Threading.Tests.Shared/JoinableTaskTests.cs index cedb8352d..d92a655eb 100644 --- a/src/Microsoft.VisualStudio.Threading.Tests.Shared/JoinableTaskTests.cs +++ b/src/Microsoft.VisualStudio.Threading.Tests.Shared/JoinableTaskTests.cs @@ -2653,6 +2653,51 @@ public void PostStress() this.PushFrame(); } + [StaFact] + public void StressFireAndForgetWorkFromCapturedSynchronizationContext() + { + for (int count = 0; count < 5000; count++) + { + var postDelegateInvoked = new ManualResetEventSlim(); + Task innerTask = null; + SynchronizationContext capturedContext = null; + bool posted = false; + + // Do the scheduling off the simulated STA thread so we can conveniently block later. + Task.Run(delegate + { + this.asyncPump.Run(delegate + { + capturedContext = SynchronizationContext.Current; + innerTask = Task.Run(async delegate + { + await Task.Yield(); + + capturedContext.Post( + s => + { + postDelegateInvoked.Set(); + }, + null); + posted = true; + }); + return TplExtensions.CompletedTask; + }); + }).Wait(); + + try + { + innerTask.GetAwaiter().GetResult(); + Assert.True(postDelegateInvoked.Wait(AsyncDelay), "Timed out waiting for posted delegate to execute. Posted: " + posted); + } + catch + { + this.Logger.WriteLine("iteration {0}", count); + throw; + } + } + } + /// /// Verifies that in the scenario when the initializing thread doesn't have a sync context at all (vcupgrade.exe) /// that reasonable behavior still occurs. From 0372ca1526249f2df9769e61381ab3493d3d2628 Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Wed, 17 May 2017 18:30:44 -0700 Subject: [PATCH 2/4] Avoid NRE thrown from JoinableTaskSynchronizationContext.Post This avoids a race condition where the this.job field is cleared in between the check and the dereference of that field. Fixes #114 --- .../JoinableTask+JoinableTaskSynchronizationContext.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.VisualStudio.Threading.Shared/JoinableTask+JoinableTaskSynchronizationContext.cs b/src/Microsoft.VisualStudio.Threading.Shared/JoinableTask+JoinableTaskSynchronizationContext.cs index 2295fef8a..25df9658c 100644 --- a/src/Microsoft.VisualStudio.Threading.Shared/JoinableTask+JoinableTaskSynchronizationContext.cs +++ b/src/Microsoft.VisualStudio.Threading.Shared/JoinableTask+JoinableTaskSynchronizationContext.cs @@ -75,9 +75,10 @@ internal bool MainThreadAffinitized /// public override void Post(SendOrPostCallback d, object state) { - if (this.job != null) + JoinableTask job = this.job; // capture as local in case field becomes null later. + if (job != null) { - this.job.Post(d, state, this.mainThreadAffinitized); + job.Post(d, state, this.mainThreadAffinitized); } else { From 5728fd0bfa01dd5f1ffa6cb15d5099a92b85c5f6 Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Wed, 17 May 2017 18:30:44 -0700 Subject: [PATCH 3/4] Avoid dropping threadpool work items that race with completion of JTF.Run Fixes #115 --- src/Microsoft.VisualStudio.Threading.Shared/JoinableTask.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.VisualStudio.Threading.Shared/JoinableTask.cs b/src/Microsoft.VisualStudio.Threading.Shared/JoinableTask.cs index 6fa8db588..cae996cd1 100644 --- a/src/Microsoft.VisualStudio.Threading.Shared/JoinableTask.cs +++ b/src/Microsoft.VisualStudio.Threading.Shared/JoinableTask.cs @@ -465,7 +465,8 @@ private bool SynchronouslyBlockingThreadPool get { return (this.state & JoinableTaskFlags.StartedSynchronously) == JoinableTaskFlags.StartedSynchronously - && (this.state & JoinableTaskFlags.StartedOnMainThread) == JoinableTaskFlags.None; + && (this.state & JoinableTaskFlags.StartedOnMainThread) == JoinableTaskFlags.None + && (this.state & JoinableTaskFlags.CompleteRequested) != JoinableTaskFlags.CompleteRequested; } } @@ -475,7 +476,8 @@ private bool SynchronouslyBlockingMainThread get { return (this.state & JoinableTaskFlags.StartedSynchronously) == JoinableTaskFlags.StartedSynchronously - && (this.state & JoinableTaskFlags.StartedOnMainThread) == JoinableTaskFlags.StartedOnMainThread; + && (this.state & JoinableTaskFlags.StartedOnMainThread) == JoinableTaskFlags.StartedOnMainThread + && (this.state & JoinableTaskFlags.CompleteRequested) != JoinableTaskFlags.CompleteRequested; } } From 6c6e2b099ab4555190d1fe5f5d8a11106bad8435 Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Thu, 18 May 2017 10:37:47 -0700 Subject: [PATCH 4/4] Applies bitflag check pattern more consistently In response to @bertanaygun CR comments --- src/Microsoft.VisualStudio.Threading.Shared/JoinableTask.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Microsoft.VisualStudio.Threading.Shared/JoinableTask.cs b/src/Microsoft.VisualStudio.Threading.Shared/JoinableTask.cs index cae996cd1..a84826b62 100644 --- a/src/Microsoft.VisualStudio.Threading.Shared/JoinableTask.cs +++ b/src/Microsoft.VisualStudio.Threading.Shared/JoinableTask.cs @@ -465,7 +465,7 @@ private bool SynchronouslyBlockingThreadPool get { return (this.state & JoinableTaskFlags.StartedSynchronously) == JoinableTaskFlags.StartedSynchronously - && (this.state & JoinableTaskFlags.StartedOnMainThread) == JoinableTaskFlags.None + && (this.state & JoinableTaskFlags.StartedOnMainThread) != JoinableTaskFlags.StartedOnMainThread && (this.state & JoinableTaskFlags.CompleteRequested) != JoinableTaskFlags.CompleteRequested; } }