Fixed wait_for_initialization for multiple threads #12

paulbourelly999 · 2023-06-29T18:47:03Z

PR Details

Description

Fix carma-clock object to support multiple threads waiting on initialization. Also added method to sleep_for(milliseconds)

Related GitHub Issue

#10

Related Jira Key

VH-1230

https://usdot-carma.atlassian.net/browse/VH-1230

Motivation and Context

Fix carma-clock which enables time-sync for simulation

How Has This Been Tested?

Unit testing

Types of changes

[x ] Defect fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that cause existing functionality to change)

Checklist:

I have added any new packages to the sonar-scanner.properties file
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

dan-du-car · 2023-06-30T17:01:20Z

src/carma_clock.cpp

@@ -61,7 +61,8 @@ void CarmaClock::update(timeStampMilliseconds current_time) {
        // if not initialized then do it and let anyone waiting know
        _is_initialized = true;
        std::unique_lock lock(_initialization_mutex);
-        _initialization_cv.notify_one();
+        // Notify all blocked threads
+        _initialization_cv.notify_all();


Why do we choose notify all over one? Is there a scenario that we are testing require it to notify all upon an update?

the conditional variable here can be used to block one our many threads with the wait call. Notify One will only notify a single thread. If multiple threads are waiting on initialization, only 1 will be notified here. The desired behavior is to notify and unblock all threads as shown in my unit test. (https://en.cppreference.com/w/cpp/thread/condition_variable)

dan-du-car · 2023-06-30T17:17:29Z

src/carma_clock.cpp

@@ -85,6 +86,10 @@ void CarmaClock::wait_for_initialization() {
    }
 }

+void CarmaClock::sleep_for(timeStampMilliseconds time_to_sleep) {
+    sleep_until( _current_time + time_to_sleep);


In the sleep_until function, there is a wait call sleepCVPairValue.first->wait(lock); How does that get notified?

In the update function there is a mechanism that notifies each conditional variable based on whether the most recent update is larger or equal to the sleep until value.

Can we added description about those two conditional variables _initialization_cv and sleepCVPair. What they do and why they are needed?

Added addition comments to header with descriptions of the variable used.

dan-du-car · 2023-06-30T17:19:15Z

Error trying to build with cmake:

This is meant to be build using the https://github.com/usdot-fhwa-stol/carma-builds as a base image. That is why it also has a dev-container setup that pulls the image for you. This is something Rob setup to make standing up independent libraries easier and more reproducible.

dan-du-car · 2023-07-05T17:13:46Z

test/test_carma_clock.cpp

+TEST(test_carma_clock, test_sim_time_initialization_multiple_threads)
+{
+    // try a sim clock
+    CarmaClock clock(true);


CarmaClock clock(true); clock.update(1);

After calling the update (1), the _current_time is 1, and _is_initialized = true. The _initialization_cv will notify_all().
Then thread t sleep for every 150 milliseconds and call update(2), update(3), update(4).
I am assuming the _current_time - 4 by now? Also wait_for_initialization() is not executed as it is only executed when the _is_initialized = false.
Why do we call the
wait_for_initialization() if it is already initialized?

I am able to run the unit test and see the _current_time run differently from system clock time. Maybe we have a function to return the _current_time, and ASSERT the _current_time change instead of the system clock time? I guess if we run this carma-clock in simulated time, we care more about the simulated time change.

I am not sure you understand this unit test. The first thread is sleeping for .15 seconds and then calling update, the other 3 are called immediately. They are blocked for .15 seconds until the update is called from the other thread, which notifies all of them. If you add cout statements you can confirm this in the update method by checking when threads are notified.

In other words all the code inside the 4 threads is executed in parallel.

dan-du-car · 2023-07-05T17:32:16Z

test/test_carma_clock.cpp

+    auto msCount = duration_cast<milliseconds>(after - start).count();
+    t.join();
+    // should have happened after the update occurred in other thread
+    EXPECT_NEAR(SYSTEM_SLEEP_TIME, msCount, 5);


Shouldn't this be 3 instead of 5?
line 195: clock.update(1), wait SYSTEM_SLEEP_TIME, then clock.update(2); The current_time should be 2. Then clock.sleep_for(1), the clock wait for 1 millisecond on top of current time = 2. so the sleep in total may be 3 milliseconds?

This 5 is the discrepancy for the unit test. Depending on machine resources this unit test may run faster or slower. The msCount is the difference in machine time for how long the sleep_for command waits. In this case is should wait for SYSTEM_SLEEP_TIME (150 ms) since that is how long it takes for the 1ms time step to be updated. We are leaving a 5ms (machine time) +- buffer to account for resource limitations, especially when run in CI/CD environment (Github actions)

sonarcloud · 2023-07-05T20:35:04Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

Fixed wait_for_initialization for multiple threads

37d8c94

paulbourelly999 requested review from kjrush and dan-du-car June 29, 2023 18:47

Updated fix to use notify all instead of vector

04217ab

dan-du-car reviewed Jun 30, 2023

View reviewed changes

dan-du-car reviewed Jul 5, 2023

View reviewed changes

Added javadoc comments

137bff2

dan-du-car approved these changes Jul 6, 2023

View reviewed changes

dan-du-car merged commit 107edbf into develop Jul 6, 2023
54 checks passed

paulbourelly999 deleted the 10-wait_for_initialization_multi-thread-fix branch September 18, 2023 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed wait_for_initialization for multiple threads #12

Fixed wait_for_initialization for multiple threads #12

paulbourelly999 commented Jun 29, 2023

dan-du-car Jun 30, 2023

paulbourelly999 Jul 3, 2023

dan-du-car Jun 30, 2023

paulbourelly999 Jul 3, 2023

dan-du-car Jul 5, 2023

paulbourelly999 Jul 5, 2023

dan-du-car commented Jun 30, 2023 •

edited by paulbourelly999

Loading

dan-du-car Jul 5, 2023 •

edited

Loading

dan-du-car Jul 5, 2023 •

edited

Loading

paulbourelly999 Jul 5, 2023

dan-du-car Jul 5, 2023 •

edited

Loading

paulbourelly999 Jul 5, 2023 •

edited

Loading

sonarcloud bot commented Jul 5, 2023

Fixed wait_for_initialization for multiple threads #12

Fixed wait_for_initialization for multiple threads #12

Conversation

paulbourelly999 commented Jun 29, 2023

PR Details

Description

Related GitHub Issue

Related Jira Key

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

dan-du-car Jun 30, 2023

Choose a reason for hiding this comment

paulbourelly999 Jul 3, 2023

Choose a reason for hiding this comment

dan-du-car Jun 30, 2023

Choose a reason for hiding this comment

paulbourelly999 Jul 3, 2023

Choose a reason for hiding this comment

dan-du-car Jul 5, 2023

Choose a reason for hiding this comment

paulbourelly999 Jul 5, 2023

Choose a reason for hiding this comment

dan-du-car commented Jun 30, 2023 • edited by paulbourelly999 Loading

dan-du-car Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

dan-du-car Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

paulbourelly999 Jul 5, 2023

Choose a reason for hiding this comment

dan-du-car Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

paulbourelly999 Jul 5, 2023 • edited Loading

Choose a reason for hiding this comment

sonarcloud bot commented Jul 5, 2023

dan-du-car commented Jun 30, 2023 •

edited by paulbourelly999

Loading

dan-du-car Jul 5, 2023 •

edited

Loading

dan-du-car Jul 5, 2023 •

edited

Loading

dan-du-car Jul 5, 2023 •

edited

Loading

paulbourelly999 Jul 5, 2023 •

edited

Loading