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

Close sentry instance when quitting the app #608

Merged
merged 6 commits into from
Mar 9, 2022
Merged

Conversation

vaind
Copy link
Collaborator

@vaind vaind commented Mar 3, 2022

closes #543

Native integrations

  • standalone native (windows)
  • iOS
  • Android

@vaind
Copy link
Collaborator Author

vaind commented Mar 3, 2022

My original approach was to listen to SentryMonoBehaviour.OnApplicationQuit() - that's what's currently implemented in the current WIP version of this PR. However, I've now noticed there's also another implementation, using Application.Quitting, also used here:

private void OnQuitting()
{
_sentryOptions?.DiagnosticLogger?.LogInfo("OnQuitting was invoked. Unhooking log callback and pausing session.");
// Note: iOS applications are usually suspended and do not quit. You should tick "Exit on Suspend" in Player settings for iOS builds to cause the game to quit and not suspend, otherwise you may not see this call.
// If "Exit on Suspend" is not ticked then you will see calls to OnApplicationPause instead.
// Note: On Windows Store Apps and Windows Phone 8.1 there is no application quit event. Consider using OnApplicationFocus event when focusStatus equals false.
// Note: On WebGL it is not possible to implement OnApplicationQuit due to nature of the browser tabs closing.
_application.LogMessageReceived -= OnLogMessageReceived;
// 'OnQuitting' is invoked even when an uncaught exception happens in the ART. To make sure the .NET
// SDK checks with the native layer on restart if the previous run crashed (through the CrashedLastRun callback)
// we'll just pause sessions on shutdown. On restart they can be closed with the right timestamp and as 'exited'.
_hub?.PauseSession();
_hub?.FlushAsync(_sentryOptions?.ShutdownTimeout ?? TimeSpan.FromSeconds(1)).GetAwaiter().GetResult();
}

Now I'm confused which one would be the best one to use - they seem to be duplicate implementations binding to the same event..

@bitsandfoxes
Copy link
Contributor

The SentryMonoBehaviour was added to give us access to callbacks on the MonoBehaviour and things from inside Unity itself. It's an actual game object that receives the OnApplicationQuit inherently to destroy itself.

In your case, I'd go with the IApplication and use the OnQuitting there.

@vaind vaind force-pushed the chore/close-sentry-on-quit branch from 3dfea1c to 9ed8200 Compare March 4, 2022 10:52
@vaind vaind force-pushed the chore/close-sentry-on-quit branch from 9ed8200 to 08e198f Compare March 8, 2022 14:36
@vaind vaind marked this pull request as ready for review March 8, 2022 14:37
Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

One questions but other than that. lgtm

src/Sentry.Unity.Android/SentryNativeAndroid.cs Outdated Show resolved Hide resolved
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
@vaind vaind force-pushed the chore/close-sentry-on-quit branch from c3882f4 to c1f7b14 Compare March 9, 2022 10:17
@vaind vaind enabled auto-merge (squash) March 9, 2022 17:32
Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

The only risk I see is that an error/crash happens after the Unity's AppShutdown thing ran, and we'd be all shut down and not capture. For that reason, recently we've changed the .NET SDK to have the Dispose handle returned by the Init call behave like a Flush only since that's what the semantics where in the code snippets (block to capture and flush). So the SDK would continue to capture even after that block.
That said, .NET SDK's Close continues to completely close the SDK since it swaps to a NoOp hub instance (the old one would be GC'ed):

https://github.com/getsentry/sentry-dotnet/blob/2d6c847268c9422c0a02bffe87dd656736cf197c/src/Sentry/SentrySdk.cs#L123-L125

@vaind vaind merged commit 9318a96 into main Mar 9, 2022
@vaind vaind deleted the chore/close-sentry-on-quit branch March 9, 2022 22:34
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.

Call native sentry_close() when the app quits
3 participants