From 63f3c5a098439d14492939dddf5bdc5192fb2c68 Mon Sep 17 00:00:00 2001 From: Daniel Adam Date: Mon, 4 Sep 2023 13:16:29 +0200 Subject: [PATCH] Refactor commit message for clarity and conciseness: **Fix Race Condition in Main Loop** This commit addresses a race condition in the main loop involving `pthread_cond_wait()` and `pthread_cond_signal()`. The issue occurs in multithreaded applications using condition variables in the main loop, as follows: **Previous code:** ```C while (quit != 1) { oc_clock_time_t next_event = oc_main_poll_v1(); pthread_mutex_lock(&g_mutex); ... pthread_cond_wait(&g_cv, &g_mutex); ... pthread_mutex_unlock(&g_mutex); } ``` ```C void signal_event_loop(void) { pthread_cond_signal(&g_cv); } ``` The problem arises when: 1. The `oc_main_poll_v1()` call in the main thread is completed. 2. The main thread is paused. 3. A worker thread requests polling and calls `signal_event_loop()`, expecting to wake up the main thread for `oc_main_poll_v1`. 4. The worker thread is paused. 5. The main thread resumes execution. 6. `pthread_cond_wait` is called, causing the main loop to wait on the condition variable, missing the requested polling from the worker thread. To resolve this issue, we introduced additional synchronization. We extended the public API with a new function, `bool oc_main_needs_poll()`, which returns `true` if polling was requested but not yet processed. By using `oc_main_needs_poll()` and correctly synchronizing `pthread_cond_wait` and `pthread_cond_signal`, we prevent the race condition. **Updated Code:** ```C while (quit != 1) { oc_clock_time_t next_event = oc_main_poll_v1(); pthread_mutex_lock(&g_mutex); if (oc_main_needs_poll()) { pthread_mutex_unlock(&g_mutex); continue; } ... pthread_cond_wait(&g_cv, &g_mutex); ... pthread_mutex_unlock(&g_mutex); } ``` ```C void signal_event_loop(void) { pthread_mutex_lock(&g_mutex); pthread_cond_signal(&g_cv); pthread_mutex_unlock(&g_mutex); } ``` This change ensures correct synchronization in the main loop, preventing race conditions when waiting for polling requests. --- .github/workflows/cmake-linux.yml | 2 + .../workflows/plgd-device-test-with-cfg.yml | 8 +- .github/workflows/plgd-device-tests.yml | 8 + .github/workflows/plgd-hub-test-with-cfg.yml | 8 +- .github/workflows/plgd-hub-tests.yml | 14 + api/oc_main.c | 6 + api/unittest/maintest.cpp | 318 ++++++++++++++++++ api/unittest/ocapitest.cpp | 8 +- apps/cloud_server.c | 75 +++-- .../apps/Dockerfile.cloud-server-debug-clang | 34 ++ include/oc_api.h | 13 + port/esp32/main/main.c | 22 +- tests/gtest/Device.cpp | 8 +- tests/gtest/Device.h | 2 +- util/oc_compiler.h | 6 + util/oc_process.c | 8 +- util/oc_process.h | 7 + util/unittest/etimertest.cpp | 13 +- util/unittest/processtest.cpp | 36 +- 19 files changed, 542 insertions(+), 54 deletions(-) create mode 100644 api/unittest/maintest.cpp create mode 100644 docker/apps/Dockerfile.cloud-server-debug-clang diff --git a/.github/workflows/cmake-linux.yml b/.github/workflows/cmake-linux.yml index e84944d248..c88501a23e 100644 --- a/.github/workflows/cmake-linux.yml +++ b/.github/workflows/cmake-linux.yml @@ -102,6 +102,8 @@ jobs: install_faketime: false # thread sanitizer - args: -DOC_TSAN_ENABLED=ON + # GCC thread-sanitizer keeps reporting false positives, so we use clang instead for tests with thread-sanitizer + clang: true install_faketime: true # undefined behaviour sanitizer - args: -DOC_UBSAN_ENABLED=ON diff --git a/.github/workflows/plgd-device-test-with-cfg.yml b/.github/workflows/plgd-device-test-with-cfg.yml index 3b78762d3f..b104be8667 100644 --- a/.github/workflows/plgd-device-test-with-cfg.yml +++ b/.github/workflows/plgd-device-test-with-cfg.yml @@ -25,6 +25,11 @@ on: type: string required: false default: P256 + clang: + description: Use clang instead of gcc + type: boolean + required: false + default: false coverage: type: boolean required: false @@ -42,6 +47,7 @@ env: CERT_TOOL_IMAGE: ghcr.io/plgd-dev/hub/cert-tool:vnext CERT_PATH: .tmp/pki_certs CLOUD_SERVER_DOCKER_FILE: docker/apps/Dockerfile.cloud-server-debug + CLOUD_SERVER_CLANG_DOCKER_FILE: docker/apps/Dockerfile.cloud-server-debug-clang CLOUD_SERVER_DOCKER_TAG: dbg jobs: @@ -60,7 +66,7 @@ jobs: with: context: . push: false - file: ${{ env.CLOUD_SERVER_DOCKER_FILE }} + file: ${{ (inputs.clang && env.CLOUD_SERVER_CLANG_DOCKER_FILE) || env.CLOUD_SERVER_DOCKER_FILE }} tags: ${{ env.CLOUD_SERVER_DOCKER_TAG }} build-args: | BUILD_ARGS=${{ inputs.build_args }} diff --git a/.github/workflows/plgd-device-tests.yml b/.github/workflows/plgd-device-tests.yml index b5ffe0b30b..667f58131c 100644 --- a/.github/workflows/plgd-device-tests.yml +++ b/.github/workflows/plgd-device-tests.yml @@ -49,6 +49,8 @@ jobs: args: "-DOC_ASAN_ENABLED=ON" - name: cloud-server-tsan args: "-DOC_TSAN_ENABLED=ON" + # GCC thread-sanitizer keeps reporting false positives, so we use clang instead for tests with thread-sanitizer + clang: true - name: cloud-server-access-in-RFOTM args: "-DOC_RESOURCE_ACCESS_IN_RFOTM_ENABLED=ON" @@ -56,6 +58,7 @@ jobs: args: "-DOC_ASAN_ENABLED=ON -DOC_RESOURCE_ACCESS_IN_RFOTM_ENABLED=ON" - name: cloud-server-tsan-access-in-RFOTM args: "-DOC_TSAN_ENABLED=ON -DOC_RESOURCE_ACCESS_IN_RFOTM_ENABLED=ON" + clang: true - name: cloud-server-discovery-resource-observable args: "-DOC_DISCOVERY_RESOURCE_OBSERVABLE_ENABLED=ON" @@ -63,6 +66,7 @@ jobs: args: "-DOC_DISCOVERY_RESOURCE_OBSERVABLE_ENABLED=ON -DOC_ASAN_ENABLED=ON" - name: cloud-server-discovery-resource-observable-tsan args: "-DOC_DISCOVERY_RESOURCE_OBSERVABLE_ENABLED=ON -DOC_TSAN_ENABLED=ON" + clang: true - name: cloud-server-discovery-resource-observable-access-in-RFOTM args: "-DOC_DISCOVERY_RESOURCE_OBSERVABLE_ENABLED=ON -DOC_RESOURCE_ACCESS_IN_RFOTM_ENABLED=ON" @@ -70,6 +74,7 @@ jobs: args: "-DOC_DISCOVERY_RESOURCE_OBSERVABLE_ENABLED=ON -DOC_ASAN_ENABLED=ON -DOC_RESOURCE_ACCESS_IN_RFOTM_ENABLED=ON" - name: cloud-server-discovery-resource-observable-tsan-access-in-RFOTM args: "-DOC_DISCOVERY_RESOURCE_OBSERVABLE_ENABLED=ON -DOC_TSAN_ENABLED=ON -DOC_RESOURCE_ACCESS_IN_RFOTM_ENABLED=ON" + clang: true - name: cloud-server-rep-realloc args: "-DOC_REPRESENTATION_REALLOC_ENCODING_ENABLED=ON" @@ -77,6 +82,7 @@ jobs: args: "-DOC_REPRESENTATION_REALLOC_ENCODING_ENABLED=ON -DOC_ASAN_ENABLED=ON" - name: cloud-server-rep-realloc-tsan args: "-DOC_REPRESENTATION_REALLOC_ENCODING_ENABLED=ON -DOC_TSAN_ENABLED=ON" + clang: true - name: cloud-server-discovery-resource-observable-access-in-RFOTM-rep-realloc # same configuration as "cloud-server-discovery-resource-observable-access-in-RFOTM-rep-realloc" in the SonarCloud scan job, skip for events that trigger both jobs @@ -86,6 +92,7 @@ jobs: args: "-DOC_DISCOVERY_RESOURCE_OBSERVABLE_ENABLED=ON -DOC_RESOURCE_ACCESS_IN_RFOTM_ENABLED=ON -DOC_REPRESENTATION_REALLOC_ENCODING_ENABLED=ON-DOC_ASAN_ENABLED=ON" - name: cloud-server-discovery-resource-observable-access-in-RFOTM-rep-realloc-tsan args: "-DOC_DISCOVERY_RESOURCE_OBSERVABLE_ENABLED=ON -DOC_RESOURCE_ACCESS_IN_RFOTM_ENABLED=ON -DOC_REPRESENTATION_REALLOC_ENCODING_ENABLED=ON -DOC_TSAN_ENABLED=ON" + clang: true uses: ./.github/workflows/plgd-device-test-with-cfg.yml with: @@ -94,4 +101,5 @@ jobs: build_type: Debug cert_signature_algorithm: ${{ (github.event_name == 'workflow_dispatch' && inputs.cert_signature_algorithm) || 'ECDSA-SHA256' }} cert_elliptic_curve: ${{ (github.event_name == 'workflow_dispatch' && inputs.cert_elliptic_curve) || 'P256' }} + clang: ${{ matrix.clang || false }} skip: ${{ matrix.skip || false }} diff --git a/.github/workflows/plgd-hub-test-with-cfg.yml b/.github/workflows/plgd-hub-test-with-cfg.yml index fd23d6724f..f2ed47c7c1 100644 --- a/.github/workflows/plgd-hub-test-with-cfg.yml +++ b/.github/workflows/plgd-hub-test-with-cfg.yml @@ -30,6 +30,11 @@ on: type: string required: false default: P256 + clang: + description: Use clang instead of gcc + type: boolean + required: false + default: false coverage: description: gather and upload coverage data type: boolean @@ -58,6 +63,7 @@ on: env: TEST_CLOUD_SERVER_IMAGE: ghcr.io/plgd-dev/hub/test-cloud-server:latest CLOUD_SERVER_DOCKER_FILE: docker/apps/Dockerfile.cloud-server-debug + CLOUD_SERVER_CLANG_DOCKER_FILE: docker/apps/Dockerfile.cloud-server-debug-clang CLOUD_SERVER_DOCKER_TAG: dbg jobs: @@ -78,7 +84,7 @@ jobs: build-args: | BUILD_ARGS=${{ inputs.build_args }} BUILD_TYPE=${{ inputs.build_type }} - file: ${{ env.CLOUD_SERVER_DOCKER_FILE }} + file: ${{ (inputs.clang && env.CLOUD_SERVER_CLANG_DOCKER_FILE) || env.CLOUD_SERVER_DOCKER_FILE }} tags: ${{ env.CLOUD_SERVER_DOCKER_TAG }} - name: Pull plgd hub tests image diff --git a/.github/workflows/plgd-hub-tests.yml b/.github/workflows/plgd-hub-tests.yml index 06bac5b205..8b43f86e4d 100644 --- a/.github/workflows/plgd-hub-tests.yml +++ b/.github/workflows/plgd-hub-tests.yml @@ -47,6 +47,8 @@ jobs: build_args: "-DOC_ASAN_ENABLED=ON" - name: cloud-server-tsan build_args: "-DOC_TSAN_ENABLED=ON" + # GCC thread-sanitizer keeps reporting false positives, so we use clang instead for tests with thread-sanitizer + clang: true - name: cloud-server-simulate-tpm-asan build_args: "-DOC_ASAN_ENABLED=ON" @@ -64,6 +66,7 @@ jobs: build_args: "-DOC_TSAN_ENABLED=ON" docker_args: '-e FAKETIME="@2000-01-01 11:12:13"' args: "--disable-tls-verify-time" + clang: true - name: cloud-server-time-2100-01-01 build_args: "" @@ -77,6 +80,7 @@ jobs: build_args: "-DOC_TSAN_ENABLED=ON" docker_args: '-e FAKETIME="@2100-01-01 11:12:13"' args: "--disable-tls-verify-time" + clang: true - name: cloud-server-set-mbedtls-time-2000-01-01 build_args: "" @@ -93,6 +97,7 @@ jobs: build_args: "-DOC_ASAN_ENABLED=ON -DOC_RESOURCE_ACCESS_IN_RFOTM_ENABLED=ON" - name: cloud-server-tsan-access-in-RFOTM build_args: "-DOC_TSAN_ENABLED=ON -DOC_RESOURCE_ACCESS_IN_RFOTM_ENABLED=ON" + clang: true - name: cloud-server-discovery-resource-observable build_args: "-DOC_DISCOVERY_RESOURCE_OBSERVABLE_ENABLED=ON" @@ -100,6 +105,7 @@ jobs: build_args: "-DOC_DISCOVERY_RESOURCE_OBSERVABLE_ENABLED=ON -DOC_ASAN_ENABLED=ON" - name: cloud-server-discovery-resource-observable-tsan build_args: "-DOC_DISCOVERY_RESOURCE_OBSERVABLE_ENABLED=ON -DOC_TSAN_ENABLED=ON" + clang: true - name: cloud-server-discovery-resource-observable-access-in-RFOTM # same configuration as "cloud-server-discovery-resource-observable-access-in-RFOTM" in the SonarCloud scan job, skip for events that trigger both jobs @@ -109,6 +115,7 @@ jobs: build_args: "-DOC_DISCOVERY_RESOURCE_OBSERVABLE_ENABLED=ON -DOC_ASAN_ENABLED=ON -DOC_RESOURCE_ACCESS_IN_RFOTM_ENABLED=ON" - name: cloud-server-discovery-resource-observable-tsan-access-in-RFOTM build_args: "-DOC_DISCOVERY_RESOURCE_OBSERVABLE_ENABLED=ON -DOC_TSAN_ENABLED=ON -DOC_RESOURCE_ACCESS_IN_RFOTM_ENABLED=ON" + clang: true - name: cloud-server-rep-realloc build_args: "-DOC_REPRESENTATION_REALLOC_ENCODING_ENABLED=ON" @@ -116,6 +123,7 @@ jobs: build_args: "-DOC_REPRESENTATION_REALLOC_ENCODING_ENABLED=ON -DOC_ASAN_ENABLED=ON" - name: cloud-server-rep-realloc-tsan build_args: "-DOC_REPRESENTATION_REALLOC_ENCODING_ENABLED=ON -DOC_TSAN_ENABLED=ON" + clang: true - name: cloud-server-discovery-resource-observable-access-in-RFOTM-rep-realloc build_args: "-DOC_DISCOVERY_RESOURCE_OBSERVABLE_ENABLED=ON -DOC_RESOURCE_ACCESS_IN_RFOTM_ENABLED=ON -DOC_REPRESENTATION_REALLOC_ENCODING_ENABLED=ON" @@ -123,6 +131,7 @@ jobs: build_args: "-DOC_DISCOVERY_RESOURCE_OBSERVABLE_ENABLED=ON -DOC_RESOURCE_ACCESS_IN_RFOTM_ENABLED=ON -DOC_REPRESENTATION_REALLOC_ENCODING_ENABLED=ON -DOC_ASAN_ENABLED=ON" - name: cloud-server-discovery-resource-observable-access-in-RFOTM-rep-realloc-tsan build_args: "-DOC_DISCOVERY_RESOURCE_OBSERVABLE_ENABLED=ON -DOC_RESOURCE_ACCESS_IN_RFOTM_ENABLED=ON -DOC_REPRESENTATION_REALLOC_ENCODING_ENABLED=ON -DOC_TSAN_ENABLED=ON" + clang: true # ports - name: cloud-server-tcp-disabled @@ -138,6 +147,7 @@ jobs: hub_args: "-e COAP_GATEWAY_UDP_ENABLED=true" - name: dtls-cloud-server-tsan build_args: "-DOC_TSAN_ENABLED=ON" + clang: true hub_args: "-e COAP_GATEWAY_UDP_ENABLED=true" - name: dtls-cloud-server-discovery-resource-observable @@ -148,6 +158,7 @@ jobs: hub_args: "-e COAP_GATEWAY_UDP_ENABLED=true" - name: dtls-cloud-server-discovery-resource-observable-tsan build_args: "-DOC_DISCOVERY_RESOURCE_OBSERVABLE_ENABLED=ON -DOC_TSAN_ENABLED=ON" + clang: true hub_args: "-e COAP_GATEWAY_UDP_ENABLED=true" - name: dtls-cloud-server-rep-realloc @@ -160,6 +171,7 @@ jobs: hub_args: "-e COAP_GATEWAY_UDP_ENABLED=true" - name: dtls-cloud-server-rep-realloc-tsan build_args: "-DOC_REPRESENTATION_REALLOC_ENCODING_ENABLED=ON -DOC_TSAN_ENABLED=ON" + clang: true hub_args: "-e COAP_GATEWAY_UDP_ENABLED=true" - name: dtls-cloud-server-discovery-resource-observable-rep-realloc @@ -170,6 +182,7 @@ jobs: hub_args: "-e COAP_GATEWAY_UDP_ENABLED=true" - name: dtls-cloud-server-discovery-resource-observable-rep-realloc-tsan build_args: "-DOC_DISCOVERY_RESOURCE_OBSERVABLE_ENABLED=ON -DOC_REPRESENTATION_REALLOC_ENCODING_ENABLED=ON -DOC_TSAN_ENABLED=ON" + clang: true hub_args: "-e COAP_GATEWAY_UDP_ENABLED=true" uses: ./.github/workflows/plgd-hub-test-with-cfg.yml @@ -182,4 +195,5 @@ jobs: docker_args: ${{ matrix.docker_args }} cert_signature_algorithm: ${{ (github.event_name == 'workflow_dispatch' && inputs.cert_signature_algorithm) || 'ECDSA-SHA256' }} cert_elliptic_curve: ${{ (github.event_name == 'workflow_dispatch' && inputs.cert_elliptic_curve) || 'P256' }} + clang: ${{ matrix.clang || false }} skip: ${{ matrix.skip || false }} diff --git a/api/oc_main.c b/api/oc_main.c index 9035f87bbb..3a65e795be 100644 --- a/api/oc_main.c +++ b/api/oc_main.c @@ -411,6 +411,12 @@ oc_main_poll(void) return (oc_clock_time_t)((int64_t)(next_event_mt - now_mt) + (int64_t)now); } +bool +oc_main_needs_poll(void) +{ + return oc_process_needs_poll(); +} + void oc_main_shutdown(void) { diff --git a/api/unittest/maintest.cpp b/api/unittest/maintest.cpp new file mode 100644 index 0000000000..ead6d2fcff --- /dev/null +++ b/api/unittest/maintest.cpp @@ -0,0 +1,318 @@ +/****************************************************************** + * + * Copyright 2018 Samsung Electronics All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"), + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + ******************************************************************/ + +#include "api/oc_event_callback_internal.h" +#include "api/oc_events_internal.h" +#include "api/oc_main_internal.h" +#include "api/oc_runtime_internal.h" +#include "oc_config.h" +#include "port/oc_log_internal.h" +#include "util/oc_atomic.h" +#include "util/oc_etimer_internal.h" +#include "util/oc_process_internal.h" + +#include +#include +#include + +#ifdef _WIN32 +#include +#else /* !_WIN32 */ +#include +#endif /* _WIN32 */ + +using namespace std::chrono_literals; + +OC_PROCESS(test_process, "Testing process in a worker thread"); + +class TestMain : public testing::Test { +public: + static void SetUpTestCase() + { +#ifdef _WIN32 + InitializeCriticalSection(&mutex); + InitializeConditionVariable(&cv); +#else + if (pthread_mutex_init(&mutex, nullptr) != 0) { + throw std::string("cannot initialize mutex"); + } + pthread_condattr_t attr; + if (pthread_condattr_init(&attr) != 0) { + throw std::string("cannot attributes of conditional variable"); + } + if (pthread_condattr_setclock(&attr, CLOCK_MONOTONIC) != 0) { + throw std::string("cannot configure clockid"); + } + if (pthread_cond_init(&cv, &attr) != 0) { + throw std::string("cannot initialize conditional variable"); + } + pthread_condattr_destroy(&attr); +#endif /* _WIN32 */ + + oc_runtime_init(); + oc_process_init(); + oc_event_assign_oc_process_events(); + oc_process_start(&oc_etimer_process, nullptr); + oc_event_callbacks_process_start(); + oc_process_start(&test_process, nullptr); + } + + static void TearDownTestCase() + { + oc_process_exit(&test_process); + oc_event_callbacks_process_exit(); + oc_process_exit(&oc_etimer_process); + oc_process_shutdown(); + oc_runtime_shutdown(); + +#ifndef _WIN32 + pthread_cond_destroy(&cv); + pthread_mutex_destroy(&mutex); +#endif /* _WIN32 */ + } + + void SetUp() override + { + OC_ATOMIC_STORE8(TestMain::terminate, 0); + OC_ATOMIC_STORE8(TestMain::needsPoll, 0); + OC_ATOMIC_STORE32(TestMain::pollCounter, 0); + OC_ATOMIC_STORE32(TestMain::eventCounter, 0); + } + + void Lock(); + void Unlock(); + void PoolEventsMs(uint64_t mseconds); + void SignalEventLoop(); + void Terminate(); + void WaitForEvent(oc_clock_time_t next_event_mt); + + void testSignalEventLoopinThreadWithMainLoop( + const std::function &mainLoop); + +#ifdef _WIN32 + static CRITICAL_SECTION mutex; + static CONDITION_VARIABLE cv; +#else /* !_WIN32 */ + static pthread_mutex_t mutex; + static pthread_cond_t cv; +#endif /* _WIN32 */ + static OC_ATOMIC_UINT8_T terminate; + + static OC_ATOMIC_UINT8_T needsPoll; + static OC_ATOMIC_UINT32_T pollCounter; + static OC_ATOMIC_UINT32_T eventCounter; +}; + +#ifdef _WIN32 +CRITICAL_SECTION TestMain::mutex; +CONDITION_VARIABLE TestMain::cv; +#else /* !_WIN32 */ +pthread_mutex_t TestMain::mutex; +pthread_cond_t TestMain::cv; +#endif /* _WIN32 */ +OC_ATOMIC_UINT8_T TestMain::terminate = 0; + +OC_ATOMIC_UINT8_T TestMain::needsPoll = 0; +OC_ATOMIC_UINT32_T TestMain::pollCounter = 0; +OC_ATOMIC_UINT32_T TestMain::eventCounter = 0; + +OC_PROCESS_THREAD(test_process, ev, data) +{ + (void)data; + (void)ev; + OC_PROCESS_POLLHANDLER([]() { + OC_DBG("polling(%u)", OC_ATOMIC_LOAD32(TestMain::pollCounter)); + OC_ATOMIC_STORE8(TestMain::needsPoll, 0); + OC_ATOMIC_INCREMENT32(TestMain::pollCounter); + }()); + OC_PROCESS_BEGIN(); + while (oc_process_is_running(&test_process)) { + OC_PROCESS_YIELD(); + OC_DBG("received event(%u)", OC_ATOMIC_LOAD32(TestMain::eventCounter)); + OC_ATOMIC_INCREMENT32(TestMain::eventCounter); + } + OC_PROCESS_END(); +} + +void +TestMain::Lock() +{ +#ifdef _WIN32 + EnterCriticalSection(&TestMain::mutex); +#else /* !_WIN32 */ + pthread_mutex_lock(&TestMain::mutex); +#endif /* _WIN32 */ +} + +void +TestMain::Unlock() +{ +#ifdef _WIN32 + LeaveCriticalSection(&TestMain::mutex); +#else /* !_WIN32 */ + pthread_mutex_unlock(&TestMain::mutex); +#endif /* _WIN32 */ +} + +void +TestMain::WaitForEvent(oc_clock_time_t next_event_mt) +{ +#ifdef _WIN32 + if (next_event_mt == 0) { + SleepConditionVariableCS(&TestMain::cv, &TestMain::mutex, INFINITE); + return; + } + oc_clock_time_t now_mt = oc_clock_time_monotonic(); + if (now_mt < next_event_mt) { + SleepConditionVariableCS( + &TestMain::cv, &TestMain::mutex, + (DWORD)((next_event_mt - now_mt) * 1000 / OC_CLOCK_SECOND)); + } +#else /* !_WIN32 */ + if (next_event_mt == 0) { + pthread_cond_wait(&TestMain::cv, &TestMain::mutex); + return; + } + struct timespec next_event = { 0, 0 }; + if (oc_clock_time_t next_event_cv; oc_clock_monotonic_time_to_posix( + next_event_mt, CLOCK_MONOTONIC, &next_event_cv)) { + next_event = oc_clock_time_to_timespec(next_event_cv); + } + pthread_cond_timedwait(&TestMain::cv, &TestMain::mutex, &next_event); +#endif /* _WIN32 */ +} + +void +TestMain::SignalEventLoop() +{ + // we need to lock the main loop mutex to synchronize this call with + // oc_process_nevents() in the main loop, without it we could miss events + Lock(); +#ifdef _WIN32 + WakeConditionVariable(&TestMain::cv); +#else /* !_WIN32 */ + pthread_cond_signal(&TestMain::cv); +#endif /* _WIN32 */ + Unlock(); +} + +void +TestMain::Terminate() +{ + OC_ATOMIC_STORE8(TestMain::terminate, 1); + SignalEventLoop(); +} + +static constexpr int kRepeats = 3000; + +static void * +pollProcessAndSignal(void *data) +{ + auto *instance = static_cast(data); + for (int i = 0; i < kRepeats && OC_ATOMIC_LOAD8(TestMain::terminate) == 0; + ++i) { + OC_DBG("request poll"); + OC_ATOMIC_STORE8(TestMain::needsPoll, 1); + oc_process_poll(&test_process); + instance->SignalEventLoop(); + while (OC_ATOMIC_LOAD8(TestMain::terminate) == 0 && + OC_ATOMIC_LOAD8(TestMain::needsPoll) != 0) { + continue; + } + } + instance->Terminate(); + return nullptr; +} + +#ifdef _WIN32 + +static DWORD +pollProcessAndSignalWin32(LPVOID data) +{ + pollProcessAndSignal(data); + return 0; +} + +#endif /* _WIN32 */ + +void +TestMain::testSignalEventLoopinThreadWithMainLoop( + const std::function &mainLoop) +{ +#ifdef _WIN32 + DWORD worker_thread_id; + HANDLE worker_thread = CreateThread(nullptr, 0, pollProcessAndSignalWin32, + nullptr, 0, &worker_thread_id); + ASSERT_NE(worker_thread, nullptr); +#else /* !_WIN32 */ + pthread_t worker_thread; + ASSERT_EQ( + 0, pthread_create(&worker_thread, nullptr, pollProcessAndSignal, this)); +#endif /* _WIN32 */ + + auto timeout = 2000ms; + auto quit = [](void *data) { + auto *instance = static_cast(data); + instance->Terminate(); + return OC_EVENT_DONE; + }; + oc_set_delayed_callback_ms_v1(this, quit, timeout.count()); + + mainLoop(); + + oc_remove_delayed_callback(this, quit); + +#ifdef _WIN32 + WaitForSingleObject(worker_thread, INFINITE); +#else /* !_WIN32 */ + pthread_join(worker_thread, nullptr); +#endif /* _WIN32 */ + + ASSERT_EQ(kRepeats, OC_ATOMIC_LOAD32(TestMain::pollCounter)); +} + +TEST_F(TestMain, SignalEventLoopFromThread) +{ + testSignalEventLoopinThreadWithMainLoop([this]() { + while (OC_ATOMIC_LOAD8(TestMain::terminate) == 0) { + oc_clock_time_t next_event = oc_main_poll_v1(); + Lock(); + if (oc_main_needs_poll()) { + Unlock(); + continue; + } + if (OC_ATOMIC_LOAD8(TestMain::terminate) != 0) { + Unlock(); + break; + } + WaitForEvent(next_event); + Unlock(); + } + }); +} + +TEST_F(TestMain, NeedsPoll) +{ + oc_process_poll(&test_process); + + EXPECT_TRUE(oc_main_needs_poll()); + + while (oc_main_poll_v1() != 0) { + // no-op + } +} diff --git a/api/unittest/ocapitest.cpp b/api/unittest/ocapitest.cpp index 7eb974dfcb..3c3efa949a 100644 --- a/api/unittest/ocapitest.cpp +++ b/api/unittest/ocapitest.cpp @@ -241,11 +241,13 @@ class ApiHelper { static void signalEventLoop(void) { + lock(); #ifdef _WIN32 WakeConditionVariable(&s_cv); #else pthread_cond_signal(&s_cv); #endif /* _WIN32 */ + unlock(); } static oc_event_callback_retval_t quitEvent(void *) @@ -298,8 +300,12 @@ class ApiHelper { oc_set_delayed_callback_ms_v1(nullptr, quitEvent, msecs); while (OC_ATOMIC_LOAD8(s_terminate) == 0) { - lock(); oc_clock_time_t next_event = oc_main_poll_v1(); + lock(); + if (oc_main_needs_poll()) { + unlock(); + continue; + } if (OC_ATOMIC_LOAD8(s_terminate) != 0) { unlock(); break; diff --git a/apps/cloud_server.c b/apps/cloud_server.c index 4e217c6c51..0809cde3eb 100644 --- a/apps/cloud_server.c +++ b/apps/cloud_server.c @@ -26,8 +26,9 @@ #include "oc_pki.h" #include "oc_clock_util.h" #include "port/oc_assert.h" +#include "util/oc_compiler.h" #include "util/oc_features.h" -#include "util/oc_macros_internal.h" +#include "util/oc_process.h" #ifdef OC_HAS_FEATURE_PLGD_TIME #include "plgd/plgd_time.h" @@ -42,35 +43,38 @@ #include #endif /* _MSC_VER */ +#define ARRAY_SIZE(array) (sizeof(array) / sizeof((array)[0])) #define CHAR_ARRAY_LEN(x) (sizeof(x) - 1) -static bool quit = false; +static bool g_quit = false; #ifdef _WIN32 #include -static CONDITION_VARIABLE cv; -static CRITICAL_SECTION cs; +static CONDITION_VARIABLE g_cv; +static CRITICAL_SECTION g_cs; static void signal_event_loop(void) { - WakeConditionVariable(&cv); + EnterCriticalSection(&g_cs); + WakeConditionVariable(&g_cv); + LeaveCriticalSection(&g_cs); } static void handle_signal(int signal) { (void)signal; - quit = true; + g_quit = true; signal_event_loop(); } static int init(void) { - InitializeCriticalSection(&cs); - InitializeConditionVariable(&cv); + InitializeCriticalSection(&g_cs); + InitializeConditionVariable(&g_cv); signal(SIGINT, handle_signal); return 0; } @@ -84,19 +88,24 @@ deinit(void) static void run_loop(void) { - while (!quit) { - EnterCriticalSection(&cs); + while (!g_quit) { oc_clock_time_t next_event_mt = oc_main_poll_v1(); + EnterCriticalSection(&g_cs); + if (oc_main_needs_poll()) { + LeaveCriticalSection(&g_cs); + continue; + } if (next_event_mt == 0) { - SleepConditionVariableCS(&cv, &cs, INFINITE); + SleepConditionVariableCS(&g_cv, &g_cs, INFINITE); } else { oc_clock_time_t now_mt = oc_clock_time_monotonic(); if (now_mt < next_event_mt) { SleepConditionVariableCS( - &cv, &cs, (DWORD)((next_event_mt - now_mt) * 1000 / OC_CLOCK_SECOND)); + &g_cv, &g_cs, + (DWORD)((next_event_mt - now_mt) * 1000 / OC_CLOCK_SECOND)); } } - LeaveCriticalSection(&cs); + LeaveCriticalSection(&g_cs); } } @@ -107,13 +116,15 @@ run_loop(void) #include // suseconds_t #include -static pthread_mutex_t mutex; -static pthread_cond_t cv; +static pthread_mutex_t g_mutex; +static pthread_cond_t g_cv; static void signal_event_loop(void) { - pthread_cond_signal(&cv); + pthread_mutex_lock(&g_mutex); + pthread_cond_signal(&g_cv); + pthread_mutex_unlock(&g_mutex); } static void @@ -122,7 +133,7 @@ handle_signal(int signal) if (signal == SIGPIPE) { return; } - quit = true; + g_quit = true; signal_event_loop(); } @@ -137,7 +148,7 @@ init(void) sigaction(SIGPIPE, &sa, NULL); sigaction(SIGTERM, &sa, NULL); - int err = pthread_mutex_init(&mutex, NULL); + int err = pthread_mutex_init(&g_mutex, NULL); if (err != 0) { OC_PRINTF("ERROR: pthread_mutex_init failed (error=%d)!\n", err); return false; @@ -146,21 +157,21 @@ init(void) err = pthread_condattr_init(&attr); if (err != 0) { OC_PRINTF("ERROR: pthread_condattr_init failed (error=%d)!\n", err); - pthread_mutex_destroy(&mutex); + pthread_mutex_destroy(&g_mutex); return false; } err = pthread_condattr_setclock(&attr, CLOCK_MONOTONIC); if (err != 0) { OC_PRINTF("ERROR: pthread_condattr_setclock failed (error=%d)!\n", err); pthread_condattr_destroy(&attr); - pthread_mutex_destroy(&mutex); + pthread_mutex_destroy(&g_mutex); return false; } - err = pthread_cond_init(&cv, &attr); + err = pthread_cond_init(&g_cv, &attr); if (err != 0) { OC_PRINTF("ERROR: pthread_cond_init failed (error=%d)!\n", err); pthread_condattr_destroy(&attr); - pthread_mutex_destroy(&mutex); + pthread_mutex_destroy(&g_mutex); return false; } pthread_condattr_destroy(&attr); @@ -170,18 +181,22 @@ init(void) static void deinit(void) { - pthread_cond_destroy(&cv); - pthread_mutex_destroy(&mutex); + pthread_cond_destroy(&g_cv); + pthread_mutex_destroy(&g_mutex); } static void run_loop(void) { - while (!quit) { + while (!g_quit) { oc_clock_time_t next_event_mt = oc_main_poll_v1(); - pthread_mutex_lock(&mutex); + pthread_mutex_lock(&g_mutex); + if (oc_main_needs_poll()) { + pthread_mutex_unlock(&g_mutex); + continue; + } if (next_event_mt == 0) { - pthread_cond_wait(&cv, &mutex); + pthread_cond_wait(&g_cv, &g_mutex); } else { struct timespec next_event = { 1, 0 }; oc_clock_time_t next_event_cv; @@ -189,9 +204,9 @@ run_loop(void) &next_event_cv)) { next_event = oc_clock_time_to_timespec(next_event_cv); } - pthread_cond_timedwait(&cv, &mutex, &next_event); + pthread_cond_timedwait(&g_cv, &g_mutex, &next_event); } - pthread_mutex_unlock(&mutex); + pthread_mutex_unlock(&g_mutex); } } @@ -1507,7 +1522,7 @@ cloud_server_send_response_cb(oc_request_t *request, oc_status_t response_code) #ifdef OC_HAS_FEATURE_ETAG if (request->etag != NULL) { char buf[32]; - size_t buf_size = OC_ARRAY_SIZE(buf); + size_t buf_size = ARRAY_SIZE(buf); oc_conv_byte_array_to_hex_string(request->etag, request->etag_len, buf, &buf_size); OC_PRINTF(", etag [0x%s]", buf); diff --git a/docker/apps/Dockerfile.cloud-server-debug-clang b/docker/apps/Dockerfile.cloud-server-debug-clang new file mode 100644 index 0000000000..9ddadb3267 --- /dev/null +++ b/docker/apps/Dockerfile.cloud-server-debug-clang @@ -0,0 +1,34 @@ +# GCC thread-sanitizer keeps reporting false positives, so we use clang instead for tests with thread-sanitizer. +FROM ubuntu:22.04 AS service +ARG BUILD_TYPE=Release +ARG BUILD_ARGS +RUN apt-get update -y && \ + DEBIAN_FRONTEND="noninteractive" apt-get install -y bash ca-certificates cmake curl gdb git-core gcovr g++ make patch python3 --no-install-recommends +RUN DEBIAN_FRONTEND="noninteractive" apt-get install -y clang-15 +RUN update-alternatives --install /usr/bin/clang clang /usr/bin/clang-15 100 && \ + update-alternatives --install /usr/bin/clang++ clang++ /usr/bin/clang++-15 100 +COPY ./ /iotivity-lite/ +RUN cd /iotivity-lite/ && git submodule update --recursive +RUN mkdir /iotivity-lite/build && \ + cd /iotivity-lite/build && \ + cmake -DCMAKE_BUILD_TYPE=${BUILD_TYPE} -DCMAKE_VERBOSE_MAKEFILE=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C_COMPILER=clang -DBUILD_TESTING=OFF -DOC_CLOUD_ENABLED=ON ${BUILD_ARGS} .. && \ + cmake --build . --target cloud_server +RUN cp /iotivity-lite/build/apps/cloud_server /iotivity-lite/port/linux/service + +# install libfaketime +RUN git clone https://github.com/wolfcw/libfaketime.git && \ + cd /libfaketime/src && \ + make install FAKETIME_COMPILE_CFLAGS="-DFAKE_SETTIME -DFAKE_STATELESS" + +COPY /docker/logbt /usr/local/bin/logbt +RUN logbt --version +COPY /docker/run.sh /usr/local/bin/run.sh +ENV NUM_DEVICES=1 +ENV FAKETIME= +ENV FAKETIME_DONT_FAKE_MONOTONIC=1 +ENV FAKETIME_TIMESTAMP_FILE= +ENV FAKETIME_UPDATE_TIMESTAMP_FILE= +ENV FAKETIME_DONT_RESET= +ENV FAKETIME_NO_CACHE= +ENV FAKETIME_CACHE_DURATION= +ENTRYPOINT ["/usr/local/bin/run.sh"] diff --git a/include/oc_api.h b/include/oc_api.h index 44cdc62ba2..f5491e351e 100644 --- a/include/oc_api.h +++ b/include/oc_api.h @@ -291,6 +291,19 @@ OC_API oc_clock_time_t oc_main_poll(void) OC_DEPRECATED("replaced by oc_main_poll_v1 in v2.2.5.6"); +/** + * @brief Check if process polling was requested + * + * @return true A polling of processes was requested by a call to + * oc_process_poll and oc_main_poll_v1 should be called. + * @return false Otherwise + * + * @sa oc_main_poll + * @sa oc_process_poll + */ +OC_API +bool oc_main_needs_poll(void); + /** * Shutdown and free all stack related resources */ diff --git a/port/esp32/main/main.c b/port/esp32/main/main.c index 3568917e8a..c2f02e7f0a 100644 --- a/port/esp32/main/main.c +++ b/port/esp32/main/main.c @@ -59,8 +59,8 @@ static EventGroupHandle_t wifi_event_group; static const int IPV4_CONNECTED_BIT = BIT0; static const int IPV6_CONNECTED_BIT = BIT1; -static pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER; -static pthread_cond_t cv = PTHREAD_COND_INITIALIZER; +static pthread_mutex_t g_mutex = PTHREAD_MUTEX_INITIALIZER; +static pthread_cond_t g_cv = PTHREAD_COND_INITIALIZER; static struct timespec ts; static int quit = 0; static bool light_state = false; @@ -172,7 +172,9 @@ register_resources(void) static void signal_event_loop(void) { - pthread_cond_signal(&cv); + pthread_mutex_lock(&g_mutex); + pthread_cond_signal(&g_cv); + pthread_mutex_unlock(&g_mutex); } static void @@ -571,14 +573,18 @@ server_main(void *pvParameter) while (quit != 1) { oc_clock_time_t next_event = oc_main_poll(); - pthread_mutex_lock(&mutex); + pthread_mutex_lock(&g_mutex); + if (oc_main_needs_poll()) { + pthread_mutex_unlock(&g_mutex); + continue; + } if (next_event == 0) { - pthread_cond_wait(&cv, &mutex); + pthread_cond_wait(&g_cv, &g_mutex); } else { ts = oc_clock_time_to_timespec(next_event); - pthread_cond_timedwait(&cv, &mutex, &ts); + pthread_cond_timedwait(&g_cv, &g_mutex, &ts); } - pthread_mutex_unlock(&mutex); + pthread_mutex_unlock(&g_mutex); } #ifdef OC_HAS_FEATURE_PLGD_HAWKBIT @@ -603,7 +609,7 @@ app_main(void) gpio_reset_pin(BLINK_GPIO); gpio_set_direction(BLINK_GPIO, GPIO_MODE_OUTPUT); - pthread_cond_init(&cv, NULL); + pthread_cond_init(&g_cv, NULL); print_macro_info(); diff --git a/tests/gtest/Device.cpp b/tests/gtest/Device.cpp index dc5da75cdc..9b6016d07a 100644 --- a/tests/gtest/Device.cpp +++ b/tests/gtest/Device.cpp @@ -133,11 +133,13 @@ Device::~Device() void Device::SignalEventLoop() { + Lock(); #ifdef _WIN32 WakeConditionVariable(&cv_); #else pthread_cond_signal(&cv_); #endif /* _WIN32 */ + Unlock(); } void @@ -217,8 +219,12 @@ Device::PoolEventsMs(uint64_t mseconds, bool addDelay) oc_set_delayed_callback_ms_v1(this, Device::QuitEvent, interval); while (OC_ATOMIC_LOAD8(terminate_) == 0) { - Lock(); oc_clock_time_t next_event = oc_main_poll_v1(); + Lock(); + if (oc_main_needs_poll()) { + Unlock(); + continue; + } if (OC_ATOMIC_LOAD8(terminate_) != 0) { Unlock(); break; diff --git a/tests/gtest/Device.h b/tests/gtest/Device.h index 2e428a7fe4..2333d33dd5 100644 --- a/tests/gtest/Device.h +++ b/tests/gtest/Device.h @@ -26,7 +26,7 @@ #include "util/oc_atomic.h" #include "util/oc_features.h" -#if defined(_WIN32) +#ifdef _WIN32 #include #else /* !_WIN32 */ #include diff --git a/util/oc_compiler.h b/util/oc_compiler.h index a4daaf2dd5..c8c7ed6bac 100644 --- a/util/oc_compiler.h +++ b/util/oc_compiler.h @@ -67,6 +67,12 @@ #define OC_RETURNS_NONNULL #endif +#if defined(__clang__) || defined(__GNUC__) +#define OC_NO_SANITIZE(...) __attribute__((no_sanitize(__VA_ARGS__))) +#else +#define OC_NO_SANITIZE(...) +#endif + #if defined(__clang__) || defined(__GNUC__) #if defined(__MINGW32__) && defined(__USE_MINGW_ANSI_STDIO) && \ __USE_MINGW_ANSI_STDIO == 1 diff --git a/util/oc_process.c b/util/oc_process.c index 5fc2e0e67b..0834cf7315 100644 --- a/util/oc_process.c +++ b/util/oc_process.c @@ -362,6 +362,12 @@ oc_process_nevents(void) return (int)g_nevents + OC_ATOMIC_LOAD8(g_poll_requested); } +bool +oc_process_needs_poll(void) +{ + return OC_ATOMIC_LOAD8(g_poll_requested) != 0; +} + #ifdef OC_SECURITY bool oc_process_is_closing_all_tls_sessions(void) @@ -486,7 +492,7 @@ oc_process_poll(struct oc_process *p) int oc_process_is_running(const struct oc_process *p) { - return OC_ATOMIC_LOAD8(p->state) != OC_PROCESS_STATE_NONE; + return OC_ATOMIC_LOAD8(p->state) != OC_PROCESS_STATE_NONE ? 1 : 0; } #ifdef OC_TEST diff --git a/util/oc_process.h b/util/oc_process.h index f62e9863a0..1e6f05aadf 100644 --- a/util/oc_process.h +++ b/util/oc_process.h @@ -541,6 +541,13 @@ int oc_process_is_running(const struct oc_process *p); */ int oc_process_nevents(void); +/** + * Check if processes need to be polled. + * + * \return True if there are processes that need to be polled. + */ +bool oc_process_needs_poll(void); + #ifdef OC_SECURITY /** diff --git a/util/unittest/etimertest.cpp b/util/unittest/etimertest.cpp index 217c7ba072..5d95ad2bf3 100644 --- a/util/unittest/etimertest.cpp +++ b/util/unittest/etimertest.cpp @@ -380,15 +380,10 @@ TEST_F(TestEventTimer, Adjust) oc_etimer_set(&et, interval); OC_PROCESS_CONTEXT_END(&oc_test_process_1) - TestEventTimer::Poll(); - ASSERT_FALSE(oc_etimer_expired(&et)); - - oc_clock_time_t wait = interval + interval / 2; - oc_etimer_adjust(&et, static_cast(wait)); - oc_clock_wait(wait); - TestEventTimer::Poll(); - EXPECT_FALSE(oc_etimer_expired(&et)); - EXPECT_TRUE(oc_etimer_pending()); + oc_clock_time_t next = TestEventTimer::Poll(); + oc_etimer_adjust(&et, static_cast(interval)); + oc_clock_time_t next_adjusted = TestEventTimer::Poll(); + EXPECT_LT(next, next_adjusted); oc_etimer_stop(&et); } diff --git a/util/unittest/processtest.cpp b/util/unittest/processtest.cpp index c1f7ae9771..372b69980c 100644 --- a/util/unittest/processtest.cpp +++ b/util/unittest/processtest.cpp @@ -18,6 +18,7 @@ #include "api/oc_buffer_internal.h" #include "api/oc_events_internal.h" +#include "port/oc_log_internal.h" #include "tests/gtest/Device.h" #include "util/oc_process.h" #include "util/oc_process_internal.h" @@ -33,6 +34,20 @@ using namespace std::chrono_literals; static constexpr size_t kDeviceID{ 0 }; +OC_PROCESS(test_process, "Testing process"); + +OC_PROCESS_THREAD(test_process, ev, data) +{ + (void)data; + OC_PROCESS_POLLHANDLER([]() { OC_DBG("polling"); }()); + OC_PROCESS_BEGIN(); + while (oc_process_is_running(&test_process)) { + OC_PROCESS_YIELD(); + OC_DBG("received event(%d)", (int)ev); + } + OC_PROCESS_END(); +} + class TestProcess : public testing::Test { public: static void SetUpTestCase() @@ -41,9 +56,28 @@ class TestProcess : public testing::Test { oc_event_assign_oc_process_events(); } - static void TearDownTestCase() { oc_process_shutdown(); } + static void TearDownTestCase() + { + oc_process_exit(&test_process); + oc_process_shutdown(); + } }; +TEST_F(TestProcess, Start) +{ + EXPECT_EQ(0, oc_process_is_running(&test_process)); + + oc_process_start(&test_process, nullptr); + EXPECT_EQ(1, oc_process_is_running(&test_process)); + + // multiple starts are ignored + oc_process_start(&test_process, nullptr); + EXPECT_EQ(1, oc_process_is_running(&test_process)); + + oc_process_exit(&test_process); + EXPECT_EQ(0, oc_process_is_running(&test_process)); +} + #ifdef OC_SECURITY TEST_F(TestProcess, IsClosingTLSSessions_F)