Fix another deadlock in sensors system

[iche033]

🦟 Bug fix

Summary

This PR:

• Fixed a situation deadlock that occurs when waiting on the updateAvailable variable with a condition variable while that variable is also being written.
• Reduced scope of a lock (timeLock). Not needed for fixing deadlock - can be done in a separate PR if needed.
  • Update: Reverted these changes.

More info on the deadlock:

These two operations can happen concurrently (from 2 threads):

        std::unique_lock<std::mutex> cvLock(this->dataPtr->renderMutex);
        // updateAvailable is true here
        this->dataPtr->renderCv.wait(cvLock, [this] {
          return !this->dataPtr->running || !this->dataPtr->updateAvailable; });

        // updateAvailable is set to false
        this->dataPtr->updateAvailable = true;
        this->dataPtr->renderCv.notify_one();

The wait condition may miss the notify_one() call causing it to sleep forever. To solve this, we need to make sure these two groups of operations are performed one after another. This fix is to group and lock the updateAvailable = [true|false] and notify_one calls using the same lock that's used by the wait condition variable.

Test it

Ran into this deadlock when adding the DvL sensor system to the Harmonic demo world. The DVL sensor is implemented as a custom rendering sensor. It changes the timing of the sensors system which revealed this deadlock issue.

Test with this pull request that adds a DVL sensor to the harmonic demo world:
gazebosim/harmonic_demo#9

Without the changes in this PR, the deadlock happens within 1 minute of running the simulation for me.

Checklist

• Signed all commits for DCO
• Added tests
• Updated documentation (as needed)
• Updated migration guide (as needed)
• Consider updating Python bindings (if the library has them)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

[codecov]

Codecov Report

Merging #2141 (da25558) into gz-sim8 (e9fa7d4) will decrease coverage by 0.02%.
Report is 2 commits behind head on gz-sim8.
The diff coverage is 100.00%.

❗ Current head da25558 differs from pull request most recent head b8ba42f. Consider uploading reports for the commit b8ba42f to get more accurate results

@@             Coverage Diff             @@
##           gz-sim8    #2141      +/-   ##
===========================================
- Coverage    65.47%   65.45%   -0.02%     
===========================================
  Files          323      323              
  Lines        30704    30708       +4     
===========================================
- Hits         20102    20101       -1     
- Misses       10602    10607       +5     

Files Changed	Coverage Δ
src/systems/sensors/Sensors.cc	63.56% <100.00%> (+0.38%)	⬆️

... and 2 files with indirect coverage changes

[mjcarroll]

This causes SensorsFixture/UpdateRate to fail, which is a bit concerning.

[iche033]

This causes SensorsFixture/UpdateRate to fail, which is a bit concerning.

I ran CI again and confirmed that it failed again. I reverted the lock scope changes in b8ba42f and that seems to fix it. I ran CI two more times and test passes.