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

feat: Managed layer initializes native SDK on Android #1924

Merged
merged 86 commits into from
Dec 13, 2024

Conversation

bitsandfoxes
Copy link
Contributor

@bitsandfoxes bitsandfoxes commented Nov 28, 2024

Android: Android SDK Initialization

Overview

Implements the late initialization of the Android SDK. This allows users to apply code changes to the options and for those options to apply to the native layer. Related issue: sentry-unity#1907

Implementation Details

The SDK now always includes the Android SDK in the generated Gradle project with the following changes:

  1. New Native Support Option
  • Added an enum AndroidNativeInitializationType to the SentryOptions. This allows users to switch between Runtime and BuildTime initialization and defaults to Runtime.
  • The AndroidNativeSupportEnabled controls whether the SDK should add native support. This helps i.e. in the case of UaaL where users want to completely disabled the SDK from modifying the Gradle project.
  1. Build Process Changes
  • Adding the native options to the manifest is now controlled by AndroidNativeInitializationType enum
    • With type Runtime the SDK will add auto-init = false.
    • With type BuildTime it will add the options and let the Android SDK autoinitialize.
  1. Runtime Behavior
  • SentryNativeAndroid.Configure checks whether the native SDK was already initialized
  • Both early and late initialization are going through the same code paths: i.e. setting up scope observer.
  1. JNI Executor Behavior
  • The JNI Executor defaults to timeout after 16ms as to not drop more than a frame
  • Android SDK initialization takes about 25ms on a current device. So the timeout is now overwriteable when calling into the executor.

Pending Tasks

  • RuntimeInitialization is default. This means we're missing testing BuildTime in CI right now.
  • "Init Native First" flow
  • "Init Native Late" flow
  • Note: Requires duplicate mobile smoke testing (2x build, compile, test)

@bitsandfoxes bitsandfoxes force-pushed the feat/android-init-android branch from 0deb527 to ffd8221 Compare December 5, 2024 17:14
@bitsandfoxes bitsandfoxes marked this pull request as ready for review December 5, 2024 17:14
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.

CI seems not to be running

src/Sentry.Unity.Android/SentryJava.cs Outdated Show resolved Hide resolved
src/Sentry.Unity/SentryUnityOptions.cs Show resolved Hide resolved
@bruno-garcia
Copy link
Member

image

Smoke Test still unreliable?

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.

I see 5 new tests for quite a significant change. I'm concerned there are lot of untested use cases. Isn't that the case?

modules/sentry-native Outdated Show resolved Hide resolved
src/Sentry.Unity.Android/SentryJava.cs Show resolved Hide resolved
src/Sentry.Unity.Android/SentryJava.cs Outdated Show resolved Hide resolved
src/Sentry.Unity.Android/SentryNativeAndroid.cs Outdated Show resolved Hide resolved
src/Sentry.Unity.Android/SentryNativeAndroid.cs Outdated Show resolved Hide resolved
src/Sentry.Unity/SentryUnityOptions.cs Outdated Show resolved Hide resolved
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 issue left which is the thread.Join on Dispose which can throw.

Other than that, lgtm

_shutdownSource.Cancel();
_workerThread?.Join(100);
Copy link
Member

Choose a reason for hiding this comment

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

It can throw for:

Exceptions
ThreadStateException
The caller attempted to join a thread that is in the Unstarted state.

ThreadInterruptedException
The thread is interrupted while waiting.

We should catch this here and suppress it

Copy link
Member

Choose a reason for hiding this comment

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

Also worth reading the remarks of these APIs.

Curious why we use Thread directly instead of the Task abstraction. It's by default a background thread and it's much better to deal with cancellation, error handling etc (something for a potential future task, to change this)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Curious why we use Thread directly instead of the Task abstraction.

The executor needs to run all the calls on a single thread. If we converted this to a task, we'd still need to ensure that so no async/await, etc. In that case, I only see risks that we do something wrong here now or in the future version of the code which would lead to the task being rescheduled to a different thread in the pool.

@bitsandfoxes bitsandfoxes merged commit c0681ff into main Dec 13, 2024
94 checks passed
@bitsandfoxes bitsandfoxes deleted the feat/android-init-android branch December 13, 2024 12:56
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.

4 participants