Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-799

libhdfs must call DetachCurrentThread when a thread is destroyed

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.0-alpha
    • Fix Version/s: 2.0.2-alpha
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      Threads that call AttachCurrentThread in libhdfs and disappear without calling DetachCurrentThread cause a memory leak.
      Libhdfs should detach the current thread when this thread exits.

      1. HDFS-799.001.patch
        5 kB
        Colin Patrick McCabe
      2. HDFS-799.003.patch
        5 kB
        Colin Patrick McCabe
      3. HDFS-799.004.patch
        8 kB
        Colin Patrick McCabe
      4. HDFS-799.005.patch
        8 kB
        Colin Patrick McCabe
      5. HDFS-799.007.patch
        0.6 kB
        Colin Patrick McCabe

        Issue Links

          Activity

          Hide
          Bin added a comment -

          Hi, we also found same memory leak when using libhdfs. Any progress for this issue?
          It is hard to determine when to call method DetachCurrentThread since JNIEnv is shared. Calling DetachCurrentThread in hdfsDisconnect is not enough. Additonal method in hdfs.h is needed.

          Show
          Bin added a comment - Hi, we also found same memory leak when using libhdfs. Any progress for this issue? It is hard to determine when to call method DetachCurrentThread since JNIEnv is shared. Calling DetachCurrentThread in hdfsDisconnect is not enough. Additonal method in hdfs.h is needed.
          Hide
          Colin Patrick McCabe added a comment -

          This can be accomplished by using the pthread thread-local-storage interface coupled with the "optional destructor function." See http://www.manpagez.com/man/3/pthread_key_create/

          Show
          Colin Patrick McCabe added a comment - This can be accomplished by using the pthread thread-local-storage interface coupled with the "optional destructor function." See http://www.manpagez.com/man/3/pthread_key_create/
          Hide
          Colin Patrick McCabe added a comment -

          Note that the other nice thing about this solution is that it should speed things up a little bit, by eliminating the need to take a mutex in GetVM.

          Show
          Colin Patrick McCabe added a comment - Note that the other nice thing about this solution is that it should speed things up a little bit, by eliminating the need to take a mutex in GetVM.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12535303/HDFS-799.001.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2748//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2748//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12535303/HDFS-799.001.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2748//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2748//console This message is automatically generated.
          Hide
          Eli Collins added a comment -

          Patch looks good, testing?

          Show
          Eli Collins added a comment - Patch looks good, testing?
          Hide
          Colin Patrick McCabe added a comment -
          • Don't use _fini, since it may be called while threads are still active after an exit().
          Show
          Colin Patrick McCabe added a comment - Don't use _fini, since it may be called while threads are still active after an exit().
          Hide
          Colin Patrick McCabe added a comment -

          Patch looks good, testing?

          I tested it by adding printfs to the tls initialization and destruction functions, and verifying that they did what was required. More thorough testing will be provided by HDFS-3606, once it goes in.

          Show
          Colin Patrick McCabe added a comment - Patch looks good, testing? I tested it by adding printfs to the tls initialization and destruction functions, and verifying that they did what was required. More thorough testing will be provided by HDFS-3606 , once it goes in.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12535883/HDFS-799.003.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2773//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12535883/HDFS-799.003.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2773//console This message is automatically generated.
          Hide
          Colin Patrick McCabe added a comment -

          resubmitting the same patch now that Jenkins has been fixed (by HADOOP-8584)

          Show
          Colin Patrick McCabe added a comment - resubmitting the same patch now that Jenkins has been fixed (by HADOOP-8584 )
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12535883/HDFS-799.003.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2794//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2794//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12535883/HDFS-799.003.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2794//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2794//console This message is automatically generated.
          Hide
          Andy Isaacson added a comment -
          -/**
          - * getJNIEnv: A helper function to get the JNIEnv* for the given thread.
          - * If no JVM exists, then one will be created. JVM command line arguments
          - * are obtained from the LIBHDFS_OPTS environment variable.
          - *
          - * @param: None.
          - * @return The JNIEnv* corresponding to the thread.
          - */
          -JNIEnv* getJNIEnv(void)
          +JNIEnv* getGlobalJNIEnv(void)
           {
          

          we need a new function comment here. ... ah I see below that it's now a helper; make it static? Then you don't need to add a function comment.

          
          @@ -397,12 +420,12 @@ JNIEnv* getJNIEnv(void)
           
               // Only the first thread should create the JVM. The other trheads should
               // just use the JVM created by the first thread.
          -    LOCK_JVM_MUTEX();
          +    pthread_mutex_lock(&jvmMutex);
          

          Even though this is the only callsite, the macro performs a nice documentary function. I don't think there's much benefit to unmacroizing this code.

          +    tls = calloc(1, sizeof(struct hdfsTls));
          +    if (!tls) {
          +        fprintf(stderr, "getJNIEnv: OOM\n");
          

          OOM => "calloc(%d) failed" or similar.

          +static void _init()
          +{
          +    pthread_key_create(&gTlsKey, tlsDestructor);
          +}
          

          pthread_key_create can fail, we should do something useful in that case. (I don't know offhand if we are allowed to fprintf in an _attribute_((constructor)) function...)

          Other than that, LGTM. The fundamental idea is sound and does resolve the problem.

          Does this add any additional compiler/linker dependencies? I know I've had cases in the past of constructors and destructors not getting called due to link flag badness and/or ld bugs. I think the existing code will work (with the memory leak you pointed out) on any JNI + pthreads implementation; the _attribute_ stuff is a GCC extension. Which, admittedly, is implemented by most compilers nowadays, but it's still a potential issue. (I pity the fool who has to get this working with xlc on AIX.)

          Show
          Andy Isaacson added a comment - -/** - * getJNIEnv: A helper function to get the JNIEnv* for the given thread. - * If no JVM exists, then one will be created. JVM command line arguments - * are obtained from the LIBHDFS_OPTS environment variable. - * - * @param: None. - * @ return The JNIEnv* corresponding to the thread. - */ -JNIEnv* getJNIEnv(void) +JNIEnv* getGlobalJNIEnv(void) { we need a new function comment here. ... ah I see below that it's now a helper; make it static? Then you don't need to add a function comment. @@ -397,12 +420,12 @@ JNIEnv* getJNIEnv(void) // Only the first thread should create the JVM. The other trheads should // just use the JVM created by the first thread. - LOCK_JVM_MUTEX(); + pthread_mutex_lock(&jvmMutex); Even though this is the only callsite, the macro performs a nice documentary function. I don't think there's much benefit to unmacroizing this code. + tls = calloc(1, sizeof(struct hdfsTls)); + if (!tls) { + fprintf(stderr, "getJNIEnv: OOM\n" ); OOM => "calloc(%d) failed" or similar. + static void _init() +{ + pthread_key_create(&gTlsKey, tlsDestructor); +} pthread_key_create can fail, we should do something useful in that case. (I don't know offhand if we are allowed to fprintf in an _ attribute _((constructor)) function...) Other than that, LGTM. The fundamental idea is sound and does resolve the problem. Does this add any additional compiler/linker dependencies? I know I've had cases in the past of constructors and destructors not getting called due to link flag badness and/or ld bugs. I think the existing code will work (with the memory leak you pointed out) on any JNI + pthreads implementation; the _ attribute _ stuff is a GCC extension. Which, admittedly, is implemented by most compilers nowadays, but it's still a potential issue. (I pity the fool who has to get this working with xlc on AIX.)
          Hide
          Colin Patrick McCabe added a comment -

          we need a new function comment here. ... ah I see below that it's now a helper; make it static? Then you don't need to add a function comment.

          Yeah, the comment should be updated with the new name of the function. Other than that, it all still applies just as before.

          the [LOCK_JVM_MUTEX] macro performs a nice documentary function...

          I think it obfuscates the source, because I search for the mutex, and don't find any uses...

          _attribute_((constructor)) concerns

          I'll see if I can come up with something....

          Show
          Colin Patrick McCabe added a comment - we need a new function comment here. ... ah I see below that it's now a helper; make it static? Then you don't need to add a function comment. Yeah, the comment should be updated with the new name of the function. Other than that, it all still applies just as before. the [LOCK_JVM_MUTEX] macro performs a nice documentary function... I think it obfuscates the source, because I search for the mutex, and don't find any uses... _ attribute _((constructor)) concerns I'll see if I can come up with something....
          Hide
          Colin Patrick McCabe added a comment -

          Here is an approach which doesn't rely on _attribute_((constructor)). It avoids taking the mutex all the time if __thread is implemented, which it is on most modern systems. If not, it falls back to taking the mutex and checking to see if the key has been initialized yet.

          Show
          Colin Patrick McCabe added a comment - Here is an approach which doesn't rely on _ attribute _((constructor)). It avoids taking the mutex all the time if __thread is implemented, which it is on most modern systems. If not, it falls back to taking the mutex and checking to see if the key has been initialized yet.
          Hide
          Colin Patrick McCabe added a comment -
          • quickTls doesn't have to be global
          Show
          Colin Patrick McCabe added a comment - quickTls doesn't have to be global
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12536168/HDFS-799.004.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.server.namenode.TestBackupNode
          org.apache.hadoop.hdfs.server.common.TestJspHelper
          org.apache.hadoop.hdfs.TestFileAppend4
          org.apache.hadoop.hdfs.server.blockmanagement.TestBlocksWithNotEnoughRacks

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2803//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2803//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12536168/HDFS-799.004.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.server.namenode.TestBackupNode org.apache.hadoop.hdfs.server.common.TestJspHelper org.apache.hadoop.hdfs.TestFileAppend4 org.apache.hadoop.hdfs.server.blockmanagement.TestBlocksWithNotEnoughRacks +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2803//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2803//console This message is automatically generated.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12536171/HDFS-799.005.patch
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no new tests are needed for this patch.
          Also please list what manual steps were performed to verify this patch.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.hdfs.TestDatanodeBlockScanner

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2804//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2804//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12536171/HDFS-799.005.patch against trunk revision . +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests in hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.hdfs.TestDatanodeBlockScanner +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2804//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2804//console This message is automatically generated.
          Hide
          Eli Collins added a comment -

          +1 (test failure is unrelated).

          Show
          Eli Collins added a comment - +1 (test failure is unrelated).
          Hide
          Eli Collins added a comment -

          I've committed this and merged to branch-2. Thanks Colin.

          Show
          Eli Collins added a comment - I've committed this and merged to branch-2. Thanks Colin.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #2459 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2459/)
          HDFS-799. libhdfs must call DetachCurrentThread when a thread is destroyed. Contributed by Colin Patrick McCabe (Revision 1361008)

          Result = SUCCESS
          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361008
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/CMakeLists.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/config.h.cmake
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfsJniHelper.c
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #2459 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2459/ ) HDFS-799 . libhdfs must call DetachCurrentThread when a thread is destroyed. Contributed by Colin Patrick McCabe (Revision 1361008) Result = SUCCESS eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361008 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/CMakeLists.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/config.h.cmake /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfsJniHelper.c
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #2525 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2525/)
          HDFS-799. libhdfs must call DetachCurrentThread when a thread is destroyed. Contributed by Colin Patrick McCabe (Revision 1361008)

          Result = SUCCESS
          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361008
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/CMakeLists.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/config.h.cmake
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfsJniHelper.c
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #2525 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2525/ ) HDFS-799 . libhdfs must call DetachCurrentThread when a thread is destroyed. Contributed by Colin Patrick McCabe (Revision 1361008) Result = SUCCESS eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361008 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/CMakeLists.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/config.h.cmake /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfsJniHelper.c
          Hide
          Andy Isaacson added a comment -
          +    JNIEnv *env = tls->env;
          +    jint ret;
          +
          +    if (!tls)
          +        return;
          

          we've already dereferenced tls, so if it's NULL we segfaulted.

          +    pthread_mutex_lock(&jvmMutex);
          +    if (!gTlsKeyInitialized) {
          +        ret = pthread_key_create(&gTlsKey, hdfsThreadDestructor);
          +        if (ret) {
          +            pthread_mutex_unlock(&jvmMutex);
          +            fprintf("pthread_key_create failed with error %d\n", ret);
          +            return NULL;
          +        }
          

          If pthread_key_create fails, we return NULL. But we could fall back to the slow path of calling getGlobalJNIEnv() every time. Since PTHREAD_KEYS_MAX is only 1024, we probably should do so. (Note that's not a limit on the number of threads, but rather on the number of keys allocated, so it's not terribly likely to hit, but can do.)

          Show
          Andy Isaacson added a comment - + JNIEnv *env = tls->env; + jint ret; + + if (!tls) + return ; we've already dereferenced tls , so if it's NULL we segfaulted. + pthread_mutex_lock(&jvmMutex); + if (!gTlsKeyInitialized) { + ret = pthread_key_create(&gTlsKey, hdfsThreadDestructor); + if (ret) { + pthread_mutex_unlock(&jvmMutex); + fprintf( "pthread_key_create failed with error %d\n" , ret); + return NULL; + } If pthread_key_create fails, we return NULL. But we could fall back to the slow path of calling getGlobalJNIEnv() every time. Since PTHREAD_KEYS_MAX is only 1024, we probably should do so. (Note that's not a limit on the number of threads, but rather on the number of keys allocated, so it's not terribly likely to hit, but can do.)
          Hide
          Colin Patrick McCabe added a comment -

          [code example]: we've already dereferenced tls, so if it's NULL we segfaulted.

          Good catch. I think the fix is just to remove that if statement, since tls will never be NULL in that case.

          If pthread_key_create fails, we return NULL. But we could fall back to the slow path of calling getGlobalJNIEnv() every time. Since PTHREAD_KEYS_MAX is only 1024, we probably should do so. (Note that's not a limit on the number of threads, but rather on the number of keys allocated, so it's not terribly likely to hit, but can do.)

          It would be a memory leak, since then we'd have no way to release the memory when the thread exits. It's better to fail fast and let people change their code to not use so many POSIX thread-local variables than to silently do the wrong thing.

          Show
          Colin Patrick McCabe added a comment - [code example] : we've already dereferenced tls, so if it's NULL we segfaulted. Good catch. I think the fix is just to remove that if statement, since tls will never be NULL in that case. If pthread_key_create fails, we return NULL. But we could fall back to the slow path of calling getGlobalJNIEnv() every time. Since PTHREAD_KEYS_MAX is only 1024, we probably should do so. (Note that's not a limit on the number of threads, but rather on the number of keys allocated, so it's not terribly likely to hit, but can do.) It would be a memory leak, since then we'd have no way to release the memory when the thread exits. It's better to fail fast and let people change their code to not use so many POSIX thread-local variables than to silently do the wrong thing.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #2479 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2479/)
          HDFS-799. libhdfs must call DetachCurrentThread when a thread is destroyed. Contributed by Colin Patrick McCabe (Revision 1361008)

          Result = FAILURE
          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361008
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/CMakeLists.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/config.h.cmake
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfsJniHelper.c
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #2479 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2479/ ) HDFS-799 . libhdfs must call DetachCurrentThread when a thread is destroyed. Contributed by Colin Patrick McCabe (Revision 1361008) Result = FAILURE eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361008 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/CMakeLists.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/config.h.cmake /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfsJniHelper.c
          Hide
          Colin Patrick McCabe added a comment -
          • remove unnecessary if statement
          Show
          Colin Patrick McCabe added a comment - remove unnecessary if statement
          Hide
          Colin Patrick McCabe added a comment -

          reopening to remove unnecessary if statement (as adi pointed out)

          Show
          Colin Patrick McCabe added a comment - reopening to remove unnecessary if statement (as adi pointed out)
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #1102 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1102/)
          HDFS-799. libhdfs must call DetachCurrentThread when a thread is destroyed. Contributed by Colin Patrick McCabe (Revision 1361008)

          Result = FAILURE
          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361008
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/CMakeLists.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/config.h.cmake
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfsJniHelper.c
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1102 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1102/ ) HDFS-799 . libhdfs must call DetachCurrentThread when a thread is destroyed. Contributed by Colin Patrick McCabe (Revision 1361008) Result = FAILURE eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361008 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/CMakeLists.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/config.h.cmake /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfsJniHelper.c
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1135 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1135/)
          HDFS-799. libhdfs must call DetachCurrentThread when a thread is destroyed. Contributed by Colin Patrick McCabe (Revision 1361008)

          Result = SUCCESS
          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361008
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/CMakeLists.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/config.h.cmake
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfsJniHelper.c
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1135 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1135/ ) HDFS-799 . libhdfs must call DetachCurrentThread when a thread is destroyed. Contributed by Colin Patrick McCabe (Revision 1361008) Result = SUCCESS eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361008 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/CMakeLists.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/config.h.cmake /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfsJniHelper.c
          Hide
          Eli Collins added a comment -

          +1 to the delta, good catch Andy.

          Show
          Eli Collins added a comment - +1 to the delta, good catch Andy.
          Hide
          Eli Collins added a comment -

          I've committed the delta to trunk and branch-2.

          Show
          Eli Collins added a comment - I've committed the delta to trunk and branch-2.
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #2535 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2535/)
          Amend HDFS-799. Remove unnecessary tls check. Contributed by Colin Patrick McCabe (Revision 1361561)

          Result = SUCCESS
          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361561
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfsJniHelper.c
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #2535 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2535/ ) Amend HDFS-799 . Remove unnecessary tls check. Contributed by Colin Patrick McCabe (Revision 1361561) Result = SUCCESS eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361561 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfsJniHelper.c
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #2470 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2470/)
          Amend HDFS-799. Remove unnecessary tls check. Contributed by Colin Patrick McCabe (Revision 1361561)

          Result = SUCCESS
          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361561
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfsJniHelper.c
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #2470 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2470/ ) Amend HDFS-799 . Remove unnecessary tls check. Contributed by Colin Patrick McCabe (Revision 1361561) Result = SUCCESS eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361561 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfsJniHelper.c
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #2490 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2490/)
          Amend HDFS-799. Remove unnecessary tls check. Contributed by Colin Patrick McCabe (Revision 1361561)

          Result = FAILURE
          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361561
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfsJniHelper.c
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #2490 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2490/ ) Amend HDFS-799 . Remove unnecessary tls check. Contributed by Colin Patrick McCabe (Revision 1361561) Result = FAILURE eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361561 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfsJniHelper.c
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #1104 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1104/)
          Amend HDFS-799. Remove unnecessary tls check. Contributed by Colin Patrick McCabe (Revision 1361561)

          Result = FAILURE
          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361561
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfsJniHelper.c
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1104 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1104/ ) Amend HDFS-799 . Remove unnecessary tls check. Contributed by Colin Patrick McCabe (Revision 1361561) Result = FAILURE eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361561 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfsJniHelper.c
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1137 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1137/)
          Amend HDFS-799. Remove unnecessary tls check. Contributed by Colin Patrick McCabe (Revision 1361561)

          Result = SUCCESS
          eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361561
          Files :

          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfsJniHelper.c
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1137 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1137/ ) Amend HDFS-799 . Remove unnecessary tls check. Contributed by Colin Patrick McCabe (Revision 1361561) Result = SUCCESS eli : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1361561 Files : /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfsJniHelper.c

            People

            • Assignee:
              Colin Patrick McCabe
              Reporter:
              Christian Kunz
            • Votes:
              0 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development