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

Thread safety in custom rendering sensor system (DopplerVelocityLogSystem.cc) #2289

Closed
5p00kk opened this issue Jan 17, 2024 · 4 comments · Fixed by #2345
Closed

Thread safety in custom rendering sensor system (DopplerVelocityLogSystem.cc) #2289

5p00kk opened this issue Jan 17, 2024 · 4 comments · Fixed by #2345
Assignees
Labels
bug Something isn't working

Comments

@5p00kk
Copy link

5p00kk commented Jan 17, 2024

Environment

Description

In DopplerVelocityLogSystem.cc there are time-related variables: nextUpdateTime, simTime. They appear to be used by both: the rendering thread (OnRedner, PostRender) and physics thread (DoPostUpdate). There seems to be no thread safety for these variables, compared to for instance pendingRequests which is atomic.

@5p00kk 5p00kk added the bug Something isn't working label Jan 17, 2024
@azeey azeey self-assigned this Jan 22, 2024
@azeey azeey moved this from Inbox to To do in Core development Jan 22, 2024
@azeey azeey changed the title Thread safety in custom rednering sensor system (DopplerVelocityLogSystem.cc) Thread safety in custom rendering sensor system (DopplerVelocityLogSystem.cc) Jan 26, 2024
@azeey
Copy link
Contributor

azeey commented Jan 26, 2024

Thanks for catching those. There does seem to be a race condition on those variables. Would you be able to submit a PR?

@azeey azeey moved this from To do to In progress in Core development Jan 26, 2024
@sauk2
Copy link
Contributor

sauk2 commented Mar 25, 2024

@azeey Could I work on this?

@azeey
Copy link
Contributor

azeey commented Mar 25, 2024

Yes, please go ahead. I'll assign it to you.

@azeey azeey assigned sauk2 and unassigned azeey Mar 25, 2024
@sauk2
Copy link
Contributor

sauk2 commented Mar 26, 2024

Yes, please go ahead. I'll assign it to you.

Would like to know your opinion on a possible implementation idea here:

  • Have a single mutex to protect both simTime and nextUpdateTime.
  • Always lock the mutex using lock_guard at the start of the OnRender and OnPostRender callbacks.
  • This if block in DoPostUpdate uses both variables. Break it down into a nested if and lock the mutex inside it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants