Skip to content

Don't discard thrown exception #1651

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

xxDark
Copy link

@xxDark xxDark commented Jan 10, 2025

Attach pending exception as the cause in throwByName if possible. This change makes it easier to debug errors.

Copy link
Member

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

Sorry for the late review. In general the idea makes sense. I left an inline comment, that shows how a simpler version could look from my POV.

What is crucially missing is a unittest, that verifies that this works. Please add a demonstrator to validate.

Comment on lines 426 to 469
if (cls != NULL) { /* Otherwise an exception has already been thrown */
if (thrown != NULL) {
/* Attach thrown exception if possible. */
/* do_throw_clean is an exit path that will delete 'thrown' if we can't do that. */
jmethodID init = (*env)->GetMethodID(env, cls, "<init>", "(Ljava/lang/String;)V");
if (init == NULL) {
goto do_throw_clean;
}
jmethodID initCause = (*env)->GetMethodID(env, cls, "initCause", "(Ljava/lang/Throwable;)Ljava/lang/Throwable;");
if (initCause == NULL) {
goto do_throw_clean;
}
jobject str = (*env)->NewStringUTF(env, msg);
if (str == NULL) {
goto do_throw_clean;
}
jthrowable t = (jthrowable) (*env)->NewObject(env, cls, init, str);
(*env)->DeleteLocalRef(env, str);
if (t == NULL) {
goto do_throw_clean;
}
jobject ret = (*env)->CallObjectMethod(env, t, initCause, thrown);
if (ret == NULL) {
goto do_throw_clean;
}
(*env)->DeleteLocalRef(env, ret);
(*env)->Throw(env, t);
goto clean_cls;
} else {
goto do_throw;
}
/* Deletes thrown exception reference and clears the exception. */
do_throw_clean:
(*env)->DeleteLocalRef(env, thrown);
(*env)->ExceptionClear(env);
do_throw:
(*env)->ThrowNew(env, cls, msg);

clean_cls:
/* It's a good practice to clean up the local references. */
(*env)->DeleteLocalRef(env, cls);
} else if (thrown != NULL) {
(*env)->DeleteLocalRef(env, thrown);
}
Copy link
Member

Choose a reason for hiding this comment

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

I think the cases are to complex and can be simplified to:

Suggested change
if (cls != NULL) { /* Otherwise an exception has already been thrown */
if (thrown != NULL) {
/* Attach thrown exception if possible. */
/* do_throw_clean is an exit path that will delete 'thrown' if we can't do that. */
jmethodID init = (*env)->GetMethodID(env, cls, "<init>", "(Ljava/lang/String;)V");
if (init == NULL) {
goto do_throw_clean;
}
jmethodID initCause = (*env)->GetMethodID(env, cls, "initCause", "(Ljava/lang/Throwable;)Ljava/lang/Throwable;");
if (initCause == NULL) {
goto do_throw_clean;
}
jobject str = (*env)->NewStringUTF(env, msg);
if (str == NULL) {
goto do_throw_clean;
}
jthrowable t = (jthrowable) (*env)->NewObject(env, cls, init, str);
(*env)->DeleteLocalRef(env, str);
if (t == NULL) {
goto do_throw_clean;
}
jobject ret = (*env)->CallObjectMethod(env, t, initCause, thrown);
if (ret == NULL) {
goto do_throw_clean;
}
(*env)->DeleteLocalRef(env, ret);
(*env)->Throw(env, t);
goto clean_cls;
} else {
goto do_throw;
}
/* Deletes thrown exception reference and clears the exception. */
do_throw_clean:
(*env)->DeleteLocalRef(env, thrown);
(*env)->ExceptionClear(env);
do_throw:
(*env)->ThrowNew(env, cls, msg);
clean_cls:
/* It's a good practice to clean up the local references. */
(*env)->DeleteLocalRef(env, cls);
} else if (thrown != NULL) {
(*env)->DeleteLocalRef(env, thrown);
}
if (cls != NULL) { /* Otherwise an exception has already been thrown */
jthrowable t = NULL;
jmethodID init = (*env)->GetMethodID(env, cls, "<init>", "(Ljava/lang/String;)V");
jobject str = (*env)->NewStringUTF(env, msg);
if (init != NULL && str != NULL) {
t = (jthrowable) (*env)->NewObject(env, cls, init, str);
}
if (str != NULL) {
(*env)->DeleteLocalRef(env, str);
}
if (thrown != NULL && t != NULL) {
jmethodID initCause = (*env)->GetMethodID(env, cls, "initCause", "(Ljava/lang/Throwable;)Ljava/lang/Throwable;");
/* Attach thrown exception if possible. */
jobject ret = (*env)->CallObjectMethod(env, t, initCause, thrown);
(*env)->DeleteLocalRef(env, ret);
}
(*env)->Throw(env, t);
(*env)->DeleteLocalRef(env, cls);
}
if (thrown != NULL) {
(*env)->DeleteLocalRef(env, thrown);
}

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.

2 participants