Runtime: Support global unhandled exception handler

[terrajobst]

As outlined in https://github.com/dotnet/coreclr/issues/2999, we need a replacement for AppDomain.UnhandledException. Couple of requirements:

• Should be a low-level API that isn't depending on other orthogonal features such as AssemblyLoadContext, Task, or Thread
• Can't be on a type that is already exposed in .NET Framework (such as Environment)
• Should be raised for all threads

Thoughts?

/cc @weshaggard @KrzysztofCwalina @stephentoub

[stephentoub]

Shouldn't be a low-level API

I assume you meant "Should be"?

Thoughts?

What would the handler do, simply provide a hook to be notified about an exception as it's on its way to crashing the app and do something like some logging or some quick cleanup? Or go further and allow you to eat it and stop the app from crashing? I'm ok with the former... I'd be more concerned about the latter.

[KrzysztofCwalina]

I am pretty sure (and hopeful) it's the former. it might be worth adding a note about it to the issue.
... And my thoughts are that it's a good approach.

[NickCraver]

@stephentoub I think the former meets almost all _reasonable_ needs. I'm not sure how you'd possibly handle the latter - a global "recover from anything" scenario seems a bit on the insane side to me. I agree on staying away from attempting anything like that.

I'm looking forward to this one personally because of StackExchange.Exceptional - we want to log all errors that are encountered and hopefully developers can hook up such functionality in a very trivial way (as they can today).

[stephentoub]

a global "recover from anything" scenario seems a bit on the insane side to me

Yes :wink:

[nguerrera]

I think the following could work and meet all of the criteria:

C# namespace System { public static class UnhandledException { public static event UnhandledExceptionEventHandler Raised; } }

That would reuse UnhandledExceptionEventHandler and UnhandledExceptionEventArgs and group next to them in IntelliSense.

[justinvp]

Adding back just AppDomain.CurrentDomain.UnhandledException is off the table?

[nguerrera]

We are avoiding re-introducing System.AppDomain because AppDomains are not a concept in .NET Core.

[justinvp]

We are avoiding re-introducing System.AppDomain because AppDomains are not a concept in .NET Core.

I understand the desire -- I'm just wondering if it'd be more practical (both for implementing it and for developers porting existing code to .NET Core already familiar with the existing API) to just expose the existing API (without adding back any other APIs on AppDomain). Its existence in .NET Core could be explained as "there's only ever a single AppDomain, the current one".

[NickCraver]

I agree with not bringing back AppDomain for this - it's both confusing and incorrect. This is a process-level event handler, not an "app domain" one, since that level of containment no longer exists.

[aL3891]

How about Exception.[On]Unhandled? (A static event on the Exception class)

[nguerrera]

How about Exception.[On]Unhandled ? (A static event on the Exception class)

That would break this criteria from the description:

Can't be on a type that is already exposed in .NET Framework (such as Environment )

[mellinoe]

namespace System {
  public static class UnhandledException {
    public static event UnhandledExceptionEventHandler Raised;
  }
}

This seems simple and good to me.

[svick]

@nguerrera I don't like that that doesn't fit with the naming in the rest of the framework. Only exceptions are named ending with Exception and events aren't called things like Raised.

Maybe naming the class something general, like Exceptions could work (so the event would be Exceptions.UnhandledException)? Or ExceptionHandling? Something along those lines.

[nguerrera]

Only exceptions are named ending with Exception

Good point.

[nguerrera]

What do folks think about AppContext.UnhandledException? We've already moved other things you used to do via AppDomain.CurrentDomain there.

[nguerrera]

Ah, that's another existing desktop type, isn't it.

[NickCraver]

Yep, AppContext was in 4.6.

[justinvp]

Something else to keep in mind: When the semi-related AppDomain.FirstChanceException event was added in 4.0, we added FirstChanceExceptionEventArgs to the System.Runtime.ExceptionServices namespace instead of System (to avoid adding new clutter to the System namespace) and used EventHandler<T> instead of creating a new delegate.

If a new API is created,

1. Should it go in the System namespace or System.Runtime.ExceptionServices (or other namespace)?
2. Should System.UnhandledExceptionEventArgs be reused, or should a similar type be added under System.Runtime.ExceptionServices (or other namespace)?
3. Should System.UnhandledExceptionEventHandler be reused, or since this is a new API, just use EventHandler<T>?

[SamuelEnglard]

When would this new method be raised? When AppDomain.UnhandledException was raised or when AppDomain.FirstChanceException was? I use FirstChanceException to get dumps of the application to help debugging and I could see that being useful in core too.

@terrajobst why can't it be on an existing type?

[NickCraver]

@SamuelEnglard it can't be on an existing type because that type exists on desktop CLR already and can't be forwarded properly if it has new members not already on the desktop CLR method. It's a type forwarding issue that affects a lot of API decisions. With a new type, we're loading the new type (instead of forwarding) and have a lot more flexibility.

[aL3891]

Maybe GlobalEvents.UnhandledException / FirstChanceException There could be other events that might be useful as well for debugging or profiling that could be added there down the line like the process exiting.

Another alternatives, ApplicationEvents, ProcessEvents

[terrajobst]

@aL3891 GlobalEvents, ApplicationEvents, ProcessEvents

They all sound a bit too generic for my taste. I liked @nguerrera suggestion. The issue with the Exception suffix is well taken, so what about this:

C# namespace System.Runtime { public static class UnhandledError { public static event UnhandledExceptionEventHandler Raised; } }

[SamuelEnglard]

@terrajobst I actually kind of like *Events because it gives a good place for other events in the future, kind of like Microsoft.Win32.SystemEvents

[justinvp]

@terrajobst,

1. Why System.Runtime namespace and not System.Runtime.ExceptionServices?
2. Where would the AppDomain.FirstChanceException event be added if it was eventually added to .NET Core? I don't think it'd make sense on a class named UnhandledError and not sure I like the idea of adding another type for it (e.g. FirstChanceError.Raised?).

How about something like this:

``` c#
namespace System.Runtime.ExceptionServices
{
public static class ExceptionEvents
{
public static event EventHandler UnhandledException;
}

public class UnhandledExceptionEventArgs : EventArgs
{
    public UnhandledExceptionEventArgs(object exception, bool isTerminating);
    public object ExceptionObject { get; }
    public bool IsTerminating { get; }
}

}
```

I'm open to better suggestions for the ExceptionEvents name. It's less generic than Global/Application/Process, but not so specific that it prevents other Exception events from being added to it. I think UnhandledError is too specific and I'm not sure about the Raised name for the event. I know we talk about events as being "raised" vs "fired" or "triggered", but we don't have any prior art naming events Raised (AFAICT) and the terminology bleeds into the confusion around exceptions being "thrown" vs "raised" that might add to the confusion.

In the proposal above, UnhandledExceptionEventArgs is a new type added to System.Runtime.ExceptionServices. Yes, it is the same name as the type in the System namespace in the full framework and I think it's OK to break the general rule of not having types with the same name in different namespaces for this event args type. I don't think there would be a lot of confusion as the documentation for both would be the same.

EventHandler<T> is used instead of adding back UnhandledExceptionEventHandler in the System namespace.

If the FirstChanceException event is ever needed in .NET Core, it can be added to the same ExceptionEvents class. It's FirstChanceExceptionEventArgs type is already in the System.Runtime.ExceptionServices namespace in the full framework.

[SamuelEnglard]

@terrajobst I like what @justinvp has suggested.

I'd make ExceptionObject of type Exception though. I don't think the reason for it being of type object in the full framework are valid here.

[justinvp]

I believe the reason it is typed as object in the full framework is because the CLR allows any object to be thrown (which some languages allow). Not sure if that's applicable in .NET Core.

[NickCraver]

Is there any progress on this API? I'd hate for this to miss RC2 since it's the last API needed to port several applications here (that use our global error handler to monitor the entire network).

I want to throw another +1 for @justinvp's proposal above, though Exception seems too redundant in the namespace, type, _and_ each event. I'd shorten the event names to remove the suffix, e.g. just Unhandled:

``` C#
namespace System.Runtime.ExceptionServices
{
public static class ExceptionEvents
{
public static event EventHandler Unhandled;
}

public class UnhandledExceptionEventArgs : EventArgs
{
    public UnhandledExceptionEventArgs(object exception, bool isTerminating);
    public object ExceptionObject { get; }
    public bool IsTerminating { get; }
}

}

This means the usage becomes:

``` C#
ExceptionEvents.Unhandled += (o, args) => {};

[jchannon]

This has been labelled 1.0.0-rtm, @terrajobst any chance we can get this into RC2?

[niemyjski]

We could really use this too.

[SamuelEnglard]

If you need it now... https://www.nuget.org/packages/System.AppDomain/1.1.0

[richardszalay]

The proposed API doesn't appear to allow the handler to be asynchronous or otherwise delay the impending termination. Given that the IO operations are all asynchronous, and that most use cases of this API would involve logging it somewhere, is that going to be an issue? Or is the intent that handlers would block on asynchronous IO APIs?

[SamuelEnglard]

@richardszalay the issue with asynchronous handling here is that once the app is here things could be really bad. While yes IO should be asynchronous I don't think it's worth the chance of abuse. If it's not a terminating issue just do the IO fire and forget style. Otherwise you should block.

[danmosemsft]

AppDomain.UnhandledException came back with https://github.com/dotnet/corefx/pull/11275. for .NET Core 1.2. @terrajobst please close if you agree this is not necessary anymore.

[niemyjski]

On Wed, Oct 5, 2016 at 4:48 PM, Dan Moseley [email protected]
wrote:

dotnet/corefx#11275

Any idea when .NET Core 1.1 or 1.2 will be released?

Thanks
-Blake Niemyjski

[terrajobst]

@danmosemsft

AppDomain.UnhandledException came back with https://github.com/dotnet/corefx/pull/11275. for .NET Core 1.2. @terrajobst please close if you agree this is not necessary anymore.

Agreed, it's now redundant.

@niemyjski

Any idea when .NET Core 1.1 or 1.2 will be released?

We don't have timelines yet, but the goal is to roughly align with the Dev15 timeframe.

[jinek]

I think it is a lack in .NET that we can't catch exception. If we know the type of exception and want to drive something using that exception (ThreadAbortException is a good example) why not to allow to catch it.

[danmosemsft]

@jinek if you'd like to propose that please open a new issue, including scenarios where this is valuable (and safe). In particular, your ThreadAbortException example.