diff --git a/.github/scripts/execute-tests-under-sanitizer.sh b/.github/scripts/execute-tests-under-sanitizer.sh deleted file mode 100755 index 9646994bb..000000000 --- a/.github/scripts/execute-tests-under-sanitizer.sh +++ /dev/null @@ -1,46 +0,0 @@ -#!/bin/bash - -set -o pipefail - -# Note: This script is intended to be run from the root of the repository. -# -# This script runs each unit-test separately and generates reports from the currently active sanitizer. -# Output is saved in ./.sanitizer-report in the root of the repository - -if [[ -z "$1" ]]; then - cat <"$OUTPUT_FILE" 2>&1 - - if [ $? -ne 0 ]; then - echo "'$TEST' failed a sanitizer check." - else - rm "$OUTPUT_FILE" - fi -done diff --git a/.github/workflows/reusable-test.yml b/.github/workflows/reusable-test.yml index 2cec38398..294d08ab0 100644 --- a/.github/workflows/reusable-test.yml +++ b/.github/workflows/reusable-test.yml @@ -45,10 +45,6 @@ jobs: if: ${{ inputs.run_unit_tests }} - env: - # TODO: remove completely when we have fixed all currently existing issues with sanitizers - SANITIZER_IGNORE_ERRORS: ${{ endsWith(inputs.conan_profile, '.tsan') }} - steps: - name: Cleanup workspace if: ${{ runner.os == 'macOS' }} @@ -65,34 +61,13 @@ jobs: - name: Make clio_tests executable run: chmod +x ./clio_tests - - name: Run clio_tests (regular) - if: ${{ env.SANITIZER_IGNORE_ERRORS == 'false' }} + - name: Run clio_tests + continue-on-error: true + id: run_clio_tests run: ./clio_tests - - name: Run clio_tests (sanitizer errors ignored) - if: ${{ env.SANITIZER_IGNORE_ERRORS == 'true' }} - run: ./.github/scripts/execute-tests-under-sanitizer.sh ./clio_tests - - - name: Check for sanitizer report - if: ${{ env.SANITIZER_IGNORE_ERRORS == 'true' }} - id: check_report - run: | - if ls .sanitizer-report/* 1> /dev/null 2>&1; then - echo "found_report=true" >> $GITHUB_OUTPUT - else - echo "found_report=false" >> $GITHUB_OUTPUT - fi - - - name: Upload sanitizer report - if: ${{ env.SANITIZER_IGNORE_ERRORS == 'true' && steps.check_report.outputs.found_report == 'true' }} - uses: actions/upload-artifact@b7c566a772e6b6bfb58ed0dc250532a479d7789f # v6.0.0 - with: - name: sanitizer_report_${{ runner.os }}_${{ inputs.build_type }}_${{ inputs.conan_profile }} - path: .sanitizer-report/* - include-hidden-files: true - - name: Create an issue - if: ${{ false && env.SANITIZER_IGNORE_ERRORS == 'true' && steps.check_report.outputs.found_report == 'true' }} + if: ${{ steps.run_clio_tests.outcome == 'failure' && endsWith(inputs.conan_profile, 'san') }} uses: ./.github/actions/create-issue env: GH_TOKEN: ${{ github.token }} @@ -100,10 +75,13 @@ jobs: labels: "bug" title: "[${{ inputs.conan_profile }}] reported issues" body: > - Clio tests failed one or more sanitizer checks when built with ${{ inputs.conan_profile }}`. + Clio tests failed one or more sanitizer checks when built with `${{ inputs.conan_profile }}`. Workflow: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}/ - Reports are available as artifacts. + + - name: Fail the job if clio_tests failed + if: ${{ steps.run_clio_tests.outcome == 'failure' }} + run: exit 1 integration_tests: name: Integration testing diff --git a/.github/workflows/sanitizers.yml b/.github/workflows/sanitizers.yml index ba397423b..3f795b730 100644 --- a/.github/workflows/sanitizers.yml +++ b/.github/workflows/sanitizers.yml @@ -15,7 +15,6 @@ on: - ".github/actions/**" - "!.github/actions/build-docker-image/**" - "!.github/actions/create-issue/**" - - .github/scripts/execute-tests-under-sanitizer.sh - CMakeLists.txt - conanfile.py diff --git a/src/cluster/impl/RepeatedTask.hpp b/src/cluster/impl/RepeatedTask.hpp index 9037c6a47..8e4ef8ea5 100644 --- a/src/cluster/impl/RepeatedTask.hpp +++ b/src/cluster/impl/RepeatedTask.hpp @@ -30,7 +30,6 @@ #include #include #include -#include #include #include @@ -96,7 +95,12 @@ public: if (auto expected = State::Running; not state_.compare_exchange_strong(expected, State::Stopped)) return; // Already stopped or not started - boost::asio::spawn(strand_, [this](auto&&) { timer_.cancel(); }, boost::asio::use_future).wait(); + std::binary_semaphore cancelSemaphore{0}; + boost::asio::post(strand_, [this, &cancelSemaphore]() { + timer_.cancel(); + cancelSemaphore.release(); + }); + cancelSemaphore.acquire(); semaphore_.acquire(); } }; diff --git a/src/feed/impl/SingleFeedBase.cpp b/src/feed/impl/SingleFeedBase.cpp index ded998914..1a08609cf 100644 --- a/src/feed/impl/SingleFeedBase.cpp +++ b/src/feed/impl/SingleFeedBase.cpp @@ -64,7 +64,7 @@ SingleFeedBase::unsub(SubscriberSharedPtr const& subscriber) void SingleFeedBase::pub(std::string msg) { - [[maybe_unused]] auto task = strand_.execute([this, msg = std::move(msg)]() { + strand_.submit([this, msg = std::move(msg)] { auto const msgPtr = std::make_shared(msg); signal_.emit(msgPtr); }); diff --git a/src/feed/impl/TrackableSignal.hpp b/src/feed/impl/TrackableSignal.hpp index c0629ec3a..70407e1b9 100644 --- a/src/feed/impl/TrackableSignal.hpp +++ b/src/feed/impl/TrackableSignal.hpp @@ -73,10 +73,15 @@ public: // This class can't hold the trackable's shared_ptr, because disconnect should be able to be called in the // the trackable's destructor. However, the trackable can not be destroyed when the slot is being called - // either. track_foreign will hold a weak_ptr to the connection, which makes sure the connection is valid when - // the slot is called. + // either. `track_foreign` is racey when one shared_ptr is tracked by multiple signals. Therefore we are storing + // a weak_ptr of the trackable and using weak_ptr::lock() to atomically check existence and acquire a shared_ptr + // during slot invocation. This guarantees to keep the trackable alive for the duration of the slot call and + // avoids potential race conditions. connections->emplace( - trackable.get(), signal_.connect(typename SignalType::slot_type(slot).track_foreign(trackable)) + trackable.get(), signal_.connect([slot, weakTrackable = std::weak_ptr(trackable)](Args&&... args) { + if (auto lifeExtender = weakTrackable.lock(); lifeExtender) + std::invoke(slot, std::forward(args)...); + }) ); return true; } diff --git a/tests/unit/feed/SubscriptionManagerTests.cpp b/tests/unit/feed/SubscriptionManagerTests.cpp index 5ea968d05..5cb20557a 100644 --- a/tests/unit/feed/SubscriptionManagerTests.cpp +++ b/tests/unit/feed/SubscriptionManagerTests.cpp @@ -72,11 +72,11 @@ protected: } StrictMockAmendmentCenterSharedPtr mockAmendmentCenterPtr_; - std::shared_ptr subscriptionManagerPtr_ = - std::make_shared(Execution(2), backend_, mockAmendmentCenterPtr_); web::SubscriptionContextPtr session_ = std::make_shared(); MockSession* sessionPtr_ = dynamic_cast(session_.get()); uint32_t const networkID_ = 123; + std::shared_ptr subscriptionManagerPtr_ = + std::make_shared(Execution(2), backend_, mockAmendmentCenterPtr_); }; using SubscriptionManagerTest = SubscriptionManagerBaseTest; diff --git a/tests/unit/util/async/AsyncExecutionContextTests.cpp b/tests/unit/util/async/AsyncExecutionContextTests.cpp index 9bc25c3c9..32008983c 100644 --- a/tests/unit/util/async/AsyncExecutionContextTests.cpp +++ b/tests/unit/util/async/AsyncExecutionContextTests.cpp @@ -209,7 +209,7 @@ TYPED_TEST(ExecutionContextTests, repeatingOperation) { auto const repeatDelay = std::chrono::milliseconds{1}; auto const timeout = std::chrono::milliseconds{15}; - auto callCount = 0uz; + std::atomic_size_t callCount = 0uz; auto res = this->ctx.executeRepeatedly(repeatDelay, [&] { ++callCount; }); auto timeSpent = util::timed([timeout] { std::this_thread::sleep_for(timeout); }); // calculate actual time spent