From 652e838f57061c17bfdde756ed9a7d29e5fe57bc Mon Sep 17 00:00:00 2001 From: Kevin Ushey Date: Wed, 26 Oct 2022 22:48:12 -0700 Subject: [PATCH 1/5] allow pthread_join after thread has finished execution --- inst/include/tthread/tinythread.h | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/inst/include/tthread/tinythread.h b/inst/include/tthread/tinythread.h index e2f56e44..a1724207 100644 --- a/inst/include/tthread/tinythread.h +++ b/inst/include/tthread/tinythread.h @@ -492,7 +492,7 @@ class thread { /// Default constructor. /// Construct a @c thread object without an associated thread of execution /// (i.e. non-joinable). - thread() : mHandle(0), mNotAThread(true) + thread() : mHandle(0), mJoinable(false) #if defined(_TTHREAD_WIN32_) , mWin32ThreadID(0) #endif @@ -554,7 +554,7 @@ class thread { private: native_handle_type mHandle; ///< Thread handle. mutable mutex mDataMutex; ///< Serializer for access to the thread private data. - bool mNotAThread; ///< True if this object is not a thread of execution. + bool mJoinable; ///< Is the thread joinable? #if defined(_TTHREAD_WIN32_) unsigned int mWin32ThreadID; ///< Unique thread ID (filled out by _beginthreadex). #endif @@ -871,11 +871,16 @@ inline void * thread::wrapper_function(void * aArg) // The thread is no longer executing lock_guard guard(ti->mThread->mDataMutex); - ti->mThread->mNotAThread = true; + // On Linux, we allow the thread to be joined even after execution has finished. + // This is necessary to ensure that thread-local memory can be cleaned up. + // // The thread is responsible for freeing the startup information - delete ti; +#if defined(_TTHREAD_WIN32_) + ti->mThread->mJoinable = false; +#endif + delete ti; return 0; } @@ -891,8 +896,8 @@ inline thread::thread(void (*aFunction)(void *), void * aArg) ti->mArg = aArg; ti->mThread = this; - // The thread is now alive - mNotAThread = false; + // Mark thread as joinable + mJoinable = true; // Create the thread #if defined(_TTHREAD_WIN32_) @@ -905,9 +910,10 @@ inline thread::thread(void (*aFunction)(void *), void * aArg) // Did we fail to create the thread? if(!mHandle) { - mNotAThread = true; + mJoinable = false; delete ti; } + } inline thread::~thread() @@ -926,28 +932,31 @@ inline void thread::join() #elif defined(_TTHREAD_POSIX_) pthread_join(mHandle, NULL); #endif + mJoinable = false; } } inline bool thread::joinable() const { mDataMutex.lock(); - bool result = !mNotAThread; + bool result = mJoinable; mDataMutex.unlock(); return result; } inline void thread::detach() { + // TODO: Attempting to detach a non-joinable thread should throw. + // https://en.cppreference.com/w/cpp/thread/thread/detach mDataMutex.lock(); - if(!mNotAThread) + if(mJoinable) { #if defined(_TTHREAD_WIN32_) CloseHandle(mHandle); #elif defined(_TTHREAD_POSIX_) pthread_detach(mHandle); #endif - mNotAThread = true; + mJoinable = false; } mDataMutex.unlock(); } From 3f5b64d688c9ecf87f5e8c0f6bc26abb3e634a27 Mon Sep 17 00:00:00 2001 From: Kevin Ushey Date: Wed, 26 Oct 2022 22:50:37 -0700 Subject: [PATCH 2/5] fixup --- inst/include/tthread/tinythread.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/inst/include/tthread/tinythread.h b/inst/include/tthread/tinythread.h index a1724207..6f799871 100644 --- a/inst/include/tthread/tinythread.h +++ b/inst/include/tthread/tinythread.h @@ -872,7 +872,7 @@ inline void * thread::wrapper_function(void * aArg) // The thread is no longer executing lock_guard guard(ti->mThread->mDataMutex); - // On Linux, we allow the thread to be joined even after execution has finished. + // On POSIX, we allow the thread to be joined even after execution has finished. // This is necessary to ensure that thread-local memory can be cleaned up. // // The thread is responsible for freeing the startup information From 71f8ebd25a531e2de4d65a17d21a9c88e9c04140 Mon Sep 17 00:00:00 2001 From: Kevin Ushey Date: Wed, 26 Oct 2022 22:55:03 -0700 Subject: [PATCH 3/5] comments --- inst/include/tthread/tinythread.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/inst/include/tthread/tinythread.h b/inst/include/tthread/tinythread.h index 6f799871..8d2f47ed 100644 --- a/inst/include/tthread/tinythread.h +++ b/inst/include/tthread/tinythread.h @@ -874,13 +874,13 @@ inline void * thread::wrapper_function(void * aArg) // On POSIX, we allow the thread to be joined even after execution has finished. // This is necessary to ensure that thread-local memory can be cleaned up. - // - // The thread is responsible for freeing the startup information #if defined(_TTHREAD_WIN32_) ti->mThread->mJoinable = false; #endif + // The thread is responsible for freeing the startup information delete ti; + return 0; } @@ -932,6 +932,12 @@ inline void thread::join() #elif defined(_TTHREAD_POSIX_) pthread_join(mHandle, NULL); #endif + + // https://linux.die.net/man/3/pthread_join states: + // + // Joining with a thread that has previously been joined results in undefined behavior. + // + // We just allow a thread to be joined once. mJoinable = false; } } From 4c475d3cde87b625146987c66e9faa44c5c7df32 Mon Sep 17 00:00:00 2001 From: Kevin Ushey Date: Wed, 26 Oct 2022 22:56:24 -0700 Subject: [PATCH 4/5] update NEWS --- NEWS.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/NEWS.md b/NEWS.md index 96e9acd5..9bfe7059 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,4 +1,9 @@ +## RcppParallel 5.1.6 (UNRELEASED) + +* Fixed a memory leak that could occur when using TinyThread on POSIX systems. + (#185; @dipertix and and @kevinushey) + ## RcppParallel 5.1.5 * Patches to ensure compatibility with the R 4.2.0 UCRT toolchain on Windows, From c663396bb0969515bee3b24b70fd5db1914f4caa Mon Sep 17 00:00:00 2001 From: Kevin Ushey Date: Thu, 27 Oct 2022 09:36:37 -0700 Subject: [PATCH 5/5] comment tweak --- inst/include/tthread/tinythread.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/inst/include/tthread/tinythread.h b/inst/include/tthread/tinythread.h index 8d2f47ed..290dba43 100644 --- a/inst/include/tthread/tinythread.h +++ b/inst/include/tthread/tinythread.h @@ -873,7 +873,7 @@ inline void * thread::wrapper_function(void * aArg) lock_guard guard(ti->mThread->mDataMutex); // On POSIX, we allow the thread to be joined even after execution has finished. - // This is necessary to ensure that thread-local memory can be cleaned up. + // This is necessary to ensure that thread-local memory can be reclaimed. #if defined(_TTHREAD_WIN32_) ti->mThread->mJoinable = false; #endif