Create test hook mechanism for XTaskQueue (#948)

* Create retail test hook mechanism

* Fix for xcode

Author: brianpepin

Source/Task/TaskQueue.cpp

@@ -3,13 +3,9 @@
 
 #include "pch.h"
 #include "referenced_ptr.h"
+#include "XTaskQueuePriv.h"
 #include "TaskQueueP.h"
 #include "TaskQueueImpl.h"
-#include "XTaskQueuePriv.h"
-
-#ifdef HC_UNITTEST_API
-TestBarrier* g_testBarrier = nullptr;
-#endif
 
 //
 // ApiRefs tracks global refcounts for all APIs. It is used to identify memory leaks
@@ -1111,32 +1107,27 @@ bool TaskQueuePortImpl::ScheduleNextPendingCallback(
         uint64_t noDueTime = UINT64_MAX;
         if (m_timerDue.compare_exchange_strong(dueTime, noDueTime))
         {
-#ifdef HC_UNITTEST_API
-            // Test hook: two-phase barrier reproduces the timer race
-            // that results in lost delayed task wakes.
-            if (g_testBarrier != nullptr)
-            {
-                {
-                    std::lock_guard<std::mutex> lk(g_testBarrier->mtx);
-                    g_testBarrier->phase1_ready = true;
-                }
-                g_testBarrier->cv.notify_all();
-
-                std::unique_lock<std::mutex> lk(g_testBarrier->mtx);
-                g_testBarrier->cv.wait_for(lk, std::chrono::seconds(5),
-                    [&] { return g_testBarrier->phase2_ready; });
-            }
-#endif
             // Bug fix: ScheduleNextPendingCallback timer race results
-            // in lost delayed task wakes.
-            //
+            // in lost delayed task wakes. Don't cancel the timer here
+            // as another scheduled callback could have been added.
             // The CAS above is sufficient: the timer has already fired
             // (call site 1: SubmitPendingCallback) or was already
             // canceled (call site 2: CancelPendingEntries).  A Cancel()
             // here raced with concurrent QueueItem/Start calls on other
             // threads, permanently stranding entries in m_pendingList.
             // See VerifyDelayedCallbackTimerRaceOnManualQueue for full
-            // analysis.
+            // analysis. The test hook here allows unit tests to verify
+            // there is no race.
+            m_attachedContexts.Visit([&](ITaskQueuePortContext* portContext)
+            {
+                auto hooks = portContext->GetQueue()->GetTestHooks();
+                if (hooks != nullptr)                {
+                    hooks->NextPendingCallbackScheduled(
+                        portContext->GetType(),
+                        dueTime,
+                        noDueTime);
+                }
+            });
         }
     }
 
@@ -1277,14 +1268,14 @@ void TaskQueuePortImpl::SignalTerminations()
         entry->portContext->AddRef();
 
         entry->callback(entry->callbackContext);
-        
-        // Release portContext after callback completes
-        entry->portContext->Release();
-        
+                
         {
             std::lock_guard<std::mutex> lock(m_terminationLock);
             m_terminationList->free_node(address);
         }
+
+        // Release portContext after callback completes
+        entry->portContext->Release();
         delete entry;
     }
 }
@@ -2349,3 +2340,17 @@ STDAPI_(bool) XTaskQueueUninitialize(
 
     return ApiRefs::WaitZeroRefs(timeoutMilliseconds);
 }
+
+/// <summary>
+/// Sets or clears test hooks on a task queue.
+/// </summary>
+STDAPI XTaskQueueSetTestHooks(
+    _In_ XTaskQueueHandle queue,
+    _In_ XTaskQueueTestHooks* hooks
+    ) noexcept
+{
+    referenced_ptr<ITaskQueue> aq(GetQueue(queue));
+    RETURN_HR_IF(E_GAMERUNTIME_INVALID_HANDLE, aq == nullptr);
+    aq->SetTestHooks(hooks);
+    return S_OK;
+}

Source/Task/TaskQueueImpl.h

@@ -402,6 +402,8 @@ class TaskQueueImpl : public Api<ApiId::TaskQueue, ITaskQueue>
         _In_ XTaskQueuePortHandle completionPort);
 
     XTaskQueueHandle __stdcall GetHandle() override { return &m_header; }
+    XTaskQueueTestHooks* __stdcall GetTestHooks() override { return m_testHooks; }
+    void __stdcall SetTestHooks(_In_ XTaskQueueTestHooks* testHooks) override { m_testHooks = testHooks; }
 
     HRESULT __stdcall GetPortContext(
         _In_ XTaskQueuePort port,
@@ -473,6 +475,7 @@ class TaskQueueImpl : public Api<ApiId::TaskQueue, ITaskQueue>
     TerminationData m_termination;
     TaskQueuePortContextImpl m_work;
     TaskQueuePortContextImpl m_completion;
+    XTaskQueueTestHooks* m_testHooks = nullptr;
 
 #ifdef SUSPEND_API
     SuspendResumeHandler m_suspendHandler;

Source/Task/TaskQueueP.h

@@ -125,6 +125,8 @@ struct ITaskQueuePortContext : IApi
 struct ITaskQueue : IApi
 {
     virtual XTaskQueueHandle __stdcall GetHandle() = 0;
+    virtual XTaskQueueTestHooks* __stdcall GetTestHooks() = 0;
+    virtual void __stdcall SetTestHooks(_In_ XTaskQueueTestHooks* testHooks) = 0;
 
     virtual HRESULT __stdcall GetPortContext(
         _In_ XTaskQueuePort port,

Source/Task/XTaskQueuePriv.h

@@ -5,11 +5,6 @@
 
 #include "XTaskQueue.h"
 
-#ifdef HC_UNITTEST_API
-#include <mutex>
-#include <condition_variable>
-#endif
-
 //----------------------------------------------------------------//
 //
 // These APIs should be reserved for driving unit test harnesses.
@@ -45,6 +40,34 @@ STDAPI_(void) XTaskQueueResumeTermination(
     _In_ XTaskQueueHandle queue
     ) noexcept;
 
+/// <summary>
+/// This structure can be passed as a pointer to the task queue so unit tests
+/// can hook into its behavior. Some race conditions are very difficult to get
+/// to happen naturally so sometimes a hook is needed. A pointer to this
+/// structure will be stored on the task queue. It is up to the test to ensure
+/// the structure lifetime exceeds that of the task queue under test. 
+/// </summary>
+struct XTaskQueueTestHooks
+{
+    virtual void NextPendingCallbackScheduled(
+        XTaskQueuePort port,
+        uint64_t lastDueTime,
+        uint64_t nextDueTime)
+    {
+        UNREFERENCED_PARAMETER(port);
+        UNREFERENCED_PARAMETER(lastDueTime);
+        UNREFERENCED_PARAMETER(nextDueTime);
+    }
+};
+
+/// <summary>
+/// Sets or clears test hooks on a task queue.
+/// </summary>
+STDAPI XTaskQueueSetTestHooks(
+    _In_ XTaskQueueHandle queue,
+    _In_ XTaskQueueTestHooks* hooks
+    ) noexcept;
+
 //----------------------------------------------------------------//
 //
 // These APIs are internal to the runtime
@@ -125,20 +148,3 @@ STDAPI_(bool) XTaskQueueGetCurrentProcessTaskQueueWithOptions(
 STDAPI_(bool) XTaskQueueUninitialize(
     _In_ uint32_t timeoutMilliseconds = 0
     ) noexcept;
-
-#ifdef HC_UNITTEST_API
-
-// Two-phase barrier for deterministic reproduction of the
-// ScheduleNextPendingCallback timer race that results in lost delayed
-// task wakes. See VerifyDelayedCallbackTimerRaceOnManualQueue.
-struct TestBarrier
-{
-    std::mutex mtx;
-    std::condition_variable cv;
-    bool phase1_ready = false;  // threadpool -> test: "CAS done"
-    bool phase2_ready = false;  // test -> threadpool: "Start done"
-};
-
-extern TestBarrier* g_testBarrier;
-
-#endif