Exceptions on background thread are not handled and reported correctly

[DrummerB]

Environment

How do you use Sentry?
sentry.io

Which version of the SDK?
0.20.1

How did you install the package?
UPM, git-url

Which version of Unity?
2021.3.6f1 LTS

Is this happening in Unity (editor) or on a player like Android, iOS, Windows?
Reproduced in Windows Editor and Windows Player

Steps to Reproduce

1. Create a new empty Unity project.
2. Import sentry-unity 0.20.1 in UPM.
3. Attach the code below to a GameObject
4. Start the game
5. Throw some exceptions in a background thread (click the in-game button).

Expected Result

Exceptions are reported as events to sentry.io

Actual Result

Only the first exception is handled and reported correctly.

Something seems to go wrong within the SDK, because after the first exception, subsequent exceptions do not result in any debug logs from Sentry, except a single "Sentry: (Info) Disposing the Hub", which is directly followed by:

exception inside UnhandledException handler: (null) assembly:C:\Program Files\Unity\Hub\Editor\2021.3.6f1\Editor\Data\MonoBleedingEdge\lib\mono\unityjit-win32\mscorlib.dll type:Exception member:(null)

Code

using System;
using System.Threading;
using UnityEngine;

public class SentryTest : MonoBehaviour
{
	void OnGUI()
	{
		if (GUILayout.Button("Throw Exception On Background Thread"))
		{
			Debug.Log("Pressed button");
			var thread = new Thread(() =>
			{
				Thread.Sleep(1000);
				Debug.Log("Will throw exception");
				throw new Exception("Testing Background Thread Crash 2");
			});
			thread.Start();	
		}
	}
}

Any logs or screenshots

editor-log.txt

[bitsandfoxes]

@DrummerB thanks for raising this and providing the snippet. I can reproduce the issue locally and we're going to take a look!

[vaind]

I was able to verify this issue. The stack trace for that being logged indicates its caused by the AppDomainUnhandledExceptionIntegration

Sentry: (Info) Disposing the Hub. 
UnityEngine.DebugLogHandler:LogFormat (UnityEngine.LogType,UnityEngine.Object,string,object[])
Sentry.Unity.Integrations.UnityLogHandlerIntegration:LogFormat (UnityEngine.LogType,UnityEngine.Object,string,object[]) (at C:/dev/sentry-unity/src/Sentry.Unity/Integrations/UnityLogHandlerIntegration.cs:80)
UnityEngine.Debug:Log (object)
Sentry.Unity.UnityLogger:Log (Sentry.SentryLevel,string,System.Exception,object[]) (at C:/dev/sentry-unity/src/Sentry.Unity/UnityLogger.cs:41)
Sentry.Extensibility.DiagnosticLoggerExtensions:LogIfEnabled (Sentry.Extensibility.IDiagnosticLogger,Sentry.SentryLevel,System.Exception,string)
Sentry.Extensibility.DiagnosticLoggerExtensions:LogInfo (Sentry.SentryOptions,string)
Sentry.Internal.Hub:Dispose ()
Sentry.Integrations.AppDomainUnhandledExceptionIntegration:Handle (object,System.UnhandledExceptionEventArgs)
Sentry.Internal.AppDomainAdapter:OnUnhandledException (object,System.UnhandledExceptionEventArgs)

After that dispose is called, no more errors are captured during the same runtime (regardless of the thread). I didn't get the exception user mentions above, but that may be irrelevant - the main problem here is the Hub is shut down on an exception thrown in a background thread.

[vaind]

This looks like sentry-dotnet bug to me, adding a flag.

[bruno-garcia]

This makes sense in .NET in general since once we return the process crashes, so we flush in there. I believe Hub.Dispose was changed to only flush. If not, maybe that's the fix

[bruno-garcia]

from the PoV of Unity, this is a p0

[vaind]

.NET in general since once we return the process crashes, so we flush in there.

Checking if that's the case - this indicates it's not - https://stackoverflow.com/a/6465586/2386130

[vaind]

In the end, whether it crashes or not on the given platform doesn't matter. If we think back about what this code is trying to achieve:

sentry-dotnet/src/Sentry/Integrations/AppDomainUnhandledExceptionIntegration.cs

Lines 28 to 41 in ab29664

	internal void Handle(object sender, UnhandledExceptionEventArgs e)
	{
	if (e.ExceptionObject is Exception ex)
	{
	ex.Data[Mechanism.HandledKey] = false;
	ex.Data[Mechanism.MechanismKey] = "AppDomain.UnhandledException";
	_ = _hub?.CaptureException(ex);
	}
	if (e.IsTerminating)
	{
	(_hub as IDisposable)?.Dispose();
	}
	}

the point isn't disposing of the hub (thus effectively disabling it, because of line 392):

sentry-dotnet/src/Sentry/Internal/Hub.cs

Lines 387 to 399 in ab29664

	public void Dispose()
	{
	_options.LogInfo("Disposing the Hub.");
	if (Interlocked.Exchange(ref _isEnabled, 0) != 1)
	{
	return;
	}
	_ownedClient.FlushAsync(_options.ShutdownTimeout).GetAwaiter().GetResult();
	//Dont dispose of _rootScope and ScopeManager since we want dangling transactions to still be able to access tags.
	}

• what we really want to do is is flushing the just created event so that it's not lost if the app crashes.

My suggestion is to change the code so that it doesn't dispose of the Hub, but instead flushes events. There are a few non-breaking changes we can do to achieve this. E. g. we can add an internal method to the Hub: FlushSync().

[bruno-garcia]

.NET in general since once we return the process crashes, so we flush in there.

Checking if that's the case - this indicates it's not - stackoverflow.com/a/6465586/2386130

We don't support .NET Framework 1.0 through 2.0. In the versions we support, unless you set args.Handled=true (granted, this could be done after we run our code if they subscribed to the event after us), the app will terminte.

My suggestion is to change the code so that it doesn't dispose of the, but instead flushes events. There are a few non-breaking changes we can do to achieve this. E. g. we can add an internal method to the Hub: FlushSync().

Agreed. See: #599

[mattjohnsonpint]

So, if in the AppDomainUnhandledExceptionIntegration, we don't dispose the hub but just flush it, then we should be ok here, right?

[bruno-garcia]

Thanks for the fix @mattjohnsonpint
@DrummerB we'll make a release of the Unity sdk with this fix soon

[DrummerB]

Great, thanks for the fix!