On 15/02/2023 17:17, Klaus Schmidinger wrote:
On 02.02.23 21:56, Patrick Lerda wrote:
...
diff --git a/thread.c b/thread.c
index 93eb8c0..21be7a4 100644
--- a/thread.c
+++ b/thread.c
@@ -312,13 +312,16 @@ bool cThread::Start(void)
cCondWait::SleepMs(THREAD_STOP_SLEEP);
}
if (!active) {
- active = running = true;
- if (pthread_create(&childTid, NULL, (void *(*) (void
*))&StartThread, (void *)this) == 0) {
- pthread_detach(childTid); // auto-reap
+ pthread_t localTid;
+ running = true;
+ if (pthread_create(&localTid, NULL, (void *(*) (void
*))&StartThread, (void *)this) == 0) {
+ pthread_detach(localTid); // auto-reap
+ childTid = localTid;
+ active = true;
}
else {
LOG_ERROR;
- active = running = false;
+ running = false;
return false;
}
}
@@ -339,11 +342,12 @@ bool cThread::Active(void)
// performed but no signal is actually sent.
//
int err;
- if ((err = pthread_kill(childTid, 0)) != 0) {
+ const pthread_t localTid = childTid;
+ if ((err = pthread_kill(localTid, 0)) != 0) {
if (err != ESRCH)
LOG_ERROR;
- childTid = 0;
- active = running = false;
+ if (active && childTid == localTid)
localTid was initialized to childTid 4 lines earlier, so what's with
the "childTid == localTid" check here? Isn't this always true?
The function pthread_kill() is not instantaneous and could pause the
thread, this condition ensures that childTid was not updated in the
meantime by a second thread.
+ active = running = false;
}
else
return true;
@@ -355,6 +359,7 @@ void cThread::Cancel(int WaitSeconds)
{
running = false;
if (active && WaitSeconds > -1) {
+ const pthread_t localTid = childTid;
if (WaitSeconds > 0) {
for (time_t t0 = time(NULL) + WaitSeconds; time(NULL) < t0;
) {
if (!Active())
@@ -363,9 +368,9 @@ void cThread::Cancel(int WaitSeconds)
}
esyslog("ERROR: %s thread %d won't end (waited %d seconds) -
canceling it...", description ? description : "", childThreadId,
WaitSeconds);
}
- pthread_cancel(childTid);
- childTid = 0;
- active = false;
+ pthread_cancel(localTid);
+ if (active && childTid == localTid)
Same here?
I see this happens with "address sanitizer". Is there an actual,
reproducible, real world problem that this patch fixes?
I had a few random crashes related to cThread, so it could happen.
Anyway the sanitizer seems to increase the likelihood of these events.
+ active = false;
}
}
diff --git a/thread.h b/thread.h
index 16c4bd7..06046ea 100644
--- a/thread.h
+++ b/thread.h
@@ -13,6 +13,7 @@
#include <pthread.h>
#include <stdio.h>
#include <sys/types.h>
+#include <atomic>
typedef pid_t tThreadId;
@@ -56,7 +57,7 @@ class cRwLock {
private:
pthread_rwlock_t rwlock;
int locked;
- tThreadId writeLockThreadId;
+ std::atomic<tThreadId> writeLockThreadId;
public:
cRwLock(bool PreferWriter = false);
~cRwLock();
@@ -79,9 +80,11 @@ public:
class cThread {
friend class cThreadLock;
private:
- bool active;
- bool running;
- pthread_t childTid;
+ std::atomic_bool active;
+ std::atomic_bool running;
+ std::atomic<pthread_t> childTid;
+ ///< Assume that the content of childTid is valid when the
class member
+ ///< active is set to true and undefined otherwise.
tThreadId childThreadId;
cMutex mutex;
char *description;
Are the "atomics" really necessary?
Atomic is mandatory, the compiler has to generate the proper op codes to
ensure "atomic" operations at the thread level.
Patrick
Klaus
_______________________________________________
vdr mailing list
vdr@xxxxxxxxxxx
https://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr
_______________________________________________
vdr mailing list
vdr@xxxxxxxxxxx
https://www.linuxtv.org/cgi-bin/mailman/listinfo/vdr
