-
Notifications
You must be signed in to change notification settings - Fork 90
fix: Refactor DConfig wrapper class generation for thread safety and … #531
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Dami-star 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 |
Synchronize source files from linuxdeepin/dtkcore. Source-pull-request: linuxdeepin/dtkcore#531
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the DConfig C++ wrapper class generation tool (dconfig2cpp) to improve thread safety and lifecycle management. The refactor introduces a data layer separation pattern, enhances the state machine from 3 states to 5 states, and implements atomic operations for thread-safe state transitions.
Key changes:
- Introduces a Data class (TreelandUserConfigData) to separate internal data management from the public API (TreelandUserConfig)
- Expands state machine from Invalid/Succeed/Failed to Invalid/Initializing/Succeed/Failed/Destroyed with atomic CAS operations
- Separates thread responsibilities: updateValue() in worker thread, updateProperty() in main thread with QMetaObject::invokeMethod for proper thread context
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
zccrs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
还没有看完
Synchronize source files from linuxdeepin/dtkcore. Source-pull-request: linuxdeepin/dtkcore#531
Synchronize source files from linuxdeepin/dtkcore. Source-pull-request: linuxdeepin/dtkcore#531
Synchronize source files from linuxdeepin/dtkcore. Source-pull-request: linuxdeepin/dtkcore#531
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| m_data->deleteLater(); | ||
| } | ||
| } else if (oldStatus == static_cast<int>(Data::Status::Failed) || oldStatus == static_cast<int>(Data::Status::Invalid)) { | ||
| m_data->deleteLater(); |
Copilot
AI
Jan 14, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The destructor has a potential memory leak scenario. When oldStatus is Initializing, m_data is not deleted. This can occur if the destructor is called while initialization is still in progress on the worker thread. The code should handle the Initializing state by marking it as Destroyed and allowing the worker thread to clean up, or explicitly delete m_data.
| m_data->deleteLater(); | |
| m_data->deleteLater(); | |
| } else if (oldStatus == static_cast<int>(Data::Status::Initializing)) { | |
| m_data->deleteLater(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
当 oldStatus 为 Initializing 时,我们不会在此处删除, 若在此处删除会导致空指针。工作线程将检测到 Destroyed 状态并自行清理 m_data。
Synchronize source files from linuxdeepin/dtkcore. Source-pull-request: linuxdeepin/dtkcore#531
Synchronize source files from linuxdeepin/dtkcore. Source-pull-request: linuxdeepin/dtkcore#531
Synchronize source files from linuxdeepin/dtkcore. Source-pull-request: linuxdeepin/dtkcore#531
Synchronize source files from linuxdeepin/dtkcore. Source-pull-request: linuxdeepin/dtkcore#531
Synchronize source files from linuxdeepin/dtkcore. Source-pull-request: linuxdeepin/dtkcore#531
Synchronize source files from linuxdeepin/dtkcore. Source-pull-request: linuxdeepin/dtkcore#531
Synchronize source files from linuxdeepin/dtkcore. Source-pull-request: linuxdeepin/dtkcore#531
| delete worker; | ||
| worker = nullptr; | ||
|
|
||
| auto data = safeData; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个赋值有啥必要吗
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
之前删的一块逻辑 等下删除了
| if (!data->m_status.testAndSetOrdered(static_cast<int>(Data::Status::Invalid), | ||
| static_cast<int>(Data::Status::Initializing))) { | ||
| if (data->m_status.loadRelaxed() == static_cast<int>(Data::Status::Destroyed)) { | ||
| QMetaObject::invokeMethod(data, [data]() { delete data; }, Qt::QueuedConnection); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data对象的生命管理是这样的:在状态变为 Initializing 之前,data被dconfig_org_deepin_dtk_preference对象负责销毁,在 Initializing 状态或 Succeeded 状态下,跟随config对象销毁。
所以这里直接返回就行,不用管 data 对象。
| worker = nullptr; | ||
|
|
||
| auto data = safeData; | ||
| if (!data) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
不需要做这个判断,在判断 m_status 之后,可以对data对象做断言。
…lifecycle management Problem: - Single-threaded design with weak state machine (Invalid -> Succeed/Failed) - No proper handling of object destruction during initialization - Signal emissions in worker thread context (incorrect thread context) - Fragile destructor unable to handle all cleanup scenarios Solution: 1. Introduce Data layer separation (TreelandUserConfigData + TreelandUserConfig) - Clear separation between internal data management and public API - Enables safer object lifecycle management 2. Enhance state machine (3-state -> 5-state model) - Add Initializing and Destroyed states - Use atomic CAS operations for thread-safe state transitions - States: Invalid -> Initializing -> (Succeed | Failed | Destroyed) 3. Improve async initialization and cleanup - Use QPointer for safe backref checks (prevent use-after-free) - Support 4 destruction paths: normal/failed/quick/mid-initialization - Atomic state transitions with proper signal emission guards 4. Separate thread responsibilities - updateValue(): Worker thread reads config values - updateProperty(): Main thread updates properties and emits signals - Use QMetaObject::invokeMethod for correct thread context Improvements: - Thread safety: Complete atomic operations coverage - Memory safety: QPointer guards prevent dangling pointers - Code clarity: Layered architecture with clear responsibilities - Backward compatibility: API unchanged
Synchronize source files from linuxdeepin/dtkcore. Source-pull-request: linuxdeepin/dtkcore#531
deepin pr auto review代码审查报告总体评价这段代码是自动生成的 DConfig 配置管理类,主要改进了线程安全性和对象生命周期管理。代码整体结构清晰,使用了现代 C++ 和 Qt 特性,但存在一些可以优化的地方。 语法与逻辑优点
问题与建议
Q_ASSERT(QThread::currentThread() != thread);
bool event(QEvent *e) override {
if (e->type() == QEvent::ThreadChange) {
Q_ASSERT_X(false, "dconfig_dconf-example_meta", "Moving ... is forbidden!");
}
return QObject::event(e);
}
if (e->type() == QEvent::ThreadChange) {
qCritical() << "Moving dconfig object to another thread is forbidden!";
return false;
}代码质量优点
问题与建议
QString jsonFileName = QString(jsonFileName).replace("\n", "\\n").replace("\r", "\\r");
性能优点
问题与建议
if (key == QStringLiteral("array")) {
安全性优点
问题与建议
auto config = m_data->m_config.loadRelaxed();
if (!config) return;
if (!safeData->m_status.testAndSetOrdered(...)) {
return;
}
if (!config || !config->isValid()) {
qWarning() << QLatin1String("Failed to create DConfig instance.");
// ...
}
其他建议
总结这段代码在线程安全性和对象生命周期管理方面有显著改进,但在错误处理、性能优化和代码复用方面还有提升空间。建议重点关注:
|
…lifecycle management
Problem:
Solution:
Introduce Data layer separation (TreelandUserConfigData + TreelandUserConfig)
Enhance state machine (3-state -> 5-state model)
Improve async initialization and cleanup
Separate thread responsibilities
Improvements: