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

Implement Dispatcher.UnhandledException and Dispatcher.UnhandledExceptionFilter #14432

Merged
merged 15 commits into from
Feb 24, 2024

Conversation

maxkatz6
Copy link
Member

@maxkatz6 maxkatz6 commented Feb 1, 2024

How was the solution implemented (if it's not obvious)?

Implements both Dispatcher.UnhandledException and Dispatcher.UnhandledExceptionFilter, by copypasting error handling logic from WPF. And adjusting DispatcherOperation logic in Avalonia.

The same as in WPF, this feature is not recommended for general use. Major nuances:

  1. Many Avalonia events needs to be wrapped into DispatcherOperation.Send call, so exception in these handlers can be passed to the Dispatcher.UnhandledException. This PR ONLY implements it for TopLevel.Input events - basically the most common place for exceptions. I plan to adjust more places in following PR, but for the most part we are going to leave it for contributors, as the fundamental part is done in this PR.
  2. Any exception has a potential to break Avalonia internal state. And yes, after many years the same can happen in WPF - try to throw exception in render size changed handles of Window.
  3. Dispatcher.Invoke/InvokeAsync events bypass Dispatcher.UnhandledException completely. General rule of thumb - if method can return exception (by throwing it or as part of DispatcherOperation object), then UnhandledException is not executed (as these exceptions can be in fact handled by the used). The same behavior work in WPF (except old method based on LegacyInvokeImpl, but that's where WPF is even more broken).

Checklist

Breaking changes

None

Fixed issues

Closes #13476
Fixes #13393
Fixes #8418

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.1.999-cibuild0044265-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@ltetak
Copy link
Contributor

ltetak commented Feb 1, 2024

This is a great re-implementation of the WPF handling, thanks!
Please, make sure to check #14229 as well. It happens that the loop that handles Dispatcher queue is broken (the native implementation stops and is not restarted ever after).
@maxkatz6 @kekekeks

}
else
{
InvokeAsyncImpl(new SendOrPostCallbackDispatcherOperation(this, priority, action, arg, true),
Copy link
Member

Choose a reason for hiding this comment

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

Send implies a synchronous wait, InvokeAsyncImpl won't wait for the operation to be completed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.


_inputManager?.ProcessInput(e);
topLevel._inputManager?.ProcessInput(e);
}, (this, e), DispatcherPriority.Send);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if we should be changing the current DispatcherPriority to Send, it will affect the SynchronizationContext used for the callback.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what WPF does.
Also, with other priorities dispatcher operation won't be executed immediately.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@kekekeks kekekeks Feb 11, 2024

Choose a reason for hiding this comment

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

Yes, but it will make

await Foo();
window.Close();

to cause native crashes on macOS in certain code paths (render being forced via [NSView updateLayer]) again.

It was one of the reasons why we've changed our default dispatcher priority to be lower than Render.

Not sure how to proceed here, it's technically a behavior change.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kekekeks changed the way how Dispatcher.Send works. Also fixing behavioral change my previous changes introduced.
Both AvaloniaSynchronizationContext.Send and Dispatcher.Send shouldn't change current sync context, when we already on the dispatcher thread.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.1.999-cibuild0044808-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.1.999-cibuild0045132-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.1.999-cibuild0045305-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.1.999-cibuild0045322-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@kekekeks kekekeks added this pull request to the merge queue Feb 24, 2024
Merged via the queue into master with commit 1ad5107 Feb 24, 2024
7 checks passed
@kekekeks kekekeks deleted the dispatcher-unhandled-exception branch February 24, 2024 07:58
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.

DispatcherUnhandledException alternative Introduce DispatcherUnhandledException
4 participants