ARROW-16329: [Java][C++] Keep more context when marshalling errors through JNI #13246

zhztheplayer · 2022-05-27T02:45:55Z

https://issues.apache.org/jira/browse/ARROW-16329

github-actions · 2022-05-27T02:46:19Z

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on JIRA? https://issues.apache.org/jira/browse/ARROW

Opening JIRAs ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

github-actions · 2022-05-27T02:50:24Z

https://issues.apache.org/jira/browse/ARROW-16329

github-actions · 2022-05-27T02:50:26Z

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

pitrou · 2022-05-30T16:12:25Z

cpp/src/jni/dataset/jni_util.cc

+    std::string description = JStringToCString(
+        env, (jstring)env->CallStaticObjectMethod(describer_class, describe_method, t));
+    return Status::Invalid("Error during calling Java code from native code: " +
+                           description);


Hmm... I don't know what is possible exactly, but ideally we should:

map Java exception classes to specific C++ status codes (for example IOException would map to Status::IOError while InvalidTypeException would map to Status::TypeError)

keep the Java exception object in a Java-specific StatusDetail subclass, just like we do for Python exceptions

cc @lwhite1

Also this helper function should be moved out of dataset-specific code to be reused generically in all JNI bindings.

Also this helper function should be moved out of dataset-specific code to be reused generically in all JNI bindings.

Was considering doing the similar thing but maybe after we have a common code module for JNI stuffs? Which may require some redesign for other parts of JNI codes, orc, gandiva, plasma, etc.

If having a common module for JNI utilities is non-trivial, then can you open a JIRA for it?

@pitrou sure, I have opened a ticket
https://issues.apache.org/jira/browse/ARROW-16691

@zhztheplayer Do you want to address the other parts of the initial comment in this PR?

pitrou · 2022-06-30T12:26:13Z

@lidavidm @lwhite1 Does either of you want to take this up?

lidavidm · 2022-07-01T16:29:56Z

I'm not going to have time. Since Larry is also working on reorganizing the JNI code anyways I'll leave it up to him whether/how to approach this

zhztheplayer · 2022-07-02T16:24:57Z

@pitrou @lidavidm I have updated the PR to address the early comments. Would you like to take a look again? Thanks.

lidavidm · 2022-07-05T13:15:55Z

java/dataset/src/main/java/org/apache/arrow/dataset/jni/JniExceptionDescriber.java

+/**
+ * For native code to invoke to convert a java/lang/Throwable to jstring.
+ */
+public class JniExceptionDescriber {


Does this need to be a public class?

Changed to use package private.

lidavidm · 2022-07-05T13:18:02Z

java/dataset/src/main/cpp/jni_wrapper.cc

@@ -17,14 +17,14 @@

 #include <mutex>

+#include "./jni_util.h"


Suggested change

#include "./jni_util.h"

#include "jni_util.h"

since Kou just fixed this

lidavidm · 2022-07-05T13:20:08Z

java/dataset/src/main/cpp/jni_util.cc

+arrow::Result<bool> IsErrorInstanceOf(JNIEnv* env, jthrowable t, std::string class_name) {
+  jclass jclass = env->FindClass(class_name.c_str());
+  if (jclass == nullptr) {
+    return arrow::Status::Invalid("Class not found: " + class_name);


Given the way this is used below, maybe this should be a DCHECK(jclass) << ...; or just return false?

Is DCHECK suitable for null-checking + returning? Anyway I have changed to return false in latest commit.

Well, DCHECK will abort the program (in a debug build). Presumably it's a bug if we can't find any of these classes/we shouldn't continue execution? But returning false is OK

If it's a bug that we can't find them (i.e. they should always be loaded), I would favour returning an error, or at least emitting a warning message.

I changed to use DCHECK to guard against debug build since class_name is hard coded. Would you please take a look? Thanks.

That seems ok to me, thanks.

lidavidm · 2022-07-05T13:20:27Z

java/dataset/src/main/cpp/jni_util.cc

+    // Reattach current thread to JVM
+    getEnvStat = vm->AttachCurrentThread(reinterpret_cast<void**>(&env), nullptr);
+    if (getEnvStat != JNI_OK) {
+      std::cout << "Failed to attach current thread to JVM. " << std::endl;


ARROW_LOG instead of std::cout?

lidavidm · 2022-07-05T13:20:48Z

java/dataset/src/main/cpp/jni_util.cc

@@ -17,6 +17,7 @@

 #include "./jni_util.h"


Suggested change

#include "./jni_util.h"

#include "jni_util.h"

lidavidm · 2022-07-05T15:35:03Z

java/dataset/src/main/cpp/jni_util.cc

@@ -17,6 +17,7 @@

 #include "jni_util.h"

+#include <iostream>


Unused include now?

lidavidm · 2022-07-05T15:35:46Z

java/dataset/src/main/cpp/jni_util.cc

@@ -162,11 +165,105 @@ std::shared_ptr<ReservationListener> ReservationListenableMemoryPool::get_listen

 ReservationListenableMemoryPool::~ReservationListenableMemoryPool() {}

+std::string Describe(JNIEnv* env, jthrowable t) {
+  jclass describer_class =
+      env->FindClass("org/apache/arrow/dataset/jni/JniExceptionDescriber");


If we're going to check the result of FindClass below, let's also check it here

lidavidm · 2022-07-05T15:36:04Z

java/dataset/src/main/cpp/jni_util.cc

+arrow::StatusCode MapJavaError(JNIEnv* env, jthrowable t) {
+  StatusCode code;
+  if (IsErrorInstanceOf(env, t, "org/apache/arrow/memory/OutOfMemoryException") ==
+      JNI_TRUE) {


nit: the method returns bool so no need for explicit == JNI_TRUE

lidavidm · 2022-07-05T15:38:04Z

java/dataset/src/main/cpp/jni_util.cc

+std::string JavaErrorDetail::ToString() const {
+  JNIEnv* env = GetEnvOrAttach(vm_);
+  if (env == nullptr) {
+    return "Java Exception, ID: " + std::to_string(reinterpret_cast<size_t>(cause_));


cause_ is a pointer, so use uintptr_t?

pitrou · 2022-07-05T15:45:42Z

java/dataset/src/main/cpp/jni_util.h

@@ -91,6 +93,24 @@ void ReleaseNativeRef(jlong ref) {
  delete retrieved_ptr;
 }

+const char kJavaErrorDetailTypeId[] = "arrow::dataset::jni::JavaErrorDetail";


Should this be a static member inside JavaErrorDetail, or perhaps extern and defined in the .cc?
As is, it seems there will be symbol clashes if jni_util.h is included by several .cc files.

I have moved the variable into .cc.

pitrou

I'll let a Java expert do the final approval but this looks basically ok to me (and a very marked improvement IMHO).

github-actions bot added Component: C++ Component: Java labels May 27, 2022

zhztheplayer changed the title ~~Return exception details in arrow::dataset::jni::CheckException~~ ARROW-16329: Return exception details in arrow::dataset::jni::CheckException May 27, 2022

zhztheplayer changed the title ~~ARROW-16329: Return exception details in arrow::dataset::jni::CheckException~~ ARROW-16329: [Java][C++] Keep more context when marshalling errors through JNI May 27, 2022

pitrou reviewed May 30, 2022

View reviewed changes

zhztheplayer force-pushed the ARROW-16329 branch 2 times, most recently from 1004d62 to 24bb7ab Compare July 2, 2022 14:52

lidavidm reviewed Jul 5, 2022

View reviewed changes

zhztheplayer added 4 commits July 5, 2022 21:38

Return exception details in arrow::dataset::jni::CheckException

8c56d7d

style

a5f3d9e

Address comments

1b6835f

Address comments

b544b66

zhztheplayer force-pushed the ARROW-16329 branch from 69fd193 to b544b66 Compare July 5, 2022 13:43

lidavidm reviewed Jul 5, 2022

View reviewed changes

pitrou reviewed Jul 5, 2022

View reviewed changes

Address comments

6acbe0c

zhztheplayer force-pushed the ARROW-16329 branch from c956491 to 6acbe0c Compare July 5, 2022 16:10

pitrou approved these changes Jul 5, 2022

View reviewed changes

lidavidm approved these changes Jul 5, 2022

View reviewed changes

lidavidm merged commit 2a2d01d into apache:master Jul 6, 2022

lwhite1 mentioned this pull request Aug 2, 2022

Version 9.0.0 release blog post apache/arrow-site#227

Merged

ARROW-16329: [Java][C++] Keep more context when marshalling errors through JNI #13246

ARROW-16329: [Java][C++] Keep more context when marshalling errors through JNI #13246

Conversation

zhztheplayer commented May 27, 2022 • edited Loading

github-actions bot commented May 27, 2022

github-actions bot commented May 27, 2022

github-actions bot commented May 27, 2022

pitrou May 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhztheplayer May 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitrou commented Jun 30, 2022

lidavidm commented Jul 1, 2022

zhztheplayer commented Jul 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhztheplayer Jul 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitrou left a comment

Choose a reason for hiding this comment

zhztheplayer commented May 27, 2022 •

edited

Loading

pitrou May 30, 2022 •

edited

Loading

zhztheplayer May 31, 2022 •

edited

Loading

zhztheplayer Jul 5, 2022 •

edited

Loading