Fix Check For Updates crash #14229
Fix Check For Updates crash #14229
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a thread join operation before reassigning the check_for_device_updates_thread to prevent potential resource leaks and undefined behavior.
- Adds a joinability check and join operation before thread reassignment
- Ensures proper cleanup of existing thread before creating a new one
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| std::weak_ptr< notifications_model > notification_model_protected( viewer.not_model ); | ||
| const context & ctx( viewer.ctx ); | ||
| if (check_for_device_updates_thread.joinable()) | ||
| check_for_device_updates_thread.join(); |
There was a problem hiding this comment.
Consider adding a detach() call as an alternative to join() to avoid blocking the current thread. If the existing thread is performing a long-running operation, join() will block until completion, which could cause UI freezing or performance issues.
| check_for_device_updates_thread.join(); | |
| check_for_device_updates_thread.detach(); |
| _updates_profile ); | ||
| std::weak_ptr< notifications_model > notification_model_protected( viewer.not_model ); | ||
| const context & ctx( viewer.ctx ); | ||
| if (check_for_device_updates_thread.joinable()) |
There was a problem hiding this comment.
It was a detached thread before as i see
There was a problem hiding this comment.
It was a detached thread before and that was the problem previous PR solved - a thread trying to access object that don't exist anymore (device disconnected)
| const context & ctx( viewer.ctx ); | ||
| if (check_for_device_updates_thread.joinable()) | ||
| check_for_device_updates_thread.join(); | ||
| check_for_device_updates_thread = std::thread( [ctx, |
There was a problem hiding this comment.
If calling this while thread is joinable, that the task we want to perform (checking for updates) is already being performed. Do we need to create another thread and check again? Can't we use previous thread outcome?
There was a problem hiding this comment.
We also need to see how the UI is affected, it may get it to stuck, that's why it was detached.
If access is a problem we can try using weak_ptr
There was a problem hiding this comment.
We can do that, but it's not related to the thread being joinable - once we create a thread and not detach it, we need to join it, even if it finishes running, so being joinable doesn't indicate that it's still running
There was a problem hiding this comment.
We had no issue with check for updates before the last PR (this code was there for 5 years ), we did had an issue with the platform camera new thread no?
Am I missing something?
There was a problem hiding this comment.
Platform camera was another issue. This thread handling was changed because of a problem when a camera was connected and quickly disconnected.
There was a problem hiding this comment.
I believe all use cases were solved in the past when adding a weak ptr,
@AviaAv can you revert the previous PR locally and say which resource is not longer protected maybe we can handle it?
I even prefer to skip the request than to delay the UI if this is the solution we go with
Tracked on: [LRS-1331]