Skip to content

Conversation

@oldnewthing
Copy link
Member

@oldnewthing oldnewthing commented Jun 13, 2020

The neutral apartment is not a thread, but rather steals whatever thread you happen to be using by temporarily converting it to a neutral apartment, and then restoring the original apartment when it's done.

COM will secretly create neutral apartments for issuing callbacks, and code may find itself inadvertently capturing a neutral apartment when it thought it was capturing an MTA:

fire_and_forget OnSomething()
{
    auto previous = apartment_context();
    co_await resume_foreground(dispatcher);
    .do_ui_stuff();
    co_await previous;
    do_slow_stuff();
}

If this code is called from a neutral apartment on a borrowed MTA thread, the final co_await previous will switch to the neutral apartment while staying on the UI thread, even though the code started on an MTA thread. This is surprising behavior that has been identified as the root cause of some hangs.

This change revises C++/WinRT to minimize the surprise by resuming neutral apartments on an MTA thread if coming from an STA thread.

Previously, the apartment_context worried about only two causes (STA and MTA), and it encoded the MTA as a null pointer. Now, we have three cases to worry about, so the trick of encoding the MTA as nullptr is not going to work. The apartment_context now needs to capture both the IContextCallback as well as the apartment type of that context.

Explicitly carrying the apartment type removes the need for the special nullptr sentinel value, so we get rid of it.

Switching to a neutral apartment from an STA now switches explicitly to an MTA thread before converting the MTA thread to a neutral apartment. In other words, what used to be

co_await resume_on_nta;

is now, conceptually,

co_await resume_background();
co_await resume_on_nta;

Implementing this "extra co_await" is not as pretty as it sounds because we cannot use the coroutine machinery to do the extra co_await. We have to code it up explicitly.

New internal function get_apartment_type() returns a std::pair of the apartment type and apartment type qualifier. If COM is not initialized, then we pretend that we are in the implicit MTA.

Internal function impl::is_sta() is renamed to impl::is_sta_thread() since it now checks the apartment of the underlying thread, even if the current apartment is neutral.

Removed dependency of unit test on now-defunct internal function impl::is_sta().

This PR incorporates #663 because I need try_capture.

The neutral apartment is not a thread, but rather
steals whatever thread you happen to be using
by temporarily converting it to a neutral apartment,
and then restoring the original apartment when it's done.

COM will secretly create neutral apartments for issuing
callbacks, and code may find itself inadvertently capturing
a neutral apartment when it thought it was capturing an MTA:

```cpp
fire_and_forget OnSomething()
{
    auto previous = apartment_context();
    co_await resume_foreground(dispatcher);
    ... do UI stuff ...
    co_await previous;
    ... do slow stuff ...
}

```

If this code is called from a neutral apartment
on a borrowed MTA thread, the `co_await previous`
will switch to the neutral apartment while staying
on the UI thread, even though the code started on an
MTA thread. This is surprising behavior that has been
identified as the root cause of some hangs.

This change revises C++/WinRT to minimize the surprise
by always resuming neutral apartments on an MTA thread.

Previously, the `apartment_context` worried about only
two causes (STA and MTA), and it encoded the MTA as
a null pointer. Now, we have three cases to worry about,
so the trick of encoding the MTA as `nullptr` is not
going to work. The `apartment_context` now needs to
capture both the `IContextCallback` as well as the
apartment type of that context.

Explicitly carrying the apartment type removes the
need for the special `nullptr` sentinel value, so we
get rid of it.

Switching to a neutral apartment from an STA now
switches explicitly to an MTA thread before converting
the MTA thread to a neutral apartment. In other wirds,
what used to be

```cpp
co_await resume_on_nta;
```

is now, conceptually,

```cpp
co_await resume_background();
co_await resume_on_nta;
```

Implementing this "extra `co_await`" is not as pretty
as it sounds because we cannot use the coroutine machinery
to do the extra `co_await`. We have to code it up
explicitly.

New internal function `get_apartment_type()` returns a
`std::pair` of the apartment type and apartment type
qualifier. If COM is not initialized, then we pretend that
we are in the implicit MTA.

Internal function `impl::is_sta()` is renamed to
`impl::is_sta_thread()` since it now checks the underlying
thread, in addition to than the current apartment: It returns
true for an STA apartment, or for a neutral apartment
running on a borrowed STA thread.

Removed dependency of unit test on now-defunct internal function
`impl::is_sta()`.
Also, switch to an if/else chain in `resume_apartment`, since
it actually reads pretty cleanly.

* If COM is not initialized (`m_context` is null)
  or we are in the correct context already, then call the
  continuation directly.
* If the destination is the MTA, then use a threadpool thread.
* If the destination is the NA, and we are on an STA thread,
  then resume on a threadpool thread.
* Otherwise resume synchronously.
@oldnewthing
Copy link
Member Author

The revised resume_apartment avoids going through IContextCallback in the common case where the resume_apartment is used to get from the completion context to a matching awaiter context (e.g., coroutine completed on STA, and awaiter wants to resume on STA). This conserves some stack space.

@ChrisGuzak
Copy link
Member

ChrisGuzak commented Jun 13, 2020

Switching to a neutral apartment from an STA now switches explicitly to an MTA thread before converting the MTA thread to a neutral apartment.

This is the crux, avoiding the problem of converting an STA to an NA. Emphasize this point.

Copy link
Member

@ChrisGuzak ChrisGuzak left a comment

Choose a reason for hiding this comment

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

great work... thanks for doing this.

}

inline bool is_sta() noexcept
inline std::pair<int32_t, int32_t> get_apartment_type() noexcept
Copy link
Member

Choose a reason for hiding this comment

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

consider mentioning the results hold APTTYPE_* and APTTYPEQUALIFIER_* values

Copy link
Member Author

Choose a reason for hiding this comment

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

Already mentioned in the PR description.

@kennykerr
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@kennykerr kennykerr merged commit a0b1889 into microsoft:master Jun 16, 2020
@oldnewthing oldnewthing deleted the nta-on-threadpool branch July 8, 2020 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants