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

Add an event for unhandled exceptions from delegates invoked by native code #860

Closed
wants to merge 1 commit into from

Conversation

cameronwhite
Copy link
Contributor

These unhandled exceptions would otherwise immediately terminate the program once they reach the native code boundary. This event lets applications do any logging / message dialogs etc and decide whether they want to terminate the program or attempt to continue.

Currently this only supports signal handlers for testing purposes, but would need to be extended to the other callback types.

Bug: #845

  • I agree that my contribution may be licensed either under MIT or any version of LGPL license.

@cameronwhite cameronwhite marked this pull request as draft April 30, 2023 17:12
Copy link
Member

@badcel badcel left a comment

Choose a reason for hiding this comment

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

The concept looks good to me. Please see my comments.

src/Tests/Libs/GirTest-0.1.Tests/ExceptionManagerTest.cs Outdated Show resolved Hide resolved
src/Tests/Libs/GirTest-0.1.Tests/ExceptionManagerTest.cs Outdated Show resolved Hide resolved
src/Libs/GLib-2.0/Public/ExceptionManager.cs Outdated Show resolved Hide resolved
@badcel
Copy link
Member

badcel commented Apr 30, 2023

I wonder if it should be an event at all? Is it useful to have more than one subscriber?

If not perhaps a simple callback should be used instead of an event?

Something similar to: GLib.UnhandledException.SetHandler(..) and GLib.UnhandledException.Raise(..)

or even better additionally something like:
GLib.UnhandledException.SetHandler<T>(..) where T : Exception

which could allow code like:

GLib.UnhandledException.SetHandler<NullReferenceException>(..);
GLib.UnhandledException.SetHandler<OutOfMemoryException>(..);
GLib.UnhandledException.SetHandler(..); // all other exceptions

Edit:
I probably would skip the generic method. It just adds to much complexity. If there would be user demand to handle certain exception types separately we could always add something similar later.

…e code

These unhandled exceptions would otherwise immediately terminate the program once they reach the native code boundary. This event lets applications do any logging / message dialogs etc and decide whether they want to terminate the program or attempt to continue.

Bug: #845
@cameronwhite
Copy link
Contributor Author

Thanks! Updated with those changes.

That's a good question about it being an event .. this was mostly following GtkSharp's GLib.ExceptionManager, which I think in turn was probably following AppDomain.UnhandledException, so I guess there is some precedent there.
I suppose it might give a bit more flexibility for e.g. having one handler do logging, and a handler in another component adding some UI notification

In Pinta we only ever had one handler though, which pops open a message dialog and prints some info to the console

@badcel
Copy link
Member

badcel commented May 1, 2023

I wonder if the generic handler approach enables this scenario:

  • catch exception on the native side
  • wrap exception in custom exception
  • signal the unhandled exception
  • receive the event in the signal handler with the wrapped exception on the managed side
  • throw original exception

Actually what happens if an exception is thrown in an event handler if it is purely managed? The runtime stops, too? Then this would be no help as the user is responsible to add a try/catch block in the handler themselves.

@cameronwhite
Copy link
Contributor Author

Replaced by #861

@cameronwhite cameronwhite deleted the feature/exception-handling branch May 1, 2023 20:50
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.

2 participants