Fix issue with potential deadlock when reading a managed stream that we wrap from native code

[manodasanW]

CsWinRT has some helper methods which allows to wrap a C# managed stream and pass it as an WinRT stream interface like IRandomStream. This is implemented by having a wrapper class in CsWinRT which satisfies the methods of the WinRT stream interfaces and calls the appropriate methods on the managed stream to implement them.

As part of this, the read / write methods return IAsyncOperations and when a synchronization context exists on the thread on which the async operation is created, we will queue the completion handlers onto the same context as part of the async operation infrastructure. But if the caller of the read / write is blocking on the same thread for them to get triggered and isn't pumping STA messages, we can end up in a deadlock as seen in the mentioned issue. This is specifically true when CreateStreamOverRandomAccessStream is used by apps to convert it to an IStream and is done on the UI thread or another thread with a synchronization context.

Apps / libraries can avoid this deadlock by doing it on a background / worker thread without a synchronization context. But given this will always deadlock in this scenario when used with CreateStreamOverRandomAccessStream, I am putting a targeted change in CsWinRT to accommodate for it. The change will make async operations created by our managed stream wrappers not need to run the completion handlers on the same context and thereby avoiding the deadlock. Given that the callers of this async operation should mostly be native (managed callers will probably use the stream directly) and thereby its handlers and given that SynchronizationContext look to be a managed concept, I believe this should be fine, but we will monitor to see if there is any issues that arise from this.

Fixes #670

[jlaanstra]

Doesn't this break the IAsyncOperation/IAsyncAction contract where the callbacks happen in the same apartment? Blocking on an async operation in an STA is a well known problem that generally can be easily prevented by not blocking on the STA.

/cc @kennykerr @oldnewthing for the IAsyncOperation/IAsyncAction contract. As far as I know these objects always marshal back to the calling apartment but I'm not immediately seeing in the docs that this is required. Closest I found so far is https://docs.microsoft.com/en-us/windows/uwp/cpp-and-winrt-apis/concurrency-2#execution-contexts-resuming-and-switching-in-a-coroutine

[manodasanW]

Doesn't this break the IAsyncOperation/IAsyncAction contract where the callbacks happen in the same apartment? Blocking on an async operation in an STA is a well known problem that generally can be easily prevented by not blocking on the STA.

/cc @kennykerr @oldnewthing for the IAsyncOperation/IAsyncAction contract. As far as I know these objects always marshal back to the calling apartment but I'm not immediately seeing in the docs that this is required. Closest I found so far is https://docs.microsoft.com/en-us/windows/uwp/cpp-and-winrt-apis/concurrency-2#execution-contexts-resuming-and-switching-in-a-coroutine

I have been looking more into this and it looks like C++/WinRT achieves something similar using IContextCallback. But I noticed they do pass ICallbackWithNoReentrancyToApplicationSTA and have a fallback to call without context on error. Is that what C++/WinRT uses to avoid deadlocks such as in this scenario where the completion handler is trying to be triggered on the STA thread which is where the async operation is being waited on? @kennykerr?

If the above is correct, I wonder if we should instead be using context callback to do the call on the same thread for ASTA or if we should continue using SynchronizationContext which is what is being used right now to light up something similar.

[jlaanstra]

cppwinrt explicitly forbids blocking waits on the STA: https://github.com/microsoft/cppwinrt/blob/8484e43b816d695a9b1e28582eda87696e6111d5/strings/base_coroutine_foundation.h#L34

[jlaanstra]

I also think the current change can result in some interesting behavior when the IAsyncAction/Operation ends back in C# land and a C# caller calls AsTask() on this.

[kennykerr]

The async operation is not required to complete on any particular thread or apartment. It is the responsibility of the waiter to synchronize/reschedule as desired. C++/WinRT captures the apartment context prior to suspension and resumes on that same context.

[jlaanstra]

The async operation is not required to complete on any particular thread or apartment. It is the responsibility of the waiter to synchronize/reschedule as desired. C++/WinRT captures the apartment context prior to suspension and resumes on that same context.

Sounds like we might be able to remove the context capture completely then.

[manodasanW]

Thanks @kennykerr and @jlaanstra. Have a new PR (#1131) which addresses this.