Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix race condition in main loop #520

Merged
merged 1 commit into from
Sep 6, 2023

Conversation

Danielius1922
Copy link
Member

No description provided.

@ocf-conformance-test-tool
Copy link

🎉 Thank you for your code contribution! To guarantee the change/addition is conformant to the OCF Specification, we would like to ask you to execute OCF Conformance Testing of your change ☝️ when your work is ready to be reviewed.


ℹ️ To verify your latest change (277aab3), label this PR with OCF Conformance Testing.

⚠️ Label is removed with every code change.

@Danielius1922 Danielius1922 force-pushed the adam/bugfix/519-fix-race-cond-in-mainloop branch from dda13ca to f5a57a6 Compare September 4, 2023 21:46
@Danielius1922 Danielius1922 added the OCF Conformance Testing OCF Conformance Testing required label Sep 4, 2023
@Danielius1922 Danielius1922 force-pushed the adam/bugfix/519-fix-race-cond-in-mainloop branch from f5a57a6 to ee2a4fe Compare September 5, 2023 06:49
@ocf-conformance-test-tool ocf-conformance-test-tool bot removed the OCF Conformance Testing OCF Conformance Testing required label Sep 5, 2023
@Danielius1922 Danielius1922 force-pushed the adam/bugfix/519-fix-race-cond-in-mainloop branch from ee2a4fe to 52fce5c Compare September 5, 2023 07:44
@Danielius1922 Danielius1922 added the OCF Conformance Testing OCF Conformance Testing required label Sep 5, 2023
@Danielius1922 Danielius1922 marked this pull request as ready for review September 5, 2023 09:19
apps/cloud_server.c Outdated Show resolved Hide resolved
@Danielius1922 Danielius1922 force-pushed the adam/bugfix/519-fix-race-cond-in-mainloop branch from 52fce5c to ba547ac Compare September 5, 2023 17:12
@ocf-conformance-test-tool ocf-conformance-test-tool bot removed the OCF Conformance Testing OCF Conformance Testing required label Sep 5, 2023
@Danielius1922 Danielius1922 force-pushed the adam/bugfix/519-fix-race-cond-in-mainloop branch from ba547ac to d7b7eac Compare September 5, 2023 17:21
@Danielius1922 Danielius1922 added the OCF Conformance Testing OCF Conformance Testing required label Sep 5, 2023
@Danielius1922 Danielius1922 force-pushed the adam/bugfix/519-fix-race-cond-in-mainloop branch from d7b7eac to 979768f Compare September 6, 2023 07:19
@ocf-conformance-test-tool ocf-conformance-test-tool bot removed the OCF Conformance Testing OCF Conformance Testing required label Sep 6, 2023
apps/cloud_server.c Outdated Show resolved Hide resolved
**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.
@Danielius1922 Danielius1922 force-pushed the adam/bugfix/519-fix-race-cond-in-mainloop branch from a6ded41 to 63f3c5a Compare September 6, 2023 11:57
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@Danielius1922 Danielius1922 merged commit b142614 into master Sep 6, 2023
@Danielius1922 Danielius1922 deleted the adam/bugfix/519-fix-race-cond-in-mainloop branch September 6, 2023 13:00
@github-actions github-actions bot locked and limited conversation to collaborators Sep 6, 2023
@Danielius1922 Danielius1922 linked an issue Sep 6, 2023 that may be closed by this pull request
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Race condition when polling an oc_process
2 participants