fix: prevent race condition in camera buffer management#4649
fix: prevent race condition in camera buffer management#4649thesohamdatta wants to merge 2 commits intoBasedHardware:mainfrom
Conversation
fix: add upload guard to prevent camera buffer race condition
There was a problem hiding this comment.
Code Review
The pull request introduces a safety check in the take_photo() function to prevent capturing while a photo upload is in progress, which is a good step towards preventing race conditions and memory corruption. However, the effectiveness of this fix relies heavily on the thread-safety of the photoDataUploading flag. If this flag is a shared global variable and modified by multiple tasks, its access needs proper synchronization to truly prevent race conditions in a multi-tasking environment, aligning with the rule that global variables are only safe without thread-safe accessors if modified by a single thread.
| bool take_photo() | ||
| { | ||
| // Safety check: Don't capture if currently uploading | ||
| if (photoDataUploading) { |
There was a problem hiding this comment.
The photoDataUploading flag is used here to prevent a race condition. However, if photoDataUploading is a global boolean variable modified by another task (e.g., the photo upload task) and read by this function, its access needs to be synchronized (e.g., using a mutex or std::atomic_bool) to ensure thread-safety. Without proper synchronization, a data race could still occur, leading to inconsistent state and potentially memory corruption, which this PR aims to prevent. This is a critical issue for the reliability of the system.
References
- This rule implies that for shared variables that are modified and read by multiple tasks, a critical section (or other synchronization mechanism) is necessary to prevent race conditions and ensure data consistency.
|
@thesohamdatta Quick nudge: tests/demo evidence is required before review. Please add test output or a short screenshot/video, then flip this back to ready. Thanks. by AI for @beastoin |
|
Closing this stale draft — no activity in 3+ days. Feel free to reopen when it's ready for review. |
|
Hey @thesohamdatta 👋 Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request. After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:
Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out. Thank you for being part of the Omi community! 💜 |
Add safety check in take_photo() to prevent capturing while upload
is in progress. This fixes potential memory corruption and crashes
during continuous photo capture operations.