fix(notification): fix memory leak in NotifyServerApplet destructor#1598
fix(notification): fix memory leak in NotifyServerApplet destructor#1598heysion wants to merge 1 commit into
Conversation
Fixed the issue where NotificationManager was not properly destroyed when the worker thread exited. The deleteLater() call was scheduled but never executed because the thread's event loop had already stopped. Added comprehensive unit tests to verify the fix covers all destruction scenarios including worker-only, manager-only, and both present cases. Log: Fix memory leak in NotifyServerApplet destructor fix(notification): 修复 NotifyServerApplet 析构函数中的内存泄漏 修复了当工作线程退出时 NotificationManager 未被正确销毁的问题。 deleteLater() 被调度但从未执行，因为线程的事件循环已经停止。 添加了全面的单元测试来验证修复涵盖了所有销毁场景， 包括仅 worker、仅 manager 和两者都存在的情况。 Log: 修复 NotifyServerApplet 析构函数中的内存泄漏
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: heysion 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 |
Reviewer's GuideRefactors NotifyServerApplet’s destructor to ensure NotificationManager is destroyed correctly in all worker-thread and main-thread cases, and adds targeted unit tests that validate the new destruction behavior and prevent regressions around the previous memory leak. Sequence diagram for updated NotifyServerApplet destruction logicsequenceDiagram
participant NotifyServerApplet
participant QThread_worker as m_worker
participant NotificationManager_manager as m_manager
NotifyServerApplet->>NotifyServerApplet: ~NotifyServerApplet()
alt m_worker != nullptr
opt m_manager != nullptr
NotifyServerApplet->>QThread_worker: connect(m_worker.finished, m_manager.deleteLater)
end
NotifyServerApplet->>QThread_worker: exit()
NotifyServerApplet->>QThread_worker: wait()
NotifyServerApplet->>NotifyServerApplet: delete m_worker
NotifyServerApplet->>NotifyServerApplet: m_worker = nullptr
NotifyServerApplet->>NotifyServerApplet: m_manager = nullptr
else m_worker == nullptr and m_manager != nullptr
NotifyServerApplet->>NotifyServerApplet: delete m_manager
NotifyServerApplet->>NotifyServerApplet: m_manager = nullptr
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review这份代码变更主要重构了 整体来看，这次修改的方向是正确的，解决了一个典型的 Qt 线程对象销毁顺序问题。但在代码逻辑、安全性和测试设计上还有一些可以改进和注意的地方。以下是详细的审查意见： 一、 语法与逻辑1. 析构函数中的 改进意见： 去掉 NotifyServerApplet::~NotifyServerApplet()
{
qDebug(notifyLog) << "Exit notification server.";
if (m_worker) {
if (m_manager) {
// Schedule manager deletion while the worker event loop is still able to process it.
connect(m_worker, &QThread::finished, m_manager, &QObject::deleteLater, Qt::UniqueConnection);
}
m_worker->exit();
m_worker->wait();
delete m_worker;
m_worker = nullptr;
// m_manager 的生命周期已交给 QThread::finished 信号管理，无需在此置空或删除
} else if (m_manager) {
// 没有 worker 线程，直接在当前线程删除
delete m_manager;
m_manager = nullptr;
}
}2. 二、 代码质量1. 测试代码中破坏封装性的宏定义 #define private public
#include "notifyserverapplet.h"
#undef private这是一种非常不推荐的做法。它破坏了类的封装性，使得测试与类的内部实现强耦合。一旦类的成员变量名或结构改变，测试就会崩溃，且可能导致 ODR（One Definition Rule）问题。 改进意见：
2. 测试类 改进意见： QSignalSpy destroySpy(manager, &QObject::destroyed);
// ... 触发析构 ...
EXPECT_EQ(destroySpy.count(), 1);三、 代码性能1. connect(m_worker, &QThread::finished, m_manager, &QObject::deleteLater, Qt::UniqueConnection);从性能角度看， 改进意见： connect(m_worker, &QThread::finished, m_manager, &QObject::deleteLater);四、 代码安全1. 线程安全： 潜在风险：如果在调用 改进建议： 2. 改进意见： if (!m_worker->wait(5000)) { // 例如等待5秒
qWarning(notifyLog) << "Worker thread failed to exit in time, forcing termination.";
m_worker->terminate();
m_worker->wait();
}总结这次修改的核心逻辑（利用
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The destructor logic assumes the worker thread is still running when the connection to
finishedis made; if the thread has already finished (butm_workeris non-null), thefinishedsignal will never fire andm_managerwill leak, so consider checkingm_worker->isRunning()and deletingm_managerdirectly in the non-running case. - The tests use
#define private publicto accessNotifyServerAppletinternals; it would be more robust to expose the needed state via test-only helpers or friend declarations rather than redefining access specifiers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The destructor logic assumes the worker thread is still running when the connection to `finished` is made; if the thread has already finished (but `m_worker` is non-null), the `finished` signal will never fire and `m_manager` will leak, so consider checking `m_worker->isRunning()` and deleting `m_manager` directly in the non-running case.
- The tests use `#define private public` to access `NotifyServerApplet` internals; it would be more robust to expose the needed state via test-only helpers or friend declarations rather than redefining access specifiers.
## Individual Comments
### Comment 1
<location path="panels/notification/server/notifyserverapplet.cpp" line_range="28-31" />
<code_context>
NotifyServerApplet::~NotifyServerApplet()
{
qDebug(notifyLog) << "Exit notification server.";
- if (m_manager) {
- m_manager->deleteLater();
- }
if (m_worker) {
+ if (m_manager) {
+ // Schedule manager deletion while the worker event loop is still able to process it.
+ connect(m_worker, &QThread::finished, m_manager, &QObject::deleteLater, Qt::UniqueConnection);
+ }
m_worker->exit();
</code_context>
<issue_to_address>
**issue (bug_risk):** QThread::finished → deleteLater on an object in the worker thread is likely never processed after calling exit(), leading to a leak.
Because `QThread::finished` is emitted in the owning (likely main/UI) thread, the `deleteLater` call on `m_manager` becomes a queued event to the worker’s event loop. But `exit()` stops that event loop and `finished` is emitted only after it has already exited, so the queued `deleteLater` may never run, leaking `m_manager`.
Instead, delete `m_manager` before shutting down the worker, for example by:
- Calling `QMetaObject::invokeMethod(m_manager, "deleteLater", Qt::BlockingQueuedConnection)` before `m_worker->exit()`/`wait()`, or
- Deleting it directly if it already lives in the main thread.
The key point is to clean up `m_manager` synchronously in the correct thread, rather than relying on `finished` after the event loop is gone.
</issue_to_address>
### Comment 2
<location path="tests/panels/notification/server/notifyserverapplet_test.cpp" line_range="127-110" />
<code_context>
+ EXPECT_NO_THROW(delete testApplet);
+}
+
+TEST_F(NotifyServerAppletTest, DestructorDeletesManagerAfterWorkerThreadExit) {
+ TrackableNotificationManager::destroyedCount = 0;
+
+ auto *testApplet = new NotifyServerApplet();
+ auto *worker = new QThread();
+ auto *manager = new TrackableNotificationManager();
+ QPointer<TrackableNotificationManager> managerGuard(manager);
+
+ testApplet->m_manager = manager;
+ testApplet->m_worker = worker;
+
+ manager->moveToThread(worker);
+ worker->start();
+
+ QTRY_VERIFY_WITH_TIMEOUT(worker->isRunning(), 1000);
+
+ delete testApplet;
+
+ EXPECT_TRUE(managerGuard.isNull());
+ EXPECT_EQ(TrackableNotificationManager::destroyedCount.load(), 1);
+}
+
</code_context>
<issue_to_address>
**suggestion (testing):** The null QPointer and destroyedCount checks may race with an asynchronous deleteLater, making this test potentially flaky.
Because the manager is deleted via a signal/`deleteLater`, its actual destruction depends on event loop processing. Immediately checking `managerGuard.isNull()` and `destroyedCount == 1` after `delete testApplet;` can therefore be timing‑dependent. To avoid flakiness, either wait for the event loop until the guard becomes null (e.g. `QTRY_VERIFY_WITH_TIMEOUT(managerGuard.isNull(), 1000);`) before asserting the count, or make the manager deletion synchronous in this path so the assertions can be deterministic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if (m_worker) { | ||
| if (m_manager) { | ||
| // Schedule manager deletion while the worker event loop is still able to process it. | ||
| connect(m_worker, &QThread::finished, m_manager, &QObject::deleteLater, Qt::UniqueConnection); |
There was a problem hiding this comment.
issue (bug_risk): QThread::finished → deleteLater on an object in the worker thread is likely never processed after calling exit(), leading to a leak.
Because QThread::finished is emitted in the owning (likely main/UI) thread, the deleteLater call on m_manager becomes a queued event to the worker’s event loop. But exit() stops that event loop and finished is emitted only after it has already exited, so the queued deleteLater may never run, leaking m_manager.
Instead, delete m_manager before shutting down the worker, for example by:
- Calling
QMetaObject::invokeMethod(m_manager, "deleteLater", Qt::BlockingQueuedConnection)beforem_worker->exit()/wait(), or - Deleting it directly if it already lives in the main thread.
The key point is to clean up m_manager synchronously in the correct thread, rather than relying on finished after the event loop is gone.
| delete testApplet; | ||
|
|
||
| EXPECT_TRUE(managerGuard.isNull()); | ||
| EXPECT_EQ(TrackableNotificationManager::destroyedCount.load(), 1); |
There was a problem hiding this comment.
suggestion (testing): The null QPointer and destroyedCount checks may race with an asynchronous deleteLater, making this test potentially flaky.
Because the manager is deleted via a signal/deleteLater, its actual destruction depends on event loop processing. Immediately checking managerGuard.isNull() and destroyedCount == 1 after delete testApplet; can therefore be timing‑dependent. To avoid flakiness, either wait for the event loop until the guard becomes null (e.g. QTRY_VERIFY_WITH_TIMEOUT(managerGuard.isNull(), 1000);) before asserting the count, or make the manager deletion synchronous in this path so the assertions can be deterministic.
2 participants
Fixed the issue where NotificationManager was not properly destroyed when the worker thread exited. The deleteLater() call was scheduled but never executed because the thread's event loop had already stopped.
Added comprehensive unit tests to verify the fix covers all destruction scenarios including worker-only, manager-only, and both present cases.
Log: Fix memory leak in NotifyServerApplet destructor
fix(notification): 修复 NotifyServerApplet 析构函数中的内存泄漏
修复了当工作线程退出时 NotificationManager 未被正确销毁的问题。
deleteLater() 被调度但从未执行，因为线程的事件循环已经停止。
添加了全面的单元测试来验证修复涵盖了所有销毁场景，
包括仅 worker、仅 manager 和两者都存在的情况。
Log: 修复 NotifyServerApplet 析构函数中的内存泄漏
Summary by Sourcery
Ensure NotifyServerApplet correctly cleans up NotificationManager and its worker thread to prevent memory leaks on destruction.
Bug Fixes:
Enhancements:
Tests: