From bf5aefc42215f5c0541b81936744b96fd4a90bb3 Mon Sep 17 00:00:00 2001 From: ClausKlein Date: Fri, 14 May 2021 17:11:32 +0200 Subject: [PATCH 1/8] change the delete while locked test --- threads.cpp | 10 +++++++--- threads_test.cpp | 29 +++++++++++++++++++---------- 2 files changed, 26 insertions(+), 13 deletions(-) diff --git a/threads.cpp b/threads.cpp index 6f1e4b9..30a722c 100644 --- a/threads.cpp +++ b/threads.cpp @@ -163,8 +163,10 @@ Synchronized::Synchronized() Synchronized::~Synchronized() { + int errors = 0; int result = pthread_cond_destroy(&cond); if (result) { + ++errors; LOG_BEGIN(loggerModuleName, ERROR_LOG | 2); LOG("Synchronized cond_destroy failed with (result)(ptr)"); LOG(result); @@ -178,12 +180,12 @@ Synchronized::~Synchronized() // wait for other threads ... if (EBUSY == pthread_mutex_trylock(&monitor)) { // another thread owns the mutex, - if (lock(123)) // NOTE: let's wait, but not forever! CK + if (lock()) // FIXME: let's wait, but not forever! CK { int retries = 0; do { pthread_mutex_unlock(&monitor); - sleep(13); + sleep(13); // NOTE: prevent busy loops! CK result = pthread_mutex_destroy(&monitor); } while (EBUSY == result && (retries++ < AGENTPP_SYNCHRONIZED_UNLOCK_RETRIES)); @@ -192,13 +194,15 @@ Synchronized::~Synchronized() } #endif - isLocked = false; if (result) { + ++errors; LOG_BEGIN(loggerModuleName, ERROR_LOG | 2); LOG("Synchronized mutex_destroy failed with (result)(ptr)"); LOG(result); LOG((unsigned long)this); LOG_END; + } else { + // NO: ThreadSanitizer: prevent data race! CK isLocked = false; } } diff --git a/threads_test.cpp b/threads_test.cpp index 7394dcd..f01777f 100644 --- a/threads_test.cpp +++ b/threads_test.cpp @@ -631,24 +631,33 @@ void handler(int signum) } #endif -BOOST_AUTO_TEST_CASE(SyncDeleteLocked_test) +bool stop = false; +void lock_task(Synchronized* m) { -#ifndef _WIN32 - signal(SIGALRM, &handler); - ualarm(1000, 0); // us -#endif + Lock l(*m); + stop = false; + + while (!stop) { + m->wait(); + } +} +BOOST_AUTO_TEST_CASE(SyncDeleteLocked_test) +{ Stopwatch sw; try { - auto sync = boost::make_shared(); - BOOST_TEST(sync->lock()); -#ifndef __WIN32 - sync->wait(1234); // for signal with timout -#endif + boost::thread t(lock_task, sync.get()); + t.detach(); + Thread::sleep(1234); BOOST_TEST_MESSAGE(BOOST_CURRENT_FUNCTION << sw.elapsed()); + { + BOOST_TEST(sync->lock()); + stop = true; + BOOST_TEST(sync->unlock()); + } } catch (std::exception& e) { BOOST_TEST_MESSAGE(BOOST_CURRENT_FUNCTION); BOOST_TEST_MESSAGE(e.what()); From 02405a9907202b63bc0668e0942b9bce5a2403e7 Mon Sep 17 00:00:00 2001 From: ClausKlein Date: Fri, 14 May 2021 17:23:29 +0200 Subject: [PATCH 2/8] add new workflow for build on ubuntu --- .github/workflows/ubuntu.yml | 44 ++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 .github/workflows/ubuntu.yml diff --git a/.github/workflows/ubuntu.yml b/.github/workflows/ubuntu.yml new file mode 100644 index 0000000..dfe9920 --- /dev/null +++ b/.github/workflows/ubuntu.yml @@ -0,0 +1,44 @@ +--- +name: Ubuntu + +on: + push: + branches: + - master + - develop + pull_request: + branches: + - master + - develop + +env: + CTEST_OUTPUT_ON_FAILURE: 1 + CPM_SOURCE_CACHE: ${{ github.workspace }}/cpm_modules + +jobs: + build: + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v2 + + - uses: actions/cache@v2 + with: + path: "**/cpm_modules" + key: ${{ github.workflow }}-cpm-modules-${{ hashFiles('**/CMakeLists.txt', '**/*.cmake') }} + + - name: Install boost libs + shell: bash + run: | + sudo apt-get install libboost-dev || echo IGNORED + + - name: configure + run: cmake -S test -B build -DCMAKE_BUILD_TYPE=Debug + + - name: build + run: cmake --build build -j4 + + - name: test + run: | + cd build + ctest --build-config Debug From 87bacf050f3973e301299ff95a2a168087cbe86a Mon Sep 17 00:00:00 2001 From: ClausKlein Date: Fri, 14 May 2021 17:35:42 +0200 Subject: [PATCH 3/8] fix source path --- .github/workflows/ubuntu.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ubuntu.yml b/.github/workflows/ubuntu.yml index dfe9920..fc7fbba 100644 --- a/.github/workflows/ubuntu.yml +++ b/.github/workflows/ubuntu.yml @@ -33,7 +33,7 @@ jobs: sudo apt-get install libboost-dev || echo IGNORED - name: configure - run: cmake -S test -B build -DCMAKE_BUILD_TYPE=Debug + run: cmake -S . -B build -DCMAKE_BUILD_TYPE=Debug - name: build run: cmake --build build -j4 @@ -41,4 +41,4 @@ jobs: - name: test run: | cd build - ctest --build-config Debug + ctest --build-config Debug --timeout 60 From f6260a9af2b960c87204fde92de491061c67603d Mon Sep 17 00:00:00 2001 From: ClausKlein Date: Fri, 14 May 2021 17:47:32 +0200 Subject: [PATCH 4/8] install libboost1.71-all-dev on ubuntu --- .github/workflows/ubuntu.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ubuntu.yml b/.github/workflows/ubuntu.yml index fc7fbba..1bae776 100644 --- a/.github/workflows/ubuntu.yml +++ b/.github/workflows/ubuntu.yml @@ -30,7 +30,7 @@ jobs: - name: Install boost libs shell: bash run: | - sudo apt-get install libboost-dev || echo IGNORED + sudo apt-get install libboost1.71-all-dev - name: configure run: cmake -S . -B build -DCMAKE_BUILD_TYPE=Debug From eaecb44ffce82757f825c61c11b3adee1d1231e9 Mon Sep 17 00:00:00 2001 From: ClausKlein Date: Fri, 14 May 2021 17:59:01 +0200 Subject: [PATCH 5/8] add CI test for macos too --- .github/workflows/macos.yml | 44 +++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 .github/workflows/macos.yml diff --git a/.github/workflows/macos.yml b/.github/workflows/macos.yml new file mode 100644 index 0000000..1971111 --- /dev/null +++ b/.github/workflows/macos.yml @@ -0,0 +1,44 @@ +--- +name: MacOS + +on: + push: + branches: + - master + - develop + pull_request: + branches: + - master + - develop + +env: + CTEST_OUTPUT_ON_FAILURE: 1 + CPM_SOURCE_CACHE: ${{ github.workspace }}/cpm_modules + +jobs: + build: + runs-on: macos-latest + + steps: + - uses: actions/checkout@v2 + + - uses: actions/cache@v2 + with: + path: "**/cpm_modules" + key: ${{ github.workflow }}-cpm-modules-${{ hashFiles('**/CMakeLists.txt', '**/*.cmake') }} + + - name: Install boost libs + shell: bash + run: | + brew install boost + + - name: configure + run: cmake -S . -B build -DCMAKE_BUILD_TYPE=Debug + + - name: build + run: cmake --build build -j4 + + - name: test + run: | + cd build + ctest --build-config Debug --timeout 60 From 218d2b734a3af253a1b19a1d95bd60d001db545c Mon Sep 17 00:00:00 2001 From: ClausKlein Date: Fri, 14 May 2021 21:57:30 +0200 Subject: [PATCH 6/8] back to simple version, but with notify_all() call --- posix/threadpool.cpp | 37 ++++++++++++++++++++++++++++++------- threads_test.cpp | 2 +- 2 files changed, 31 insertions(+), 8 deletions(-) diff --git a/posix/threadpool.cpp b/posix/threadpool.cpp index 53a4ecd..d02091d 100644 --- a/posix/threadpool.cpp +++ b/posix/threadpool.cpp @@ -92,11 +92,34 @@ Synchronized::~Synchronized() int error = 0; int errors = 0; -#ifdef NO_FAST_MUTEXES + notify_all(); + +#if defined(NO_FAST_MUTEXES) + error = pthread_mutex_destroy(&monitor); + if (EBUSY == error) { + // wait for other threads ... + error = pthread_mutex_trylock(&monitor); + if (EBUSY == error) { + // another thread owns the mutex, + if (lock()) // FIXME: let's wait, but not forever! CK + { + int retries = 0; + do { + pthread_mutex_unlock(&monitor); + ++errors; + sleep(errors * 2); // NOTE: prevent busy loops! CK + error = pthread_mutex_destroy(&monitor); + } while (EBUSY == error + && (retries++ < AGENTPP_SYNCHRONIZED_UNLOCK_RETRIES)); + } + } + } +#elif defined(NO_FAST_MUTEXES_CK) do { // first try to get the lock error = pthread_mutex_trylock(&monitor); if (!error) { + notify_all(); (void)pthread_mutex_unlock(&monitor); // if another thread waits for signal with mutex, let's wait. @@ -129,7 +152,7 @@ Synchronized::~Synchronized() } } while (EBUSY == error); #else - error = pthread_mutex_destroy(&monitor); + error = pthread_mutex_destroy(&monitor); #endif if (error == EBUSY) { @@ -207,12 +230,12 @@ bool Synchronized::wait(long timeout) # warning "gettimeofday() used" struct timeval tv = {}; gettimeofday(&tv, NULL); - ts.tv_sec = tv.tv_sec + (time_t)timeout / 1000; + ts.tv_sec = tv.tv_sec + (time_t)timeout / 1000; long millis = tv.tv_usec / 1000 + (timeout % 1000); if (millis >= 1000) { ts.tv_sec += 1; } - ts.tv_nsec = (millis % 1000) * 1000000; + ts.tv_nsec = (millis % 1000) * 1000000; # endif int err = cond_timed_wait(ts); @@ -307,7 +330,7 @@ bool Synchronized::lock(unsigned long timeout) # else struct timeval tv = {}; gettimeofday(&tv, 0); - ts.tv_sec = tv.tv_sec + (time_t)timeout / 1000; + ts.tv_sec = tv.tv_sec + (time_t)timeout / 1000; long millis = tv.tv_usec / 1000 + (timeout % 1000); if (millis >= 1000) { ts.tv_sec += 1; @@ -560,8 +583,8 @@ void Thread::nsleep(time_t secs, long nanos) } # else struct timeval interval = {}; - interval.tv_sec = s; - interval.tv_usec = n / 1000; + interval.tv_sec = s; + interval.tv_usec = n / 1000; fd_set writefds, readfds, exceptfds; FD_ZERO(&writefds); FD_ZERO(&readfds); diff --git a/threads_test.cpp b/threads_test.cpp index f01777f..c8091fa 100644 --- a/threads_test.cpp +++ b/threads_test.cpp @@ -631,7 +631,7 @@ void handler(int signum) } #endif -bool stop = false; +boost::atomic stop { false }; void lock_task(Synchronized* m) { Lock l(*m); From 002e4d0a7ec6067b770f05e6e9ce4c36ba4b3d58 Mon Sep 17 00:00:00 2001 From: ClausKlein Date: Sat, 15 May 2021 22:01:28 +0200 Subject: [PATCH 7/8] optimize Synchonized destructor add delete test with locked mutex --- posix/threadpool.cpp | 83 ++++++++++++++++---------------------------- threads_test.cpp | 40 ++++++++++++++------- 2 files changed, 58 insertions(+), 65 deletions(-) diff --git a/posix/threadpool.cpp b/posix/threadpool.cpp index d02091d..b199b92 100644 --- a/posix/threadpool.cpp +++ b/posix/threadpool.cpp @@ -89,70 +89,47 @@ Synchronized::Synchronized() Synchronized::~Synchronized() { - int error = 0; int errors = 0; - notify_all(); + notify_all(); // NOTE: just in case an other thread is waiting for the + // signal! CK + int error = + pthread_mutex_unlock(&monitor); // in case the thread own the lock! CK + if (error) { + ++errors; + } -#if defined(NO_FAST_MUTEXES) +#ifdef NO_FAST_MUTEXES error = pthread_mutex_destroy(&monitor); - if (EBUSY == error) { + if (error) { // wait for other threads ... error = pthread_mutex_trylock(&monitor); if (EBUSY == error) { + ++errors; // another thread owns the mutex, - if (lock()) // FIXME: let's wait, but not forever! CK - { - int retries = 0; - do { - pthread_mutex_unlock(&monitor); - ++errors; - sleep(errors * 2); // NOTE: prevent busy loops! CK - error = pthread_mutex_destroy(&monitor); - } while (EBUSY == error - && (retries++ < AGENTPP_SYNCHRONIZED_UNLOCK_RETRIES)); - } - } - } -#elif defined(NO_FAST_MUTEXES_CK) - do { - // first try to get the lock - error = pthread_mutex_trylock(&monitor); - if (!error) { - notify_all(); - (void)pthread_mutex_unlock(&monitor); - // if another thread waits for signal with mutex, let's wait. # if defined(_POSIX_TIMEOUTS) && _POSIX_TIMEOUTS > 0 - if (lock(10)) // NOTE: but not forever! CK -# else - error = pthread_mutex_trylock(&monitor); - if (!error) + if (lock(100)) { // NOTE: but not forever! CK + pthread_mutex_unlock(&monitor); + } # endif - { - (void)pthread_mutex_unlock(&monitor); - error = pthread_mutex_destroy(&monitor); - if (error) { + + int retries = 0; + do { + error = pthread_mutex_trylock(&monitor); + if (!error) { + pthread_mutex_unlock(&monitor); + error = pthread_mutex_destroy(&monitor); + } else { ++errors; - Thread::sleep(errors * 2); + sleep(errors * 2); // NOTE: prevent busy loops! CK } - } - } else { - // in case this thread hold the lock: - error = pthread_mutex_unlock(&monitor); - if (error != EPERM) { - break; - } - notify_all(); - ++errors; - Thread::sleep(errors * 2); - } - if (errors > AGENTPP_SYNCHRONIZED_UNLOCK_RETRIES) { - break; + } while (EBUSY == error + && (retries++ < AGENTPP_SYNCHRONIZED_UNLOCK_RETRIES)); } - } while (EBUSY == error); + } #else - error = pthread_mutex_destroy(&monitor); + error = pthread_mutex_destroy(&monitor); #endif if (error == EBUSY) { @@ -230,12 +207,12 @@ bool Synchronized::wait(long timeout) # warning "gettimeofday() used" struct timeval tv = {}; gettimeofday(&tv, NULL); - ts.tv_sec = tv.tv_sec + (time_t)timeout / 1000; + ts.tv_sec = tv.tv_sec + (time_t)timeout / 1000; long millis = tv.tv_usec / 1000 + (timeout % 1000); if (millis >= 1000) { ts.tv_sec += 1; } - ts.tv_nsec = (millis % 1000) * 1000000; + ts.tv_nsec = (millis % 1000) * 1000000; # endif int err = cond_timed_wait(ts); @@ -583,8 +560,8 @@ void Thread::nsleep(time_t secs, long nanos) } # else struct timeval interval = {}; - interval.tv_sec = s; - interval.tv_usec = n / 1000; + interval.tv_sec = s; + interval.tv_usec = n / 1000; fd_set writefds, readfds, exceptfds; FD_ZERO(&writefds); FD_ZERO(&readfds); diff --git a/threads_test.cpp b/threads_test.cpp index c8091fa..dedc147 100644 --- a/threads_test.cpp +++ b/threads_test.cpp @@ -618,18 +618,16 @@ BOOST_AUTO_TEST_CASE(SyncDeadlock_test) BOOST_TEST(sync.unlock()); } -#ifndef _WIN32 -void handler(int signum) +BOOST_AUTO_TEST_CASE(SyncDeleteLocked_test) { - switch (signum) { - case SIGALRM: - signal(signum, SIG_DFL); - break; - default: // ignored - break; + try { + Synchronized sync; + BOOST_TEST(sync.lock()); + } catch (std::exception& e) { + BOOST_TEST_MESSAGE(BOOST_CURRENT_FUNCTION); + BOOST_TEST_MESSAGE(e.what()); } } -#endif boost::atomic stop { false }; void lock_task(Synchronized* m) @@ -638,11 +636,15 @@ void lock_task(Synchronized* m) stop = false; while (!stop) { + Thread::sleep(123); // NOTE: do somet work ... m->wait(); } + + Thread::sleep( + BOOST_THREAD_TEST_TIME_MS); // do some cleanup task ... with lock! } -BOOST_AUTO_TEST_CASE(SyncDeleteLocked_test) +BOOST_AUTO_TEST_CASE(SyncDeleteWaiting_test) { Stopwatch sw; try { @@ -651,13 +653,14 @@ BOOST_AUTO_TEST_CASE(SyncDeleteLocked_test) boost::thread t(lock_task, sync.get()); t.detach(); - Thread::sleep(1234); + Thread::sleep(BOOST_THREAD_TEST_TIME_MS); BOOST_TEST_MESSAGE(BOOST_CURRENT_FUNCTION << sw.elapsed()); { BOOST_TEST(sync->lock()); - stop = true; + stop = true; // NOTE: without notify! CK BOOST_TEST(sync->unlock()); } + sync.reset(); // NOTE: this delete the Synchronized obj! CK } catch (std::exception& e) { BOOST_TEST_MESSAGE(BOOST_CURRENT_FUNCTION); BOOST_TEST_MESSAGE(e.what()); @@ -793,6 +796,19 @@ BOOST_AUTO_TEST_CASE(ThreadLivetime_test) BOOST_TEST_MESSAGE(BOOST_CURRENT_FUNCTION << sw.elapsed()); } +#ifndef _WIN32 +void handler(int signum) +{ + switch (signum) { + case SIGALRM: + signal(signum, SIG_DFL); + break; + default: // ignored + break; + } +} +#endif + BOOST_AUTO_TEST_CASE(ThreadNanoSleep_test) { #ifndef _WIN32 From a3ce40b867afb1b2d797dab62d48dfa27fc727f1 Mon Sep 17 00:00:00 2001 From: ClausKlein Date: Mon, 17 May 2021 17:42:45 +0200 Subject: [PATCH 8/8] prepare memory sanetizer too --- CMakeLists.txt | 4 ++++ GNUmakefile | 3 ++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index c55eb68..0bb7d08 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -179,6 +179,10 @@ if(USE_BOOST OR BUILD_TESTING) # ---------------------------------------------------------------------- # additional tests from me (CK) # ---------------------------------------------------------------------- + if(CMAKE_CXX_COMPILER_ID MATCHES "Clang") + # add_compile_options(-fsanitize=memory -fsanitize-memory-use-after-dtor) + endif() + add_executable(test_mutexattr test_mutexattr.c) set_target_properties(test_mutexattr PROPERTIES CXX_STANDARD 99) target_link_libraries(test_mutexattr Threads::Threads) diff --git a/GNUmakefile b/GNUmakefile index 912c29b..8f7c8bf 100644 --- a/GNUmakefile +++ b/GNUmakefile @@ -26,7 +26,7 @@ CHECKS?='-*,cppcoreguidelines-*,cppcoreguidelines-pro-*,-cppcoreguidelines-avoid CHECKS?='-*,portability-*,readability-*' CHECKS?='-*,misc-*,boost-*,cert-*,misc-unused-parameters' -#FIXME! ThreadSanitizer?=0 +#TBD: ThreadSanitizer?=0 ifeq ($(BUILD_TYPE),Coverage) ThreadSanitizer:=0 else @@ -44,6 +44,7 @@ ifeq (${ThreadSanitizer},1) export CMAKE_INSTALL_PREFIX CMAKE_STAGING_PREFIX?=/tmp/staging/$(PROJECT_NAME)$(CMAKE_INSTALL_PREFIX) CMAKE_PREFIX_PATH?="$(CMAKE_STAGING_PREFIX);$(CMAKE_INSTALL_PREFIX);/usr/local/opt/boost;/usr/local/opt/openssl;/usr" + export MSAN_OPTIONS=poison_in_dtor=1 endif