dotnet / wpf

WPF is a .NET Core UI framework for building Windows desktop applications.
MIT License
7.08k stars 1.17k forks source link

Add Dispatcher.Created static event #7301

Open matthew-a-thomas opened 1 year ago

matthew-a-thomas commented 1 year ago

The need

It's undesirable to instantiate a Dispatcher on a non-UI thread. Doing so is (usually) a bug and leads to several issues, such as:

However it is extremely easy to instantiate a Dispatcher on a non-UI thread. Broadly speaking there are two general ways this tends to happen:

It is usually possible for the disciplined solo programmer to avoid these issues in a small project. But sometimes people forget, and other times these issues are subtle and unexpected (e.g. I just recently learned that simply calling CommandManager.InvalidateRequerySuggested() creates a dispatcher). So these issues tend to crop up in larger projects with many developers. As a result it's virtually guaranteed that the software project will periodically encounter deadlocks, memory leaks, and other unexpected behavior.

A solution

I'd like to solve this problem by giving developers a proactive way to know when a dispatcher is instantiated. Developers can write #if DEBUG code that subscribes to the proposed event and inspects Thread.CurrentThread in the handler, and then log the stack trace when it's an undesirable thread. Then developers will know exactly where the offending code is and will be able to solve it promptly.

Alternatives

I think this would be better than the alternatives. I only know of a few alternatives (I'm open to hearing of more). In increasing order of hackiness:

  1. Use the Win32 API to be notified after a Win32 window is created. I assume such a hook would execute asynchronously, so this would really be no better than # 2, which is...
  2. Use reflection to poll Dispatcher._dispatchers (while under the Dispatcher._globalLock guard of course)
  3. Add a hook to intercept the call to the user32.dll > CreateWindowEx Win32 function call that happens while instantiating a dispatcher
  4. Edit the CLR method descriptor for Dispatcher.ctor or Dispatcher.get_CurrentDispatcher at runtime to point to my own hacked up method that does the same thing as this proposal but in a much messier way
  5. Run a fork of WPF

The proposal

public sealed class Dispatcher
{
  public static event DispatcherCreatedEventHandler? Created; // Add this event

  public static Dispatcher CurrentDispatcher
  {
    get
    {
      Dispatcher currentDispatcher = FromThread(Thread.CurrentThread);;
      if(currentDispatcher == null)
      {
        currentDispatcher = new Dispatcher();

        //// The following line is new ////
        Created?.Invoke();
        //// The above line is new ////

      }
      return currentDispatcher;
    }
  }
}

// Add this delegate
public delegate void DispatcherCreatedEventHander();
miloush commented 1 year ago

I do want to point out that using dispatcher on non-UI threads for dispatching prioritized operations (unrelated to UI) is a working scenario that I have used in the past.

I do not have anything against the API in principle (apart perhaps it could be just EventHandler), I am not so convinced by the motivation though, which seems to be to diagnose bugs in misbehaving code. (And therefore for example the event counterpart of dispatcher shutting down is not considered.) If that is the only need for the API, I think broader support for the proposal would help.

The Dispatcher utilizes several ETW events though. I am wondering whether introducing a dispatcher created ETW event instead would address your scenario?

lindexi commented 1 year ago

I think multi-threaded programming requires developers to have programming knowledge. This means that multi-threaded programming is not suitable for providing simple APIs, otherwise novice developers will accidentally use them and cause problems.

Keeping it complex can force developers to learn more about multi-threaded programming.

matthew-a-thomas commented 1 year ago

@miloush

I do want to point out that using dispatcher on non-UI threads for dispatching prioritized operations (unrelated to UI) is a working scenario that I have used in the past.

That's definitely valid. It's situations like that that kept me from proposing something more... drastic. Such as "block dispatcher creation on XYZ threads".

I do not have anything against the API in principle (apart perhaps it could be just EventHandler), I am not so convinced by the motivation though, which seems to be to diagnose bugs in misbehaving code. (And therefore for example the event counterpart of dispatcher shutting down is not considered.) If that is the only need for the API, I think broader support for the proposal would help.

Regarding EventHandler, I considered that but then we'd have to find something to use for both the object? sender and the EventArgs e parameters, and nothing comes to my mind that isn't readily available to subscribers—they can get the current dispatcher and the current thread, and other than that I don't see any other candidates.

Instead I tend to go the other direction. In fact I think I like just plain Action.

Regarding the motivation, yes I recognize there isn't much steam behind this proposal. But that's all the steam I have 😉

The Dispatcher utilizes several ETW events though. I am wondering whether introducing a dispatcher created ETW event instead would address your scenario?

I'm unfamiliar with ETW events. Is that Event Tracing for Windows?

matthew-a-thomas commented 1 year ago

@lindexi

I think multi-threaded programming requires developers to have programming knowledge. This means that multi-threaded programming is not suitable for providing simple APIs, otherwise novice developers will accidentally use them and cause problems.

Keeping it complex can force developers to learn more about multi-threaded programming.

I'm not sure I'm following your train of thought. Are you suggesting a more complicated solution to what I described in "The need"? If so, would you like to add to my list under "Alternatives"?

KalleOlaviNiemitalo commented 1 year ago

In principle, ETW seems nice for a diagnostic purpose like this. However, there could be complications in practice:

DiagnosticSource would be another option, more suitable for .NET in-process use; but WPF does not appear to use DiagnosticSource for anything yet, so if WPF used it just for this, it would be inconsistent.