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

Support global unhandled exception handler #16468

Closed
terrajobst opened this issue Feb 25, 2016 · 37 comments
Closed

Support global unhandled exception handler #16468

terrajobst opened this issue Feb 25, 2016 · 37 comments
Assignees
Milestone

Comments

@terrajobst
Copy link
Member

@terrajobst terrajobst commented Feb 25, 2016

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

@terrajobst terrajobst self-assigned this Feb 25, 2016
@stephentoub
Copy link
Member

@stephentoub stephentoub commented Feb 25, 2016

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
Copy link
Member

@KrzysztofCwalina KrzysztofCwalina commented Feb 25, 2016

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
Copy link
Member

@NickCraver NickCraver commented Feb 25, 2016

@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
Copy link
Member

@stephentoub stephentoub commented Feb 25, 2016

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

Yes 😉

@nguerrera
Copy link
Member

@nguerrera nguerrera commented Feb 25, 2016

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

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
Copy link
Collaborator

@justinvp justinvp commented Feb 25, 2016

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

@nguerrera
Copy link
Member

@nguerrera nguerrera commented Feb 25, 2016

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

@justinvp
Copy link
Collaborator

@justinvp justinvp commented Feb 25, 2016

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
Copy link
Member

@NickCraver NickCraver commented Feb 25, 2016

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
Copy link

@aL3891 aL3891 commented Feb 25, 2016

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

@nguerrera
Copy link
Member

@nguerrera nguerrera commented Feb 25, 2016

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
Copy link

@mellinoe mellinoe commented Feb 25, 2016

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

This seems simple and good to me.

@svick
Copy link
Contributor

@svick svick commented Feb 26, 2016

@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
Copy link
Member

@nguerrera nguerrera commented Feb 26, 2016

Only exceptions are named ending with Exception

Good point.

@nguerrera
Copy link
Member

@nguerrera nguerrera commented Feb 26, 2016

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

@nguerrera
Copy link
Member

@nguerrera nguerrera commented Feb 26, 2016

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

@NickCraver
Copy link
Member

@NickCraver NickCraver commented Feb 26, 2016

Yep, AppContext was in 4.6.

@justinvp
Copy link
Collaborator

@justinvp justinvp commented Feb 26, 2016

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
Copy link
Collaborator

@SamuelEnglard SamuelEnglard commented Feb 26, 2016

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
Copy link
Member

@NickCraver NickCraver commented Feb 27, 2016

@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
Copy link

@aL3891 aL3891 commented Feb 27, 2016

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
Copy link
Member Author

@terrajobst terrajobst commented Mar 2, 2016

@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:

namespace System.Runtime {
  public static class UnhandledError {
    public static event UnhandledExceptionEventHandler Raised;
  }
}
@SamuelEnglard
Copy link
Collaborator

@SamuelEnglard SamuelEnglard commented Mar 2, 2016

@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
Copy link
Collaborator

@justinvp justinvp commented Mar 2, 2016

@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:

namespace System.Runtime.ExceptionServices
{
    public static class ExceptionEvents
    {
        public static event EventHandler<UnhandledExceptionEventArgs> 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
Copy link
Collaborator

@SamuelEnglard SamuelEnglard commented Mar 2, 2016

@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
Copy link
Collaborator

@justinvp justinvp commented Mar 2, 2016

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
Copy link
Member

@NickCraver NickCraver commented Apr 26, 2016

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:

namespace System.Runtime.ExceptionServices
{
    public static class ExceptionEvents
    {
        public static event EventHandler<UnhandledExceptionEventArgs> Unhandled;
    }

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

This means the usage becomes:

ExceptionEvents.Unhandled += (o, args) => {};
@jchannon
Copy link

@jchannon jchannon commented Apr 29, 2016

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

@niemyjski
Copy link

@niemyjski niemyjski commented May 16, 2016

We could really use this too.

@SamuelEnglard
Copy link
Collaborator

@SamuelEnglard SamuelEnglard commented May 17, 2016

@richardszalay
Copy link

@richardszalay richardszalay commented Jul 10, 2016

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
Copy link
Collaborator

@SamuelEnglard SamuelEnglard commented Sep 12, 2016

@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
Copy link
Member

@danmosemsft danmosemsft commented Oct 5, 2016

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

@niemyjski
Copy link

@niemyjski niemyjski commented Oct 5, 2016

On Wed, Oct 5, 2016 at 4:48 PM, Dan Moseley notifications@github.com
wrote:

dotnet/corefx#11275

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

Thanks
-Blake Niemyjski

@terrajobst
Copy link
Member Author

@terrajobst terrajobst commented Oct 5, 2016

@danmosemsft

AppDomain.UnhandledException came back with dotnet/corefx#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.

@terrajobst terrajobst closed this Oct 5, 2016
@jinek
Copy link

@jinek jinek commented Aug 16, 2018

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
Copy link
Member

@danmosemsft danmosemsft commented Aug 16, 2018

@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.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.0.0 milestone Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.