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 TreeSupportBaseCircle which may sometimes cause a crash #2210

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ThomasRahm
Copy link
Contributor

Description

The non-proper locking of base_circle has caused a crash for me in my cradle support branch. I don't know it that crash can also occur here, but either way the lock should actually do something.

  • Bug fix (non-breaking change which fixes an issue)

Test Configuration:

  • Operating System:

Checklist:

  • My code follows the style guidelines of this project as described in UltiMaker Meta
  • I have read the Contribution guide
  • I have commented my code, particularly in hard-to-understand areas
  • I have uploaded any files required to test this change

@github-actions github-actions bot added the PR: Community Contribution 👑 Community Contribution PR's label Mar 11, 2025
Copy link
Contributor

github-actions bot commented Mar 11, 2025

Test Results

0 tests   - 27   0 ✅  - 27   0s ⏱️ -5s
0 suites  -  1   0 💤 ± 0 
0 files    -  1   0 ❌ ± 0 

Results for commit 5a74ed4. ± Comparison against base commit ba89f84.

♻️ This comment has been updated with latest results.

@ThomasRahm
Copy link
Contributor Author

ThomasRahm commented Mar 12, 2025

Talked with a friend about this pull request, he suggested to instead use std::call_once as the compiler may reorder the base_circle and boolean assignments, so even my fixed version was not guaranteed to be actually thread-safe. So I changed it to use that instead.
GitHub's diff is over-exaggerating the changes, I recommend to use hide whitespaces to see the actual changes.

Copy link
Contributor

@wawanbreton wawanbreton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ThomasRahm, I am not familiar with std::call_once, but it seems to do exactly what you need here. There are different ways of achieveing this, but this one is very elegant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Community Contribution 👑 Community Contribution PR's
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants