fix: resolve face enrollment freeze issue#45
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR fixes a face enrollment freeze by making the capture stop flag thread-safe, setting it before the blocking stop call, and hardening the socket send loop with bounded retry handling for transient EAGAIN/EWOULDBLOCK errors. Sequence diagram for face enrollment stop and capture loop interactionsequenceDiagram
participant DriverManger
participant ErollThread
participant Socket
DriverManger->>ErollThread: enrollStop(actionId, errMsgInfo)
DriverManger->>ErollThread: m_stopCapture = true
DriverManger->>ErollThread: Stop() (Qt::BlockingQueuedConnection)
loop sendCapture loop
ErollThread->>Socket: write(m_fileSocket, buf, countSize)
alt sendSize < 0 and errno is EAGAIN or EWOULDBLOCK
ErollThread->>ErollThread: retryCount++
alt retryCount > 100
ErollThread->>ErollThread: break loop
else retry limit not reached
ErollThread->>ErollThread: usleep(1000)
end
else sendSize >= 0
ErollThread->>ErollThread: retryCount = 0
ErollThread->>ErollThread: countSize -= sendSize
end
alt m_stopCapture is true
ErollThread->>ErollThread: exit capture loop
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
m_stopCaptureflag is now a publicstd::atomic<bool>; consider keeping it private and exposing a small setter/getter (or a dedicatedrequestStop()method) to avoid external code directly mutating thread-control state. - In
sendCapture, non-EAGAIN/EWOULDBLOCK errors fromwritestill result in a tight loop with no backoff or break condition; consider breaking out or logging on other errors to avoid potential busy-waiting on persistent failures. - When the retry count exceeds 100 in
sendCapture, the loop breaks silently; consider at least logging this condition or propagating an error so callers can distinguish between a normal stop and a send failure.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `m_stopCapture` flag is now a public `std::atomic<bool>`; consider keeping it private and exposing a small setter/getter (or a dedicated `requestStop()` method) to avoid external code directly mutating thread-control state.
- In `sendCapture`, non-EAGAIN/EWOULDBLOCK errors from `write` still result in a tight loop with no backoff or break condition; consider breaking out or logging on other errors to avoid potential busy-waiting on persistent failures.
- When the retry count exceeds 100 in `sendCapture`, the loop breaks silently; consider at least logging this condition or propagating an error so callers can distinguish between a normal stop and a send failure.
## Individual Comments
### Comment 1
<location path="workmodule.cpp" line_range="117-123" />
<code_context>
+ int retryCount = 0;
while (countSize > 0 && !m_stopCapture) {
long sendSize = write(m_fileSocket, &buf[size - countSize], static_cast<size_t>(countSize));
if (sendSize < 0) {
+ if (errno == EAGAIN || errno == EWOULDBLOCK) {
+ retryCount++;
+ if (retryCount > 100) {
+ break;
+ }
+ usleep(1000);
+ }
continue;
</code_context>
<issue_to_address>
**issue (bug_risk):** Non-EAGAIN/EWOULDBLOCK errors from `write` are ignored and may cause a busy loop.
If `write` returns `< 0` with an error other than `EAGAIN`/`EWOULDBLOCK`, we still hit the unconditional `continue` and immediately retry, so permanent errors like `EPIPE`, `ECONNRESET`, or `EBADF` will cause a tight loop until `m_stopCapture` changes. Consider detecting non-retryable errors, breaking out of the loop (and optionally logging) instead of repeatedly calling a failing `write`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
1. Set m_stopCapture flag to true before invoking Stop method to signal capture loop exit 2. Convert m_stopCapture from bool to std::atomic for thread-safe cross- thread access 3. Add retry logic for socket write operations to handle EAGAIN/ EWOULDBLOCK errors 4. Implement maximum 100 retries with 1ms delay to prevent infinite loop in sendCapture Influence: 1. Test face enrollment stop operation to verify no freeze occurs 2. Test concurrent stop request during active image capture 3. Verify socket write handles temporary unavailability gracefully 4. Test retry mechanism reaches maximum limit and breaks loop correctly 5. Verify normal face enrollment completion still works correctly 6. Test multiple rapid start/stop enrollment cycles fix: 解决录入人脸时卡死的问题 1. 在调用 Stop 方法前设置 m_stopCapture 标志为 true 以通知捕获循环退出 2. 将 m_stopCapture 从 bool 转换为 std::atomic 实现跨线程安全访问 3. 为 socket 写操作添加重试逻辑以处理 EAGAIN/EWOULDBLOCK 错误 4. 实现最大 100 次重试和 1ms 延迟以防止 sendCapture 中出现死循环 Influence: 1. 测试人脸录入停止操作验证不再发生卡死 2. 测试活动图像捕获期间的并发停止请求 3. 验证 socket 写操作能优雅处理临时不可用情况 4. 测试重试机制达到最大限制时正确跳出循环 5. 验证正常人脸录入完成流程仍然正常工作 6. 测试多次快速开始/停止录入循环 PMS: BUG-352653 Change-Id: Ice3f9e91fc29f36aa4315da9d4b6b049d6b190fe
deepin pr auto review你好!我是CodeGeeX,你的智能编程助手。我已经仔细审查了你提供的 本次代码变更主要解决了网络/套接字写入时的阻塞或死循环问题,并引入了 以下是详细的审查意见: 1. 语法与逻辑
2. 代码质量
3. 代码性能
4. 代码安全
💡 综合改进建议代码基于以上分析,我为你重构了相关代码: 1. workmodule.h // ... 头文件保持不变 ...
#include <atomic>
class ErollThread : public QObject {
// ...
private Q_SLOTS:
void captureError(int err, QImageCapture::Error, const QString &errorString);
void processCapturedImage(int id, const QImage &preview);
public:
// 提供安全的停止接口,保持封装性
void requestStop() { m_stopCapture.store(true); }
private:
QScopedPointer<QCamera> m_camera;
QScopedPointer<QImageCapture> m_imageCapture;
QString m_actionId;
int m_fileSocket;
bool m_bFirst;
// 使用 atomic 并显式初始化,设为 private
std::atomic<bool> m_stopCapture{false};
bool m_checkDone;
};2. workmodule.cpp (sendCapture 函数的 while 循环部分) // ... 前置代码 ...
if (m_fileSocket < 0) {
qWarning() << "Invalid socket descriptor";
free(buf);
return;
}
unsigned long countSize = size;
// 建议使用超时机制替代简单的重试次数
QElapsedTimer timer;
timer.start();
const qint64 MAX_RETRY_TIMEOUT_MS = 100; // 最大重试时间 100ms
while (countSize > 0 && !m_stopCapture.load()) {
ssize_t sendSize = write(m_fileSocket, &buf[size - countSize], static_cast<size_t>(countSize));
if (sendSize < 0) {
int err = errno;
if (err == EINTR) {
// 被信号中断,直接重试
continue;
} else if (err == EAGAIN || err == EWOULDBLOCK) {
// 非阻塞模式下资源暂不可用,检查超时
if (timer.elapsed() > MAX_RETRY_TIMEOUT_MS) {
qWarning() << "Write timeout (EAGAIN) after" << MAX_RETRY_TIMEOUT_MS << "ms";
break;
}
QThread::msleep(1); // 也可以使用更轻量的 QThread::yieldCurrentThread()
continue;
} else {
// 其他不可恢复的错误(如 EPIPE, EBADF 等)
qWarning() << "write error:" << strerror(err);
break;
}
}
// 写入成功,更新剩余大小
countSize -= static_cast<unsigned long>(sendSize);
}
free(buf);
// ... 后置代码 ...3. drivermanger.cpp qDebug() << "start Erollthread stop";
// 使用封装好的接口,而不是直接暴露变量
m_spErollthread->requestStop();
// 评估是否真的需要 BlockingQueuedConnection
// 如果不需要阻塞当前线程,建议改为 Qt::QueuedConnection
QMetaObject::invokeMethod(m_spErollthread.data(), "Stop",
Qt::QueuedConnection);
m_actionMap.remove(actionId);这些改进将使你的代码更加健壮、安全且易于维护,特别是在处理复杂的套接字 I/O 和多线程交互时。如果有任何疑问,欢迎随时提问! |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fly602, robertkill 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 |
|
/forcemerge |
|
This pr force merged! (status: unstable) |
Influence:
fix: 解决录入人脸时卡死的问题
Influence:
PMS: BUG-352653
Change-Id: Ice3f9e91fc29f36aa4315da9d4b6b049d6b190fe
Summary by Sourcery
Fix face enrollment freezes by improving capture stop signaling and making socket writes more robust against transient errors.
Bug Fixes: