fix: improve enrollment stop sequence and thread safety #40
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR refactors the enrollment stop sequence to address thread-safety and race conditions by disconnecting image capture signals before stopping, using a busy-wait synchronized flag with an RAII guard for task completion, reordering resource cleanup (including ModelManager unload post-blocking stop), and removing an unnecessary QCoreApplication::processEvents call. Sequence diagram for improved enrollment stop processsequenceDiagram
participant DriverManger
participant ErollThread
participant Camera
participant ModelManger
DriverManger->>ErollThread: invokeMethod("Stop", BlockingQueuedConnection)
ErollThread->>Camera: disconnect signals
ErollThread->>Camera: stop()
ErollThread->>Camera: reset()
ErollThread->>ErollThread: wait for m_checkDone (busy-wait)
ErollThread->>ErollThread: cleanup fileSocket
DriverManger->>ModelManger: unLoad()
DriverManger->>DriverManger: remove actionId from m_actionMap
DriverManger->>DriverManger: update charaList
Class diagram for ErollThread stop and CheckDoneGuard changesclassDiagram
class ErollThread {
- m_imageCapture : QImageCapture*
- m_camera : QCamera*
- m_stopCapture : bool
- m_checkDone : bool
- m_fileSocket : int
+ Stop()
+ processCapturedImage(int id, const QImage &preview)
}
class CheckDoneGuard {
- self : ErollThread*
+ CheckDoneGuard(ErollThread *s)
+ ~CheckDoneGuard()
}
ErollThread "1" o-- "1" CheckDoneGuard : uses (RAII in processCapturedImage)
class QFutureWatcher {
+ finished
}
ErollThread "1" o-- "1" QFutureWatcher : async image processing
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `workmodule.cpp:76-78` </location>
<code_context>
m_stopCapture = true;
m_camera->stop();
m_camera.reset();
+ while (!m_checkDone) {
+ qDebug() << "wait check done thread:" << QThread::currentThreadId();
+ QThread::msleep(100);
+ continue;
+ }
</code_context>
<issue_to_address>
**suggestion (performance):** Busy-wait loop may block thread responsiveness.
Consider replacing the busy-wait loop with QWaitCondition or signals to avoid blocking the thread and improve responsiveness.
Suggested implementation:
```cpp
m_mutex.lock();
while (!m_checkDone) {
qDebug() << "wait check done thread:" << QThread::currentThreadId();
m_checkDoneCondition.wait(&m_mutex, 100); // Wait for up to 100ms, then re-check
}
m_mutex.unlock();
```
You will need to:
1. Add `QWaitCondition m_checkDoneCondition;` and `QMutex m_mutex;` as members of your `ErollThread` class.
2. Wherever `m_checkDone` is set to `true` (likely in another thread), you must call:
```
m_mutex.lock();
m_checkDone = true;
m_checkDoneCondition.wakeAll();
m_mutex.unlock();
```
This will wake up the waiting thread in `Stop()`.
3. Make sure to `#include <QWaitCondition>` and `#include <QMutex>` at the top of your file.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+76
to
+78
| while (!m_checkDone) { | ||
| qDebug() << "wait check done thread:" << QThread::currentThreadId(); | ||
| QThread::msleep(100); |
There was a problem hiding this comment.
suggestion (performance): Busy-wait loop may block thread responsiveness.
Consider replacing the busy-wait loop with QWaitCondition or signals to avoid blocking the thread and improve responsiveness.
Suggested implementation:
m_mutex.lock();
while (!m_checkDone) {
qDebug() << "wait check done thread:" << QThread::currentThreadId();
m_checkDoneCondition.wait(&m_mutex, 100); // Wait for up to 100ms, then re-check
}
m_mutex.unlock();
You will need to:
- Add
QWaitCondition m_checkDoneCondition;andQMutex m_mutex;as members of yourErollThreadclass. - Wherever
m_checkDoneis set totrue(likely in another thread), you must call:This will wake up the waiting thread inm_mutex.lock(); m_checkDone = true; m_checkDoneCondition.wakeAll(); m_mutex.unlock();Stop(). - Make sure to
#include <QWaitCondition>and#include <QMutex>at the top of your file.
mhduiy
reviewed
Oct 15, 2025
workmodule.cpp
Outdated
| ErollThread *self; | ||
| CheckDoneGuard(ErollThread *s) : self(s) {} | ||
| ~CheckDoneGuard() { self->m_checkDone = true; } | ||
| } guard(this); |
Contributor
There was a problem hiding this comment.
auto cleanup = qScopeGuard([&] {
// 这里的代码会在函数退出时自动执行
qDebug() << "清理资源";
});
1. Fixed thread synchronization issue by ensuring camera cleanup completes before proceeding 2. Added proper signal disconnections to prevent race conditions during stop 3. Implemented RAII pattern with CheckDoneGuard to ensure m_checkDone flag is always set 4. Reordered resource cleanup sequence for safer shutdown 5. Removed unnecessary QCoreApplication::processEvents() call that could cause reentrancy issues Log: Fixed enrollment process stop sequence to prevent application crashes Influence: 1. Test enrollment start and stop multiple times in quick succession 2. Verify camera resources are properly released after enrollment completion 3. Check that no race conditions occur during enrollment cancellation 4. Validate that face detection and anti-spoofing processes terminate cleanly 5. Test enrollment with different camera devices and configurations fix: 改进注册停止序列和线程安全性 1. 修复线程同步问题，确保相机清理完成后再继续执行 2. 添加正确的信号断开连接，防止停止过程中的竞争条件 3. 使用 CheckDoneGuard 实现 RAII 模式，确保 m_checkDone 标志始终被设置 4. 重新安排资源清理顺序以实现更安全的关闭 5. 移除可能导致重入问题的不必要 QCoreApplication::processEvents() 调用 Log: 修复注册过程停止序列，防止应用程序崩溃 Influence: 1. 测试快速连续多次启动和停止注册过程 2. 验证注册完成后相机资源是否正确释放 3. 检查注册取消过程中是否出现竞争条件 4. 验证人脸检测和活体检测过程是否干净终止 5. 使用不同的相机设备和配置测试注册功能 PMS: BUG-309915
deepin pr auto review代码审查报告drivermanger.cpp改进建议
workmodule.cpp改进建议
总体建议
|
mhduiy
approved these changes
Oct 15, 2025
yixinshark
approved these changes
Oct 15, 2025
BLumia
approved these changes
Oct 15, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: BLumia, mhduiy, robertkill, yixinshark The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Member
|
/merge |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Log: Fixed enrollment process stop sequence to prevent application crashes
Influence:
fix: 改进注册停止序列和线程安全性
Log: 修复注册过程停止序列，防止应用程序崩溃
Influence:
PMS: BUG-309915
Summary by Sourcery
Fix enrollment stop sequence to improve thread safety and prevent race conditions during camera cleanup and data processing.
Bug Fixes:
Enhancements: