From 399dac572be0d44ec1002182944bc84512018b61 Mon Sep 17 00:00:00 2001 From: Raymond Chen Date: Wed, 10 Feb 2021 13:50:41 -0800 Subject: [PATCH] Always bounce through the threadpool when switching from STA Previously, we changed the way we switch from STA to NA by bouncing through the threadpool so that we don't run the NA on an STA thread. We extend this behavior to switches from STA to STA, to avoid RO_E_BLOCKED_CROSS_ASTA_CALL exceptions. Interestingly, we add this support by deleting code. We would normally add tests for STA and MAINSTA to the NA test, but since MTA was ruled out by the previous test, and NA+STA+MAINSTA completely cover all the non-MTA possibilities, there's no need to perform any test at all. The fact that we got here means that the destination is NA/STA/MAINSTA. Note also that the first test in this block checks if we are resuming in the same apartment that we are currently in. That test is important, because it avoids us bouncing through the threadpool just to return to our own thread, which would otherwise burn a threadpool thread. Added a new unit test to verify that STA-to-STA transfers release the source STA. The test failed before the change, and passes after. Note however that this introduces a possibly undesirable behavior change: Suppose STA thread "A" hangs and STA thread "B" has many tasks, some of which want to resume on STA thread "A". Under the old behavior, STA thread "B" would run the first task, and then that task will try to switch to STA thread "A" and hang. STA thread "B" would remain hung and be unavailable to process tasks. When STA thread "A" finally wakes up, STA thread "B" also wakes up and resumes doing work. The number of hung threads is capped at the number of STA threads. Under the new behavior, STA thread "B" would run the first task, and then ask a threadpool to resume on STA thread "A". The threadpool thread hangs instead of STA thread "B". STA thread "B" is now available to do other work (yay), but at a cost of a hung threadpool thread. One of the things STA thread "B" might do is create more work items that want to switch to STA thread "A", so we slowly (or maybe quickly) exhaust the threadpool. Mitigation for this worst-case scenario is that we sort of had that problem anyway: If any task on a threadpool thread wants to switch to STA thread "A", it will hang with or without these changes. Since it is common to pass through a threadpool thread at some point, you are likely to exhaust the threadpool even under the old algorithm. --- strings/base_coroutine_threadpool.h | 2 +- .../old_tests/UnitTests/apartment_context.cpp | 63 +++++++++++++++++++ 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/strings/base_coroutine_threadpool.h b/strings/base_coroutine_threadpool.h index 3a646a55f..6ad83fcae 100644 --- a/strings/base_coroutine_threadpool.h +++ b/strings/base_coroutine_threadpool.h @@ -96,7 +96,7 @@ namespace winrt::impl { resume_background(handle); } - else if ((context.m_context_type == 2 /* APTTYPE_NTA */) && is_sta_thread()) + else if (is_sta_thread()) { resume_apartment_on_threadpool(context.m_context, handle); } diff --git a/test/old_tests/UnitTests/apartment_context.cpp b/test/old_tests/UnitTests/apartment_context.cpp index ae3511698..7aa2f2efc 100644 --- a/test/old_tests/UnitTests/apartment_context.cpp +++ b/test/old_tests/UnitTests/apartment_context.cpp @@ -56,6 +56,64 @@ namespace REQUIRE(is_nta_on_mta()); } + + IAsyncAction TestStaToStaApartmentContext() + { + bool pass = false; + + apartment_context original; + + // Create an STA thread and switch to it. + auto controller1 = DispatcherQueueController::CreateOnDedicatedThread(); + co_await resume_foreground(controller1.DispatcherQueue()); + + // Save the COM context for the first STA thread. + apartment_context context1; + + // Create another STA thread and switch to it. + auto controller2 = DispatcherQueueController::CreateOnDedicatedThread(); + co_await resume_foreground(controller2.DispatcherQueue()); + + // The first STA thread remains hung as long as this is true. + bool hang_sta_1 = true; + + // Hang the first STA thread. + controller1.DispatcherQueue().TryEnqueue([&] { + bool hang = true; + while (hang_sta_1) + { + if (!WaitOnAddress(&hang_sta_1, &hang, sizeof(hang), 1000)) + { + return; // failed - timed out + } + } + pass = true; + }); + + // Queue work to the second STA thread to unstick the first thread. + // This requires that the second STA thread be available to dispatch work + // after the "co_await context1" below. + controller2.DispatcherQueue().TryEnqueue([&] { + hang_sta_1 = false; + WakeByAddressAll(&hang_sta_1); + }); + + // Try to switch to the first STA thread via COM context. + // The first STA thread is hung, but once this thread (the second STA thread) + // processes work, it will unstick the first STA thread. + // This test verifies that the second STA thread does become available + // to do work and is not hung waiting for the first STA thread. + co_await context1; + + REQUIRE(pass); + + // Clean up. + co_await original; + + co_await controller1.ShutdownQueueAsync(); + co_await controller2.ShutdownQueueAsync(); + } + } TEST_CASE("apartment_context coverage") @@ -67,3 +125,8 @@ TEST_CASE("apartment_context nta") { TestNeutralApartmentContext().get(); } + +TEST_CASE("apartment_context sta") +{ + TestStaToStaApartmentContext().get(); +}