From 0e39cefbd149bda2a90c404353a0765353360153 Mon Sep 17 00:00:00 2001
From: Stephen Toub <stoub@microsoft.com>
Date: Fri, 3 May 2019 21:01:39 -0400
Subject: [PATCH] Fix deadlock in SystemEventsTests.CreateTimerTests
 (dotnet/corefx#37426)
MIME-Version: 1.0
Content-Type: text/plain; charset=utf8
Content-Transfer-Encoding: 8bit

* Fix deadlock in SystemEventsTests.CreateTimerTests

The main thread in the ConcurrentTimers takes a lock on object timersSignaled and then calls CreateTimer while holding the lock.  CreateTimer tries to SendMessageW a message to be processed by the events window, and as a result won’t return until the window has processed the message.  Meanwhile, that window’s WindowThreadProc is handling a timer callback, which in the event handler in ConcurrentTimers tries to take the timersSignaled lock.  That lock is held by the main thread, that’s calling CreateTimer and doing the SendMessageW that will only wake up when this callback completes.  Deadlock.

The fix is to stop locking while performing such work.

* Address PR feedback


Commit migrated from https://github.com/dotnet/corefx/commit/4b4b791e56cfc5e3afc9fc918e51fb9b975ae0ab
---
 .../tests/SystemEvents.CreateTimer.cs         | 45 +++++++++----------
 1 file changed, 20 insertions(+), 25 deletions(-)

diff --git a/src/libraries/Microsoft.Win32.SystemEvents/tests/SystemEvents.CreateTimer.cs b/src/libraries/Microsoft.Win32.SystemEvents/tests/SystemEvents.CreateTimer.cs
index c244c3be78f..c743d0540fe 100644
--- a/src/libraries/Microsoft.Win32.SystemEvents/tests/SystemEvents.CreateTimer.cs
+++ b/src/libraries/Microsoft.Win32.SystemEvents/tests/SystemEvents.CreateTimer.cs
@@ -3,11 +3,13 @@
 // See the LICENSE file in the project root for more information.
 
 using System;
+using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Linq;
 using System.Runtime.InteropServices;
 using System.Threading;
+using System.Threading.Tasks;
 using Xunit;
 
 namespace Microsoft.Win32.SystemEventsTests
@@ -78,24 +80,19 @@ namespace Microsoft.Win32.SystemEventsTests
         public void ConcurrentTimers()
         {
             const int NumConcurrentTimers = 10;
-            var timersSignalled = new Dictionary<IntPtr, bool>();
+            var timersSignaled = new ConcurrentDictionary<IntPtr, bool>();
             int numSignaled = 0;
-            var elapsed = new AutoResetEvent(false);
+            var elapsed = new ManualResetEventSlim();
 
             TimerElapsedEventHandler handler = (sender, args) =>
             {
-                bool signaled = false;
-                lock (timersSignalled)
+                // A timer might fire more than once.  When it fires the first time, track it by adding
+                // it to a set and then increment the number of timers that have ever fired.  When all
+                // timers have fired, set the event.
+                if (timersSignaled.TryAdd(args.TimerId, true) &&
+                    Interlocked.Increment(ref numSignaled) == NumConcurrentTimers)
                 {
-                    if (timersSignalled.TryGetValue(args.TimerId, out signaled) && !signaled)
-                    {
-                        timersSignalled[args.TimerId] = true;
-
-                        if (Interlocked.Increment(ref numSignaled) == NumConcurrentTimers)
-                        {
-                            elapsed.Set();
-                        }
-                    }
+                    elapsed.Set();
                 }
             };
 
@@ -104,27 +101,25 @@ namespace Microsoft.Win32.SystemEventsTests
             {
                 if (PlatformDetection.IsFullFramework)
                 {
-                    // desktop has a bug where it will allow EnsureSystemEvents to proceed without actually creating the HWND
+                    // netfx has a bug where it will allow EnsureSystemEvents to proceed without actually creating the HWND
                     SystemEventsTest.WaitForSystemEventsWindow();
                 }
 
+                // Create all the timers
+                var timers = new List<IntPtr>();
                 for (int i = 0; i < NumConcurrentTimers; i++)
                 {
-                    lock (timersSignalled)
-                    {
-                        timersSignalled[SystemEvents.CreateTimer(TimerInterval)] = false;
-                    }
+                    timers.Add(SystemEvents.CreateTimer(TimerInterval));
                 }
 
-                Assert.True(elapsed.WaitOne(TimerInterval * SystemEventsTest.ExpectedEventMultiplier));
+                // Wait for them all to fire
+                Assert.True(elapsed.Wait(TimerInterval * SystemEventsTest.ExpectedEventMultiplier));
 
-                lock (timersSignalled)
+                // Delete them all
+                foreach (IntPtr timer in timers)
                 {
-                    foreach (var timer in timersSignalled.Keys.ToArray())
-                    {
-                        Assert.True(timersSignalled[timer]);
-                        SystemEvents.KillTimer(timer);
-                    }
+                    Assert.True(timersSignaled.TryGetValue(timer, out _));
+                    SystemEvents.KillTimer(timer);
                 }
             }
             finally
-- 
2.34.1