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

on async Main, unhandled exception unregisters before capturing crash #321

Closed
bruno-garcia opened this issue Dec 9, 2019 · 10 comments · Fixed by getsentry/sentry-docs#4895 or #1656
Assignees
Projects

Comments

@bruno-garcia
Copy link
Member

static void Main(string[] args)
{
    using var _ = SentrySdk.Init();
    throw null;
}

This exception is captured as expected by the unhandled exception handler.

static async Task Main(string[] args)
{
    using var _ = SentrySdk.Init();
    throw null;
}

The code above doesn't work as expected. Before calling the UnhandledException handler the SDK goes out of the using "block" and unregisters the unhandled exception integration.

@bruno-garcia bruno-garcia added the Bug Something isn't working label Dec 9, 2019
@kkl-acies
Copy link

The workaround for the time being is to create a async main: https://github.com/dotnet/csharplang/blob/master/proposals/csharp-7.1/async-main.md

static void Main(string[] args)
{
    using (SentrySdk.Init("https://xx"))
    {
        MainAsync().Wait();
    }
}
private static async Task MainAsync()
{
    throw new Exception("foobar");
    await Task.Run(() =>
    {
        Console.WriteLine("foo");
    });
    Console.WriteLine("Hello World!");
}

@Tyrrrz
Copy link
Contributor

Tyrrrz commented Sep 15, 2020

This doesn't look like a bug in sentry, but more like an expected behavior due to misuse of async. When you add async modifier to the method, the throw expression changes meaning. Instead of throwing an exception immediately on the stack, they are wrapped in a Task.

You can observe this behavior in the following example:

void A()
{
    throw null;
}

async Task B()
{
    throw null;
}

void C()
{
    // Will throw immediately
    A();

    // Will not throw
    var task = B();

    // Can inspect exception. Awaiting the task will also propagate the exception.
    task.Exception;
}

Here are the two methods from your original example, decompiled:

public static class Program
{
    private static void Main(string[] args)
    {
        IDisposable disposable = SentrySdk.Init();
        try
        {
            throw null;
        }
        finally
        {
            if (disposable != null)
            {
                disposable.Dispose();
            }
        }
    }
}
[CompilerGenerated]
    private sealed class <Main>d__0 : IAsyncStateMachine
    {
        public int <>1__state;

        public AsyncTaskMethodBuilder <>t__builder;

        public string[] args;

        private IDisposable <_>5__1;

        private void MoveNext()
        {
            int num = <>1__state;
            try
            {
                <_>5__1 = SentrySdk.Init();
                try
                {
                    throw null;
                }
                finally
                {
                    if (num < 0 && <_>5__1 != null)
                    {
                        <_>5__1.Dispose();
                    }
                }
            }
            catch (Exception exception)
            {
                <>1__state = -2;
                <>t__builder.SetException(exception);
            }
        }

        void IAsyncStateMachine.MoveNext()
        {
            //ILSpy generated this explicit interface implementation from .override directive in MoveNext
            this.MoveNext();
        }

        [DebuggerHidden]
        private void SetStateMachine(IAsyncStateMachine stateMachine)
        {
        }

        void IAsyncStateMachine.SetStateMachine(IAsyncStateMachine stateMachine)
        {
            //ILSpy generated this explicit interface implementation from .override directive in SetStateMachine
            this.SetStateMachine(stateMachine);
        }
    }

    [AsyncStateMachine(typeof(<Main>d__0))]
    [DebuggerStepThrough]
    private static Task Main(string[] args)
    {
        <Main>d__0 stateMachine = new <Main>d__0();
        stateMachine.args = args;
        stateMachine.<>t__builder = AsyncTaskMethodBuilder.Create();
        stateMachine.<>1__state = -1;
        AsyncTaskMethodBuilder <>t__builder = stateMachine.<>t__builder;
        <>t__builder.Start(ref stateMachine);
        return stateMachine.<>t__builder.Task;
    }

The latter generates a (useless) state machine and the exception actually gets caught and stored in Task.Exception. Because of that, the unhandled exception event is never triggered at all.

Note, the compiler also warns when you write code like this, which kind of implies that it's probably a mistake.

@bruno-garcia
Copy link
Member Author

@mattjohnsonpint could u please validate that for us and close it if it's OK?

@mattjohnsonpint
Copy link
Contributor

I just checked. It's still an issue with the latest version (3.15.0). I'll see what I can figure out.

@mattjohnsonpint
Copy link
Contributor

mattjohnsonpint commented Mar 31, 2022

A more realistic reproduction:

using Sentry;

namespace ConsoleApp1;

public class Program
{
    static async Task Main()
    {
        using var _ = SentrySdk.Init(o =>
        {
            o.Dsn = // ... my dsn
            o.Debug = true;
        });

        await DoSomethingAsync();
    }

    static async Task DoSomethingAsync()
    {
        await Task.Delay(1000);
        throw new Exception("Test Exception");
    }
}

The unhandled exception is thrown, but not captured by the Sentry SDK.

@mattjohnsonpint
Copy link
Contributor

@Tyrrrz was correct with #321 (comment). It's not caused by Sentry, and I don't think there's much we can do about it.

Here is a reproduction that doesn't use Sentry at all:

public class Program
{
    static async Task Main()
    {
        using var _ = new MyUnhandledExceptionHandler();
        await DoSomethingAsync();
    }

    static async Task DoSomethingAsync()
    {
        await Task.Yield();
        throw new Exception("Test Exception");
    }
}

public class MyUnhandledExceptionHandler : IDisposable
{
    public MyUnhandledExceptionHandler()
    {
        AppDomain.CurrentDomain.UnhandledException += OnCurrentDomainOnUnhandledException;
    }
    
    public void Dispose()
    {
        AppDomain.CurrentDomain.UnhandledException -= OnCurrentDomainOnUnhandledException;
    }

    private void OnCurrentDomainOnUnhandledException(object sender, UnhandledExceptionEventArgs args)
    {
        if (args.ExceptionObject is Exception e)
            Console.WriteLine($"*** Caught {e.Message} ***");
    }
}

The message is never printed because the handler is disposed and unregistered before the task unwinds. Replace the Main method with the following and it works fine:

    static void Main()
    {
        using var _ = new MyUnhandledExceptionHandler();
        MainAsync().GetAwaiter().GetResult();
    }

    static async Task MainAsync()
    {
        await DoSomethingAsync();
    }

This appears to be by design of .NET, but I will ask on the dotnet/runtime repo. We should also add a note in our SDK docs about this.

Also note that @kkl-acies showed using .Wait(), but that will always wrap the exception in an AggregateException. It is preferred to use .GetAwaiter().GetResult()

@mattjohnsonpint mattjohnsonpint added Documentation Impact: Small and removed Bug Something isn't working labels Apr 1, 2022
@mattjohnsonpint
Copy link
Contributor

I asked at dotnet/runtime#67407

@mattjohnsonpint
Copy link
Contributor

And they answered. As suspected, it's by design of .NET. The exception is caught and stored with the task, then the handler is disposed, then the task unwraps and the exception is thrown without anything registered to catch it.

I'll add a note to the docs.

@bruno-garcia
Copy link
Member Author

We can improve this by making sure IDisposable doesn't close up the SDK (and unregisters the AppDomain unhandled exception). Even though the Dispose logic of a lot of stuff is still in there, it's not supposed to be called anymore by the Disposable returned by Init. That's because of the changed done here (#1354) for this request: #599

It's possibly we missed something, but if we make the Disposable not dispose anything anymore we could work around this design flaw

@mattjohnsonpint
Copy link
Contributor

This is fully resolved between #2103 and #2319. Async main, and top-level async main, should work fine now (after those are both released).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment