Skip to content

Commit f635e0b

Browse files
author
Sahil Takiar
committed
HDFS-14304: Fixing PR comments
* Fixed double-locking issue * Internalize jclass initialization inside initCachedClasses
1 parent 913339d commit f635e0b

File tree

4 files changed

+79
-76
lines changed

4 files changed

+79
-76
lines changed

‎hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfs/hdfs.c‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2335,7 +2335,7 @@ int hdfsChmod(hdfsFS fs, const char *path, short mode)
23352335
if (jthr) {
23362336
ret = printExceptionAndFree(env, jthr, PRINT_EXC_ALL,
23372337
"constructNewObjectOfCachedClass(%s)", HADOOP_FSPERM);
2338-
return -1;
2338+
goto done;
23392339
}
23402340

23412341
//Create an object of org.apache.hadoop.fs.Path
@@ -2584,6 +2584,9 @@ static jthrowable hadoopRzOptionsGetEnumSet(JNIEnv *env,
25842584
}
25852585
jthr = invokeMethod(env, &jVal, STATIC, NULL, JC_ENUM_SET,
25862586
"noneOf", "(Ljava/lang/Class;)Ljava/util/EnumSet;", clazz);
2587+
if (jthr) {
2588+
goto done;
2589+
}
25872590
enumSetObj = jVal.l;
25882591
}
25892592
// create global ref

‎hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfs/jclasses.c‎

Lines changed: 64 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -16,25 +16,30 @@
1616
* limitations under the License.
1717
*/
1818

19+
#include "exception.h"
1920
#include "jclasses.h"
2021
#include "jni_helper.h"
21-
#include "exception.h"
22+
#include "os/mutexes.h"
2223

2324
#include <assert.h>
2425

25-
int jclassesInitialized = 0;
26+
/**
27+
* Whether initCachedClasses has been called or not. Protected by the mutex
28+
* jclassInitMutex.
29+
*/
30+
static int jclassesInitialized = 0;
2631

27-
struct javaClassAndName {
32+
typedef struct {
2833
jclass javaClass;
2934
const char *className;
30-
};
35+
} javaClassAndName;
3136

3237
/**
3338
* A collection of commonly used jclass objects that are used throughout
3439
* libhdfs. The jclasses are loaded immediately after the JVM is created (see
3540
* initCachedClasses). The array is indexed using CachedJavaClass.
3641
*/
37-
struct javaClassAndName cachedJavaClasses[NUM_CACHED_CLASSES];
42+
javaClassAndName cachedJavaClasses[NUM_CACHED_CLASSES];
3843

3944
/**
4045
* Helper method that creates and sets a jclass object given a class name.
@@ -62,57 +67,63 @@ static jthrowable initCachedClass(JNIEnv *env, const char *className,
6267
}
6368

6469
jthrowable initCachedClasses(JNIEnv* env) {
65-
// Set all the class names
66-
cachedJavaClasses[JC_CONFIGURATION].className =
67-
"org/apache/hadoop/conf/Configuration";
68-
cachedJavaClasses[JC_PATH].className =
69-
"org/apache/hadoop/fs/Path";
70-
cachedJavaClasses[JC_FILE_SYSTEM].className =
71-
"org/apache/hadoop/fs/FileSystem";
72-
cachedJavaClasses[JC_FS_STATUS].className =
73-
"org/apache/hadoop/fs/FsStatus";
74-
cachedJavaClasses[JC_FILE_UTIL].className =
75-
"org/apache/hadoop/fs/FileUtil";
76-
cachedJavaClasses[JC_BLOCK_LOCATION].className =
77-
"org/apache/hadoop/fs/BlockLocation";
78-
cachedJavaClasses[JC_DFS_HEDGED_READ_METRICS].className =
79-
"org/apache/hadoop/hdfs/DFSHedgedReadMetrics";
80-
cachedJavaClasses[JC_DISTRIBUTED_FILE_SYSTEM].className =
81-
"org/apache/hadoop/hdfs/DistributedFileSystem";
82-
cachedJavaClasses[JC_FS_DATA_INPUT_STREAM].className =
83-
"org/apache/hadoop/fs/FSDataInputStream";
84-
cachedJavaClasses[JC_FS_DATA_OUTPUT_STREAM].className =
85-
"org/apache/hadoop/fs/FSDataOutputStream";
86-
cachedJavaClasses[JC_FILE_STATUS].className =
87-
"org/apache/hadoop/fs/FileStatus";
88-
cachedJavaClasses[JC_FS_PERMISSION].className =
89-
"org/apache/hadoop/fs/permission/FsPermission";
90-
cachedJavaClasses[JC_READ_STATISTICS].className =
91-
"org/apache/hadoop/hdfs/ReadStatistics";
92-
cachedJavaClasses[JC_HDFS_DATA_INPUT_STREAM].className =
93-
"org/apache/hadoop/hdfs/client/HdfsDataInputStream";
94-
cachedJavaClasses[JC_DOMAIN_SOCKET].className =
95-
"org/apache/hadoop/net/unix/DomainSocket";
96-
cachedJavaClasses[JC_URI].className =
97-
"java/net/URI";
98-
cachedJavaClasses[JC_BYTE_BUFFER].className =
99-
"java/nio/ByteBuffer";
100-
cachedJavaClasses[JC_ENUM_SET].className =
101-
"java/util/EnumSet";
102-
cachedJavaClasses[JC_EXCEPTION_UTILS].className =
103-
"org/apache/commons/lang3/exception/ExceptionUtils";
70+
mutexLock(&jclassInitMutex);
71+
if (!jclassesInitialized) {
72+
// Set all the class names
73+
cachedJavaClasses[JC_CONFIGURATION].className =
74+
"org/apache/hadoop/conf/Configuration";
75+
cachedJavaClasses[JC_PATH].className =
76+
"org/apache/hadoop/fs/Path";
77+
cachedJavaClasses[JC_FILE_SYSTEM].className =
78+
"org/apache/hadoop/fs/FileSystem";
79+
cachedJavaClasses[JC_FS_STATUS].className =
80+
"org/apache/hadoop/fs/FsStatus";
81+
cachedJavaClasses[JC_FILE_UTIL].className =
82+
"org/apache/hadoop/fs/FileUtil";
83+
cachedJavaClasses[JC_BLOCK_LOCATION].className =
84+
"org/apache/hadoop/fs/BlockLocation";
85+
cachedJavaClasses[JC_DFS_HEDGED_READ_METRICS].className =
86+
"org/apache/hadoop/hdfs/DFSHedgedReadMetrics";
87+
cachedJavaClasses[JC_DISTRIBUTED_FILE_SYSTEM].className =
88+
"org/apache/hadoop/hdfs/DistributedFileSystem";
89+
cachedJavaClasses[JC_FS_DATA_INPUT_STREAM].className =
90+
"org/apache/hadoop/fs/FSDataInputStream";
91+
cachedJavaClasses[JC_FS_DATA_OUTPUT_STREAM].className =
92+
"org/apache/hadoop/fs/FSDataOutputStream";
93+
cachedJavaClasses[JC_FILE_STATUS].className =
94+
"org/apache/hadoop/fs/FileStatus";
95+
cachedJavaClasses[JC_FS_PERMISSION].className =
96+
"org/apache/hadoop/fs/permission/FsPermission";
97+
cachedJavaClasses[JC_READ_STATISTICS].className =
98+
"org/apache/hadoop/hdfs/ReadStatistics";
99+
cachedJavaClasses[JC_HDFS_DATA_INPUT_STREAM].className =
100+
"org/apache/hadoop/hdfs/client/HdfsDataInputStream";
101+
cachedJavaClasses[JC_DOMAIN_SOCKET].className =
102+
"org/apache/hadoop/net/unix/DomainSocket";
103+
cachedJavaClasses[JC_URI].className =
104+
"java/net/URI";
105+
cachedJavaClasses[JC_BYTE_BUFFER].className =
106+
"java/nio/ByteBuffer";
107+
cachedJavaClasses[JC_ENUM_SET].className =
108+
"java/util/EnumSet";
109+
cachedJavaClasses[JC_EXCEPTION_UTILS].className =
110+
"org/apache/commons/lang3/exception/ExceptionUtils";
104111

105-
// Create and set the jclass objects based on the class names set above
106-
jthrowable jthr;
107-
int numCachedClasses =
108-
sizeof(cachedJavaClasses) / sizeof(struct javaClassAndName);
109-
for (int i = 0; i < numCachedClasses; i++) {
110-
jthr = initCachedClass(env, cachedJavaClasses[i].className,
111-
&cachedJavaClasses[i].javaClass);
112-
if (jthr) {
113-
return jthr;
112+
// Create and set the jclass objects based on the class names set above
113+
jthrowable jthr;
114+
int numCachedClasses =
115+
sizeof(cachedJavaClasses) / sizeof(javaClassAndName);
116+
for (int i = 0; i < numCachedClasses; i++) {
117+
jthr = initCachedClass(env, cachedJavaClasses[i].className,
118+
&cachedJavaClasses[i].javaClass);
119+
if (jthr) {
120+
mutexUnlock(&jclassInitMutex);
121+
return jthr;
122+
}
114123
}
124+
jclassesInitialized = 1;
115125
}
126+
mutexUnlock(&jclassInitMutex);
116127
return NULL;
117128
}
118129

‎hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfs/jclasses.h‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -64,13 +64,8 @@ typedef enum {
6464
} CachedJavaClass;
6565

6666
/**
67-
* Whether initCachedClasses has been called or not. Protected by the mutex
68-
* jclassInitMutex.
69-
*/
70-
extern int jclassesInitialized;
71-
72-
/**
73-
* Internally initializes all jclass objects listed in the CachedJavaClass enum.
67+
* Internally initializes all jclass objects listed in the CachedJavaClass
68+
* enum. This method is idempotent and thread-safe.
7469
*/
7570
jthrowable initCachedClasses(JNIEnv* env);
7671

‎hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfs/jni_helper.c‎

Lines changed: 9 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,9 @@
1616
* limitations under the License.
1717
*/
1818

19-
#include "jclasses.h"
2019
#include "config.h"
2120
#include "exception.h"
21+
#include "jclasses.h"
2222
#include "jni_helper.h"
2323
#include "platform.h"
2424
#include "os/mutexes.h"
@@ -266,8 +266,8 @@ jthrowable constructNewObjectOfClass(JNIEnv *env, jobject *out,
266266
va_start(args, ctorSignature);
267267
jthr = constructNewObjectOfJclass(env, out, cls, className,
268268
ctorSignature, args);
269-
done:
270269
va_end(args);
270+
done:
271271
destroyLocalReference(env, cls);
272272
return jthr;
273273
}
@@ -551,18 +551,12 @@ JNIEnv* getJNIEnv(void)
551551
state->env = getGlobalJNIEnv();
552552
mutexUnlock(&jvmMutex);
553553

554-
if (!jclassesInitialized) {
555-
mutexLock(&jclassInitMutex);
556-
if (!jclassesInitialized) {
557-
jthrowable jthr = NULL;
558-
jthr = initCachedClasses(state->env);
559-
if (jthr) {
560-
printExceptionAndFree(state->env, jthr, PRINT_EXC_ALL,
561-
"initCachedClasses failed");
562-
return NULL;
563-
}
564-
jclassesInitialized = 1;
565-
}
554+
jthrowable jthr = NULL;
555+
jthr = initCachedClasses(state->env);
556+
if (jthr) {
557+
printExceptionAndFree(state->env, jthr, PRINT_EXC_ALL,
558+
"initCachedClasses failed");
559+
goto fail;
566560
}
567561

568562
if (!state->env) {
@@ -654,7 +648,7 @@ jthrowable hadoopConfSetStr(JNIEnv *env, jobject jConfiguration,
654648
if (jthr)
655649
goto done;
656650
jthr = invokeMethod(env, NULL, INSTANCE, jConfiguration,
657-
JC_CONFIGURATION, "set","(Ljava/lang/String;Ljava/lang/String;)V",
651+
JC_CONFIGURATION, "set", "(Ljava/lang/String;Ljava/lang/String;)V",
658652
jkey, jvalue);
659653
if (jthr)
660654
goto done;

0 commit comments

Comments
 (0)