Skip to content

Commit

Permalink
Refactor commit message for clarity and conciseness:
Browse files Browse the repository at this point in the history
**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.
  • Loading branch information
Danielius1922 committed Sep 6, 2023
1 parent 462fa30 commit 979768f
Show file tree
Hide file tree
Showing 17 changed files with 490 additions and 43 deletions.
2 changes: 2 additions & 0 deletions .github/workflows/cmake-linux.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 7 additions & 1 deletion .github/workflows/plgd-device-test-with-cfg.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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:
Expand All @@ -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 }}
Expand Down
8 changes: 8 additions & 0 deletions .github/workflows/plgd-device-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -49,34 +49,40 @@ 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"
- name: cloud-server-asan-access-in-RFOTM
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"
- name: cloud-server-discovery-resource-observable-asan
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"
- name: cloud-server-discovery-resource-observable-asan-access-in-RFOTM
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"
- name: cloud-server-rep-realloc-asan
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
Expand All @@ -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:
Expand All @@ -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 }}
8 changes: 7 additions & 1 deletion .github/workflows/plgd-hub-test-with-cfg.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand All @@ -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
Expand Down
14 changes: 14 additions & 0 deletions .github/workflows/plgd-hub-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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: ""
Expand All @@ -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: ""
Expand All @@ -93,13 +97,15 @@ 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"
- name: cloud-server-discovery-resource-observable-asan
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
Expand All @@ -109,20 +115,23 @@ 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"
- name: cloud-server-rep-realloc-asan
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"
- name: cloud-server-discovery-resource-observable-access-in-RFOTM-rep-realloc-asan
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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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 }}
6 changes: 6 additions & 0 deletions api/oc_main.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
Loading

0 comments on commit 979768f

Please sign in to comment.