-
-
Notifications
You must be signed in to change notification settings - Fork 199
feat(ndk): expose init return code #1430
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -10,7 +10,10 @@ public final class SentryNdk { | |||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| private SentryNdk() {} | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| private static native void initSentryNative(@NotNull final NdkOptions options); | ||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||
| * Initializes sentry-native and returns 0 on success, non-zero on failure. | ||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||
| private static native int initSentryNative(@NotNull final NdkOptions options); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| private static native void shutdown(); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
@@ -19,9 +22,9 @@ private SentryNdk() {} | |||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||
| * @param options the SentryAndroidOptions | ||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||
| public static void init(@NotNull final NdkOptions options) { | ||||||||||||||||||||||||||
| public static int init(@NotNull final NdkOptions options) { | ||||||||||||||||||||||||||
| loadNativeLibraries(); | ||||||||||||||||||||||||||
| initSentryNative(options); | ||||||||||||||||||||||||||
| return initSentryNative(options); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
Comment on lines
+25
to
29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The public 💡 Suggested Fix
Suggested change
Did we get this right? 👍 / 👎 to inform future reviews. |
||||||||||||||||||||||||||
| /** Closes the NDK integration */ | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -247,7 +247,7 @@ static void send_envelope(sentry_envelope_t *envelope, void *data) { | |
| sentry_envelope_free(envelope); | ||
| } | ||
|
|
||
| JNIEXPORT void JNICALL | ||
| JNIEXPORT jint JNICALL | ||
| Java_io_sentry_ndk_SentryNdk_initSentryNative( | ||
| JNIEnv *env, | ||
| jclass cls, | ||
|
|
@@ -355,8 +355,8 @@ Java_io_sentry_ndk_SentryNdk_initSentryNative( | |
| jfloat traces_sample_rate = (jfloat) (*env)->CallFloatMethod(env, sentry_ndk_options, traces_sample_rate_mid); | ||
| sentry_options_set_traces_sample_rate(options, traces_sample_rate); | ||
|
|
||
| sentry_init(options); | ||
| return; | ||
| int rv = sentry_init(options); | ||
| return (jint) rv; | ||
|
|
||
| fail: | ||
| if (!transport_owns_path) { | ||
|
|
@@ -366,6 +366,7 @@ Java_io_sentry_ndk_SentryNdk_initSentryNative( | |
| sentry_transport_free(transport); | ||
| } | ||
| sentry_options_free(options); | ||
| return (jint) -1; | ||
|
Comment on lines
366
to
+369
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The error handling returns -1 for general initialization failures but returns the actual sentry_init() result on success. This creates inconsistent error code semantics. Consider either: (1) documenting the different error code meanings (what specific non-zero values from sentry_init() mean vs -1 for setup failures), or (2) normalizing the return value to always return -1 for any failure and only 0 for success. This will make error handling more predictable for Java callers. 🤖 Prompt for AI Agent Did we get this right? 👍 / 👎 to inform future reviews. |
||
| } | ||
|
|
||
| JNIEXPORT void JNICALL | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep using
voidhere and throw an exception within init if the error code is non-0. This keeps API compatibility and ties the error code handling closer to the actual C code.We also don't need any extra handling within sentry-java then, as it's covered by this try-catch here.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would even go one step further here and check whether the return code was 1 (in which case there is some native-internal issue which could be resolved via logs and there is no need to render a return code) and having an enum range in the negative return code range, that results in an explanatory expection message for each particular return code.
Even if we don't go that far, we should at least differentiate between
> 0and< 0returnCodeexceptions, to clarify where the error happened (in theJNIlayer or native proper). Since the primary failure mode in theJNIlayer is about allocation, most exceptions from there would not be actionable to client code. However, having a basic differentiation between the source of the exception, would still be useful.