Skip to content
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.

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

Improvements to the instancing redirection API #1709

Open
andreww-msft opened this issue Nov 2, 2021 · 9 comments
Open

Improvements to the instancing redirection API #1709

andreww-msft opened this issue Nov 2, 2021 · 9 comments

Comments

@andreww-msft
Copy link
Contributor

andreww-msft commented Nov 2, 2021

RedirectActivationTo

Overview

The API spec [here](https://github.com/microsoft/WindowsAppSDK/blob/main/specs/AppLifecycle/Instancing/AppLifecycle SingleMulti-Instancing.md) describes how the Windows App SDK brings the single-/
multi-instancing API support from UWP to non-UWP apps. One of the key
methods is RedirectActivationToAsync. This document describes some problems
with the current API design, and proposals for mitigation.

The purpose of the API is that when an app instance starts, it can discover other
running instances of itself and conditionally redirect its own activation to one of
those other instances. This enables an app to be single-instanced or selectively
multi-instanced, based on the app’s own logic.

This is very similar to the UWP RedirectActivationTo method, with 2 differences:

  • The UWP method takes no parameters, and the target instance receives the
    exact same activation arguments that the redirecting instance received.
    Conversely, the Windows App SDK version allows the app to choose whether
    to pass the arguments it received or to modify or replace them.

  • In the UWP implementation, redirection is a terminal operation: after the
    RedirectActivationTo call, the platform terminates the calling instance. In
    the Windows App SDK, redirection is not necessarily a terminal operation.
    In most cases, we expect the redirecting instance to simply return from
    Main after redirecting, effectively terminating itself – but the app has the
    option to continue if it wishes.

Problems

There are 2 main problems with the Windows App SDK API in its current form:

  1. Terminating target. There’s a race where an instance of the app attempts to
    redirect to another instance, but that other instance happens to be in the
    process of terminating. This causes the calling instance to crash.

  2. STA blocking. In many app types, calling the redirection method requires
    additional threading/synchronization work.

    1. We expect the redirect call to happen very early – and before doing any
      initialization, window creation, etc, that would in most cases be redundant
      if the app is redirecting. Apps are therefore encouraged to do this in Main.

    2. The redirection method is async, but we need the caller to wait for it to
      complete before terminating, otherwise the redirection doesn’t succeed.

    3. Some app types specify that Main must run in an STA. If we wait on the
      async call, we’ll block the STA, which for some app types causes a runtime
      assert, and for other types has unpredictable consequences.

Proposed solutions

For the 2 problems, we propose 2 solutions: that is, to add 2 new methods to
the AppInstance class.

Terminating target

This is the simpler of the 2 problems to solve. The proposal is for the internal
implementation of the redirect operation to catch the case where the target
instance is terminating, and instead of propagating the error to the caller,
convert this to a bool return value (where false indicates that the redirection
failed). This requires either updating the method signature, or adding a new
method. Updating the existing method would constitute a breaking change, so
the preferred option is to add a second method.

The existing method is defined as:

	void RedirectActivationToAsync();

…the additional method could be defined like this:

	Task<bool> RedirectActivationToWithResultAsync();

STA blocking

It is possible for the app to work around the STA-blocking problem, and the exact
mitigations depend on the app type:

  1. Declare Main to be async, and use the await pattern. This is only available in
    C# apps, and only works correctly in WinForms apps. It is not
    an option for C++ apps, and it does not work for C# WPF apps,
    nor for C# WinUI apps.

  2. Explicitly write the code that async/await are doing: that is, move the redirect
    call to another thread, use an event to signal when it is complete, and use a
    non-blocking method to wait on the signal (eg, CoWaitForMultipleObjects or
    MsgWaitForMultipleObjects). This works for all app types.

Here are all the app types under consideration for the Windows App SDK (note:
packaged vs unpackaged makes no difference in this context):

App type Language Apartment Mitigation
Win32 C++ MTA n/a
Console C++ MTA n/a
Console C# MTA n/a
Windows Forms C# STA Declare Main to be async
WPF C# STA Move the call to another thread, and synchronize
WinUI C# STA Move the call to another thread, and synchronize
WinUI C++ STA Move the call to another thread, and synchronize

Here’s an example of the code you can use in Windows Forms:

// Declare Main to be async. 
[STAThread]
static async Task<int> Main(string[] args)
{
  AppActivationArguments activationArgs = 
     AppInstance.GetCurrent().GetActivatedEventArgs();
  AppInstance keyInstance = AppInstance.FindOrRegisterForKey("Foo");
  if (!keyInstance.IsCurrent)
  {
     // Use await on the redirect call. 
     await keyInstance.RedirectActivationToAsync(activationArgs);
  }
  //...etc
}

Here’s an example of the kind of code you’d use in C++ WinUI:

// Declare an event.
wil::unique_event redirectEventHandle;

// Move the redirect call to another thread, and set the
// event when the redirect operation is complete.
winrt::fire_and_forget Redirect(
  AppInstance keyInstance, AppActivationArguments args)
{
  wil::event_set_scope_exit ensure_signaled =
  wil::SetEvent_scope_exit(redirectEventHandle.get());
  co_await keyInstance.RedirectActivationToAsync(args);
}

int __stdcall wWinMain(HINSTANCE, HINSTANCE, PWSTR, int)
{
  winrt::init_apartment(winrt::apartment_type::single_threaded);

  AppActivationArguments args = 
  AppInstance::GetCurrent().GetActivatedEventArgs();
  AppInstance keyInstance = AppInstance::FindOrRegisterForKey("Foo");
  if (!keyInstance.IsCurrent())
  {
    // Create the event.
    redirectEventHandle.create();
    
    // Call the coroutine to execute the redirect operation.
    Redirect(keyInstance, args);
    
    // Use a non-blocking function to wait for the event to be set.
    DWORD handleIndex = 0;
    HANDLE rawHandle = redirectEventHandle.get();
    CoWaitForMultipleObjects(
      CWMO_DEFAULT, INFINITE, 1, &rawHandle, &handleIndex);
  }
//...etc
}

The C# equivalent for C#/WinUI and WPF apps would look like this (p/invoke declarations omitted):

[STAThread]
public static int Main(string[] args)
{
  AppActivationArguments activationArgs = 
    AppInstance.GetCurrent().GetActivatedEventArgs();
  AppInstance keyInstance = AppInstance.FindOrRegisterForKey("Foo");
  if (!keyInstance.IsCurrent)
  {
  	// Move the redirect call to another thread, set an event when 
  	// it’s complete, and use a non-blocking function to wait on the 
  	// event. 
    redirectEventHandle = CreateEvent(
      IntPtr.Zero, true, false, null);
    if (redirectEventHandle != IntPtr.Zero)
    {
      Task.Run(() =>
      {
        keyInstance.RedirectActivationToAsync(activationArgs)
          .AsTask().Wait();
        SetEvent(redirectEventHandle);
      });

      uint CWMO_DEFAULT = 0;
      uint INFINITE = 0xFFFFFFFF;
      _ = CoWaitForMultipleObjects(
        CWMO_DEFAULT, INFINITE, 1,
        new IntPtr[] { redirectEventHandle }, out uint handleIndex);
    }
  }
  //...etc
}

Asking app developers to write additional threading/synchronization code when
they want to use the redirection API suggests that the redirection API needs
improvement:

  • The required threading/synchronization code is painful and error-prone.

  • In the case of C++/WinRT it also requires a thorough understanding of
    C++/WinRT coroutines and special event types.

  • For C++/WinRT, the recommendation is to use the Windows.Implementation.Library
    (WIL), so you’d also need to include the WIL NuGet package which the app
    might not otherwise need.

  • The developer needs to understand whether their code is running in an STA
    – and this is not always obvious.

  • WPF has the additional issue that you can declare main to be async, but this
    actually causes the [STAThread] attribute to be ignored, and WPF window
    creation immediately fails.

  • C#/WinUI has the additional issue that you can declare main to be async, but
    this actually prevents Narrator from reading the XAML elements, so this is an
    accessibility concern.

  • If you want to use the same threading/synchronization mechanisms for a WPF
    or C#/WinUI app as shown above, you’d need to p/invoke the relevant native
    APIs because .NET does not include all the APIs you’d need.

  • Different app types have different mitigations available to them, which will
    only add to developer confusion.

All this makes for a very poor developer experience. We therefore propose to
update the API as follows. From an app developer’s perspective, it would be much
more convenient to be able to do something like this:

[STAThread]
public static int Main(string[] args)
{
  AppActivationArguments activationArgs = 
    AppInstance.GetCurrent().GetActivatedEventArgs();
  AppInstance keyInstance = AppInstance.FindOrRegisterForKey("Foo");
  if (!keyInstance.IsCurrent)
  {
    // Call a non-async method to do the redirection, which also 
    // doesn’t block the app’s message queue.
    keyInstance.RedirectActivationTo(activationArgs);
  }
  //...etc
}

To enable this, we propose to add a non-async RedirectActivationTo method to
the AppInstance class, that would do the functional equivalent of this:

public class AppInstance
{
    // Existing implementation omitted...

    private static IntPtr redirectEventHandle = IntPtr.Zero;

    // Sample code to illustrate the required behavior, not final code.
    public void RedirectActivationTo(AppActivationArguments args)
    {
       redirectEventHandle = CreateEvent(IntPtr.Zero, true, false, null);
       Task.Run(() =>
       {
          RedirectActivationToAsync(args).AsTask().Wait(); 
          SetEvent(redirectEventHandle);
       });
       uint CWMO_DEFAULT = 0;
       uint INFINITE = 0xFFFFFFFF;
       _ = CoWaitForMultipleObjects(
          CWMO_DEFAULT, INFINITE, 1,
          new IntPtr[] { redirectEventHandle }, out uint handleIndex);
    }
}

Documentation would then encourage developers to use RedirectActivationTo
instead of the existing RedirectActivationToAsync or the proposed new
RedirectActivationToWithResultAsync for all app types whenever they need to
do the redirection logic in Main (which we believe is the most common case).

@andreww-msft
Copy link
Contributor Author

Tracking internally 36907792.

@akash07k
Copy link

Seems promising. Any estimates to bring it up soon?

@andreww-msft
Copy link
Contributor Author

We don't have a firm date for this yet. That said, we are interested in any feedback, and to gauge the level of demand for this.

@dhoehna
Copy link
Contributor

dhoehna commented Sep 15, 2022

It has been a while. I have a branch out with RedirectActivationTo and a modified Spec. See here

The spec has been updated with RedirectActivationTo. I would like this reviewed please.

@dhoehna
Copy link
Contributor

dhoehna commented Sep 15, 2022

I've decided not to work on the first issue, about redirecting to a terminating instance because I can't repro that. I've tried

  1. Making 2 actions. One that terminates Instance A and one that redirects to instance A.
    I. Ran the terminating action first, then fired off the redirection action. No wait between starting both actions.
    II. Reversing the calls.
  2. Make the redirection call an action and run the task after terminating instance A.

In all three cases no crash happened. I do believe safe-guards were put in place to protect against this.

However, if a repro can be made, please let me know I'll get working on a fix.

@dhoehna
Copy link
Contributor

dhoehna commented Sep 15, 2022

@jonwis and @DrusTheAxe

@dhoehna
Copy link
Contributor

dhoehna commented Sep 16, 2022

I was able to repro this with a super-simple C# console app that declared main to be STA.

@nicop85
Copy link

nicop85 commented May 18, 2023

Are there any plans for the near future to improve this?

I just came across this problem while working with push notifications and their handlers when the user clicks them.

@aeloros
Copy link
Contributor

aeloros commented Jun 8, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants