fix(notification): fix memory leak in NotifyServerApplet destructor#1598
Open
heysion wants to merge 1 commit into
Open
fix(notification): fix memory leak in NotifyServerApplet destructor#1598heysion wants to merge 1 commit into
heysion wants to merge 1 commit into
Conversation
|
[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
|
![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.
Comment on lines
+28
to
+31
| 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.
|
TAG Bot TAG: 2.0.42 |
zccrs
requested changes
May 28, 2026
| m_worker->deleteLater(); | ||
| delete m_worker; | ||
| m_worker = nullptr; | ||
| m_manager = nullptr; |
|
TAG Bot New tag: 2.0.43 |
heysion
force-pushed
the
heysion-dev
branch
8 times, most recently
from
825bbf7 to
e606777
Compare
18202781743
reviewed
May 29, 2026
| m_worker = nullptr; | ||
| if (m_manager) { | ||
| m_manager->deleteLater(); | ||
| QCoreApplication::sendPostedEvents(m_manager, QEvent::DeferredDelete); |
Contributor
There was a problem hiding this comment.
释放需要这么复杂么？这里的场景很简单，将m_manager移动到worker线程中执行，释放NotifyServerApplet后需要停止这个worker线程，再释放m_manager,和worker，
zccrs
requested changes
May 29, 2026
| m_worker = nullptr; | ||
| if (m_manager) { | ||
| m_manager->deleteLater(); | ||
| QCoreApplication::sendPostedEvents(m_manager, QEvent::DeferredDelete); |
Member
There was a problem hiding this comment.
啊，不要搞这种骚操作呀，这个 manager 对象是所属哪个线程的？如果是在 worker 线程，那要确保在退出worker线程之前把manager对象销毁掉，然后再退出worker线程
heysion
force-pushed
the
heysion-dev
branch
4 times, most recently
from
b1a79a6 to
bd77fe9
Compare
18202781743
reviewed
May 29, 2026
| bool NotifyServerApplet::init() | ||
| { | ||
| // Reentrancy protection | ||
| if (m_manager || m_worker) { |
Contributor
There was a problem hiding this comment.
init不会执行多次，不用判断，另外应该是你的那个test存在问题，应该不用测直接赋值m_manager，m_worker这种场景，
heysion
force-pushed
the
heysion-dev
branch
3 times, most recently
from
e2533ae to
67fa2f8
Compare
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 析构函数中的内存泄漏
deepin pr auto review这份 Git Diff 对通知服务小程序（ 以下是详细的审查意见： 一、 语法与逻辑
二、 代码质量
三、 代码性能
四、 代码安全
总结与修改建议代码片段针对最严重的跨线程 delete 问题和 QPointer 逻辑问题，建议对析构函数和 notifyserverapplet.cpp: NotifyServerApplet::~NotifyServerApplet()
{
qDebug(notifyLog) << "Exit notification server.";
static constexpr int kWaitTimeoutMs = 3000;
static constexpr int kTerminateWaitMs = 1000;
if (m_worker) {
if (m_worker->isRunning()) {
m_worker->quit();
if (!m_worker->wait(kWaitTimeoutMs)) {
qWarning(notifyLog) << "Worker thread did not exit in time, terminating.";
m_worker->terminate();
if (!m_worker->wait(kTerminateWaitMs)) {
qCritical(notifyLog) << "Worker thread terminate timeout.";
}
}
}
// 由于 m_manager 已经通过 connect 绑定了 finished -> deleteLater，
// 这里不需要也不应该直接 delete m_manager，QPointer 会自动置空。
// 如果没有绑定信号，则必须先移回当前线程再 delete：
// if (m_manager) {
// m_manager->moveToThread(QThread::currentThread());
// delete m_manager.data();
// }
delete m_worker.data(); // QPointer 自动置空
} else if (m_manager) {
// 如果没有 worker，直接在当前线程 delete
delete m_manager.data(); // QPointer 自动置空
}
}
bool NotifyServerApplet::init()
{
if (m_manager && m_worker) {
qWarning(notifyLog) << "NotifyServerApplet is already initialized.";
return true;
}
DApplet::init();
m_manager = new NotificationManager();
if (!m_manager->registerDbusService()) {
qWarning(notifyLog) << QString("Can't register Notifications to the D-Bus object.");
delete m_manager.data(); // QPointer 自动置空
return false;
}
m_worker = new QThread();
m_manager->moveToThread(m_worker);
// 安全的异步清理机制
connect(m_worker, &QThread::finished, m_manager, &QObject::deleteLater);
m_worker->start();
return true;
} |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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.
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: