Skip to content
This repository was archived by the owner on Mar 8, 2020. It is now read-only.

Commit 81df993

Browse files
bzzbzz
committed
v2: better error handling
Replace std::runtime_error and in-band error codes with JVM exceptions. Signed-off-by: Alexander Bezzubov <[email protected]>
1 parent 5ec22ca commit 81df993

File tree

3 files changed

+89
-68
lines changed

3 files changed

+89
-68
lines changed

‎src/main/native/jni_utils.cc

Lines changed: 59 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
#include "jni_utils.h"
2+
#include <string>
23

3-
extern JavaVM *jvm; // FIXME(bzz): double-check and document
4+
// TODO(bzz): double-check and document. Suggestion and more context at
5+
// https://github.com/bblfsh/client-scala/pull/84#discussion_r288347756
6+
extern JavaVM *jvm;
47

58
JNIEnv *getJNIEnv() {
69
JNIEnv *pEnv = NULL;
@@ -20,6 +23,8 @@ JNIEnv *getJNIEnv() {
2023
// Class fully qualified names
2124
const char *CLS_NODE = "org/bblfsh/client/v2/Node";
2225
const char *CLS_CTX = "org/bblfsh/client/v2/Context";
26+
const char *CLS_OBJ = "java/lang/Object";
27+
const char *CLS_RE = "java/lang/RuntimeException";
2328

2429
const char *CLS_JNODE = "org/bblfsh/client/v2/JNode";
2530
const char *CLS_JNULL = "org/bblfsh/client/v2/JNull";
@@ -31,6 +36,7 @@ const char *CLS_JUINT = "org/bblfsh/client/v2/JUint";
3136
const char *CLS_JARR = "org/bblfsh/client/v2/JArray";
3237
const char *CLS_JOBJ = "org/bblfsh/client/v2/JObject";
3338

39+
// Method signatures
3440
const char *METHOD_JNODE_KEY_AT = "(I)Ljava/lang/String;";
3541
const char *METHOD_JNODE_VALUE_AT = "(I)Lorg/bblfsh/client/v2/JNode;";
3642
const char *METHOD_JOBJ_ADD =
@@ -39,84 +45,105 @@ const char *METHOD_JOBJ_ADD =
3945
const char *METHOD_JARR_ADD =
4046
"(Lorg/bblfsh/client/v2/JNode;)Lscala/collection/mutable/Buffer;";
4147

48+
const char *METHOD_OBJ_TO_STR = "()Ljava/lang/String;";
49+
50+
void checkJvmException(std::string msg) {
51+
JNIEnv *env = getJNIEnv();
52+
if (env->ExceptionCheck()) {
53+
// env->ExceptionDescribe(); // prints to stdout, un-comment for debuging
54+
auto err = env->ExceptionOccurred();
55+
56+
jmethodID toString = env->GetMethodID(env->FindClass(CLS_OBJ), "toString",
57+
METHOD_OBJ_TO_STR);
58+
jstring s = (jstring)env->CallObjectMethod(err, toString);
59+
const char *utf = env->GetStringUTFChars(s, 0);
60+
env->ReleaseStringUTFChars(s, utf);
61+
62+
env->ExceptionClear();
63+
64+
auto exception = env->FindClass(CLS_RE);
65+
env->ThrowNew(exception, msg.append(": ").append(utf).data());
66+
}
67+
}
68+
4269
jobject NewJavaObject(JNIEnv *env, const char *className, const char *initSign,
4370
...) {
4471
jclass cls = env->FindClass(className);
45-
if (env->ExceptionOccurred() || !cls) {
46-
return nullptr;
47-
}
72+
checkJvmException(std::string("failed to find a class ").append(className));
4873

4974
jmethodID initId = env->GetMethodID(cls, "<init>", initSign);
50-
if (env->ExceptionOccurred() || !initId) {
51-
return nullptr;
52-
}
75+
checkJvmException(
76+
std::string("failed to call <inti> constructor for ").append(className));
5377

5478
va_list varargs;
5579
va_start(varargs, initSign);
5680
jobject instance = env->NewObjectV(cls, initId, varargs);
5781
va_end(varargs);
58-
if (env->ExceptionOccurred() || !instance) {
59-
return nullptr;
60-
}
82+
checkJvmException(
83+
std::string("failed get varargs for constructor of ").append(className));
6184

6285
return instance;
6386
}
6487

6588
jfieldID getField(JNIEnv *env, jobject obj, const char *name) {
6689
jclass cls = env->GetObjectClass(obj);
67-
if (env->ExceptionOccurred() || !cls) {
68-
return nullptr;
69-
}
90+
checkJvmException("failed get an object class");
7091

7192
jfieldID jfid = env->GetFieldID(cls, name, "J");
72-
if (env->ExceptionOccurred() || !jfid) {
73-
return nullptr;
74-
}
93+
checkJvmException(std::string("failed get a field ").append(name));
94+
7595
return jfid;
7696
}
7797

7898
static jmethodID MethodID(JNIEnv *env, const char *method,
7999
const char *signature, const char *className) {
80100
jclass cls = env->FindClass(className);
81-
if (env->ExceptionOccurred() || !cls) {
82-
return nullptr;
83-
}
101+
checkJvmException(std::string("failed to find a class ").append(className));
84102

85103
jmethodID mId = env->GetMethodID(cls, method, signature);
86-
if (env->ExceptionOccurred()) {
87-
return nullptr;
88-
}
104+
checkJvmException(std::string("failed get a method ")
105+
.append(className)
106+
.append(".")
107+
.append(method));
89108

90109
return mId;
91110
}
92111

93112
jint IntMethod(JNIEnv *env, const char *method, const char *signature,
94113
const char *className, const jobject *object) {
95114
jmethodID mId = MethodID(env, method, signature, className);
96-
if (env->ExceptionOccurred() || !mId) return 0;
97-
// TODO(bzz): add better error handling
98-
// ExceptionOccurred()
99-
// ExceptionDescribe()
100-
// ExceptionClear()
101-
// ExceptionCheck()
102-
// ThrowNew("failed to call $className.$method") to JVM
115+
checkJvmException(std::string("failed get a method ")
116+
.append(className)
117+
.append(".")
118+
.append(method));
103119

104120
jint res = env->CallIntMethod(*object, mId);
105-
if (env->ExceptionOccurred()) return 0;
121+
checkJvmException(std::string("failed call a method ")
122+
.append(className)
123+
.append(".")
124+
.append(method)
125+
.append(" using signature ")
126+
.append(signature));
106127

107128
return res;
108129
}
109130

110131
jobject ObjectMethod(JNIEnv *env, const char *method, const char *signature,
111132
const char *className, const jobject *object, ...) {
112133
jmethodID mId = MethodID(env, method, signature, className);
113-
if (env->ExceptionOccurred() || !mId) return nullptr;
134+
checkJvmException(std::string("failed get a method ")
135+
.append(className)
136+
.append(".")
137+
.append(method));
114138

115139
va_list varargs;
116140
va_start(varargs, object);
117141
jobject res = env->CallObjectMethodV(*object, mId, varargs);
118142
va_end(varargs);
119-
if (env->ExceptionOccurred() || !res) return nullptr;
143+
checkJvmException(std::string("failed get varargs for ")
144+
.append(className)
145+
.append(".")
146+
.append(method));
120147

121148
return res;
122149
}

‎src/main/native/jni_utils.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,12 @@
22
#define _Included_org_bblfsh_client_libuast_Libuast_jni_utils
33

44
#include <jni.h>
5+
#include <string>
56

67
extern const char *CLS_NODE;
78
extern const char *CLS_CTX;
9+
extern const char *CLS_OBJ;
10+
extern const char *CLS_RE;
811

912
extern const char *CLS_JNODE;
1013
extern const char *CLS_JNULL;
@@ -21,6 +24,10 @@ extern const char *METHOD_JNODE_VALUE_AT;
2124
extern const char *METHOD_JOBJ_ADD;
2225
extern const char *METHOD_JARR_ADD;
2326

27+
extern const char *METHOD_OBJ_TO_STR;
28+
29+
void checkJvmException(std::string);
30+
2431
JNIEnv *getJNIEnv();
2532
jobject NewJavaObject(JNIEnv *, const char *, const char *, ...);
2633
jfieldID getField(JNIEnv *env, jobject obj, const char *name);

‎src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc

Lines changed: 23 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -189,7 +189,7 @@ class Node : public uast::Node<Node *> {
189189

190190
NodeKind Kind() { return kind; }
191191

192-
// TODO(#90) all 'As*' are unused stubs, make them buletprof and test
192+
// TODO(#90): implement and test (all 'As*' are unused stubs for now)
193193
std::string *AsString() {
194194
if (!str) {
195195
JNIEnv *env = getJNIEnv();
@@ -263,20 +263,18 @@ class Node : public uast::Node<Node *> {
263263
}
264264

265265
void SetValue(size_t i, Node *val) {
266+
JNIEnv *env = getJNIEnv();
266267
jobject v = nullptr;
267268
if (val && val->obj) {
268269
v = val->obj;
269-
} else { // FIXME(bzz)
270-
// v = "new JNull instance"; // Py_None;
270+
} else {
271+
v = NewJavaObject(env, CLS_JNULL, "()V");
271272
}
272273

273-
jobject res =
274-
ObjectMethod(getJNIEnv(), "add", METHOD_JARR_ADD, CLS_JARR, &obj, v);
275-
if (res == nullptr) { // FIXME(bzz) replace by ThrowNew() to JVM
276-
throw std::runtime_error(std::string("failed to call ")
277-
.append(CLS_JARR)
278-
.append(".add() from Node::SetValue()"));
279-
}
274+
ObjectMethod(getJNIEnv(), "add", METHOD_JARR_ADD, CLS_JARR, &obj, v);
275+
checkJvmException(std::string("failed to call ")
276+
.append(CLS_JARR)
277+
.append(".add() from Node::SetValue()"));
280278
}
281279
void SetKeyValue(std::string key, Node *val) {
282280
JNIEnv *env = getJNIEnv();
@@ -291,12 +289,10 @@ class Node : public uast::Node<Node *> {
291289

292290
jobject res =
293291
ObjectMethod(env, "add", METHOD_JOBJ_ADD, CLS_JOBJ, &obj, k, v);
294-
if (res == nullptr) { // FIXME(bzz) replace by ThrowNew() to JVM
295-
throw std::runtime_error(
296-
std::string("failed to call JObject.add() from Node::SetKeyValue(")
297-
.append(key)
298-
.append(")"));
299-
}
292+
checkJvmException(
293+
std::string("failed to call JObject.add() from Node::SetKeyValue(")
294+
.append(key)
295+
.append(")"));
300296
}
301297
};
302298

@@ -435,21 +431,16 @@ class Context {
435431
JNIEnv *env = getJNIEnv();
436432

437433
ContextExt *nodeExtCtx = getHandle<ContextExt>(env, src, "ctx");
438-
if (!nodeExtCtx) {
439-
throw std::runtime_error("Cannot get Node.ctx");
440-
}
434+
checkJvmException("failed to get Node.ctx");
435+
441436
auto sctx = nodeExtCtx->ctx;
442437
NodeHandle snode =
443438
reinterpret_cast<NodeHandle>(getHandle<NodeHandle>(env, src, "handle"));
444-
if (!snode) {
445-
throw std::runtime_error("Cannot get Node.handle");
446-
}
439+
checkJvmException("failed to get Node.handle");
447440

448441
Node *node = uast::Load(sctx, snode, ctx);
449-
if (!node) {
450-
throw std::runtime_error("Failed to uast::Load()");
451-
}
452-
return toJ(node); // new ref
442+
checkJvmException("failed to uast::Load()");
443+
return toJ(node);
453444
}
454445
};
455446

@@ -465,13 +456,10 @@ JNIEXPORT jobject JNICALL Java_org_bblfsh_client_v2_libuast_Libuast_decode(
465456

466457
// works only with ByteBuffer.allocateDirect()
467458
void *buf = env->GetDirectBufferAddress(directBuf);
468-
if (env->ExceptionCheck() == JNI_TRUE) {
469-
return nullptr;
470-
}
459+
checkJvmException("failed to use buffer for direct access");
460+
471461
jlong len = env->GetDirectBufferCapacity(directBuf);
472-
if (env->ExceptionCheck() == JNI_TRUE) {
473-
return nullptr;
474-
}
462+
checkJvmException("failed to get buffer capacity");
475463

476464
// another option (instead of XXX) is to use
477465
// GetPrimitiveArrayCritical
@@ -482,12 +470,11 @@ JNIEXPORT jobject JNICALL Java_org_bblfsh_client_v2_libuast_Libuast_decode(
482470
auto p = new ContextExt(ctx);
483471

484472
jobject jCtxExt = NewJavaObject(env, CLS_CTX, "(J)V", p);
485-
if (env->ExceptionCheck() == JNI_TRUE || !jCtxExt) {
473+
if (env->ExceptionCheck() || !jCtxExt) {
486474
jCtxExt = nullptr;
487475
delete (ctx);
488476
delete (p);
489-
env->ExceptionDescribe();
490-
throw std::runtime_error("failed to instantiate Context class");
477+
checkJvmException("failed to instantiate Context class");
491478
}
492479

493480
return jCtxExt;
@@ -547,7 +534,7 @@ JNIEXPORT jobject JNICALL Java_org_bblfsh_client_v2_Node_load(JNIEnv *env,
547534
JNIEXPORT jint JNI_OnLoad(JavaVM *vm, void *reserved) {
548535
JNIEnv *env;
549536
if (vm->GetEnv(reinterpret_cast<void **>(&env), JNI_VERSION_1_8) != JNI_OK) {
550-
return -1;
537+
return JNI_ERR;
551538
}
552539
jvm = vm;
553540

0 commit comments

Comments
 (0)