Skip to content

Always bounce through the threadpool when switching from STA #856

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Kenny Kerr (kennykerr) merged 1 commit into from
Feb 11, 2021
Merged

Always bounce through the threadpool when switching from STA #856

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion strings/base_coroutine_threadpool.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
63 changes: 63 additions & 0 deletions test/old_tests/UnitTests/apartment_context.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

@dmachaj David Machaj (dmachaj) Feb 11, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if this were broken (aka before your changes) this co_await would block forever if not for the 1000 timeout on line 85. Is that right?

@oldnewthing Raymond Chen (oldnewthing) Feb 11, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. My initial version had INFINITE, and the test hung. We don't like it when tests hang, so I had it detect that it had been stuck for 1 second (should be plenty) and declare failure if so.


REQUIRE(pass);

// Clean up.
co_await original;

co_await controller1.ShutdownQueueAsync();
co_await controller2.ShutdownQueueAsync();
}

}

TEST_CASE("apartment_context coverage")
Expand All @@ -67,3 +125,8 @@ TEST_CASE("apartment_context nta")
{
TestNeutralApartmentContext().get();
}

TEST_CASE("apartment_context sta")
{
TestStaToStaApartmentContext().get();
}