-
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.
tools/dconfig2cpp/main.cpp
Outdated
| 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
…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代码审查报告总体评估这段代码是自动生成的 DTK (Deepin Tool Kit) 配置管理类代码,主要变更涉及线程安全改进、内存管理优化和代码结构重构。整体上代码质量有显著提升,但仍存在一些可以改进的地方。 具体改进建议1. 线程安全改进优点:
建议: // 在 Data 类中添加线程安全保护
class Data : public QObject {
public:
// 使用读写锁保护复杂类型的数据访问
mutable QReadWriteLock m_dataLock;
// 修改属性访问方法
QString colorMode() const {
QReadLocker locker(&m_dataLock);
return p_colorMode;
}
void setColorMode(const QString &v) {
QWriteLocker locker(&m_dataLock);
if (p_colorMode == v && testPropertySet(1))
return;
p_colorMode = v;
markPropertySet(1);
// ... 其余代码
}
};2. 内存管理改进优点:
建议: // 在析构函数中添加更明确的资源清理
~dconfig_org_deepin_dtk_preference() {
int oldStatus = m_data->m_status.fetchAndStoreOrdered(static_cast<int>(Data::Status::Destroyed));
if (oldStatus == static_cast<int>(Data::Status::Succeeded)) {
auto config = m_data->m_config.loadRelaxed();
if (config) {
// 断开所有信号连接
QObject::disconnect(config, nullptr, m_data, nullptr);
config->deleteLater();
}
// 使用 QPointer 安全删除
QPointer<Data> data(m_data);
if (data) {
data->deleteLater();
}
}
}3. 错误处理改进建议: // 在初始化失败时添加更详细的错误信息
if (!config || !config->isValid()) {
QString errorMsg = QLatin1String("Failed to create DConfig instance.");
if (config) {
errorMsg += QLatin1String(" Config exists but is invalid.");
}
qWarning() << errorMsg;
// 添加错误代码
emit initializationError(static_cast<int>(Data::Status::Failed));
// ... 其余代码
}4. 代码性能优化建议: // 减少不必要的 QVariant 转换
void setColorMode(const QString &v) {
// 使用 const QString& 避免拷贝
if (m_data->p_colorMode == v && m_data->testPropertySet(1))
return;
// 使用移动语义
m_data->p_colorMode = std::move(v);
// ... 其余代码
}
// 批量更新时减少信号发射
void updateProperties(const QMap<QString, QVariant>& updates) {
bool changed = false;
for (auto it = updates.begin(); it != updates.end(); ++it) {
if (updatePropertyInternal(it.key(), it.value())) {
changed = true;
}
}
if (changed) {
Q_EMIT propertiesChanged();
}
}5. 代码可维护性改进建议: // 添加更多文档注释
/**
* @brief 设置颜色模式
* @param v 颜色模式值,有效值为 "light", "dark", "auto"
* @note 此操作会触发 colorModeChanged() 和 valueChanged() 信号
* @warning 线程安全:此方法可以从任何线程调用
*/
void setColorMode(const QString &v);
// 使用枚举代替魔法字符串
enum class ColorMode {
Light,
Dark,
Auto
};
QString colorModeToString(ColorMode mode);
ColorMode stringToColorMode(const QString& str);6. 安全性改进建议: // 添加输入验证
void setColorMode(const QString &v) {
// 验证输入
if (!isValidColorMode(v)) {
qWarning() << "Invalid color mode:" << v;
return;
}
// ... 其余代码
}
// 添加敏感操作保护
void resetToDefaults() {
// 检查权限
if (!hasResetPermission()) {
qWarning() << "No permission to reset to defaults";
return;
}
// ... 其余代码
}7. 测试建议建议添加以下测试用例:
总结代码整体质量良好,主要改进集中在线程安全和内存管理方面。建议优先实施线程安全改进和内存管理优化,这些改进可以显著提高代码的稳定性和性能。同时,添加更多的错误处理和输入验证可以提高代码的健壮性。 |
| 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:
Solution:
Introduce Data layer separation (TreelandUserConfigData + TreelandUserConfig)
Enhance state machine (3-state -> 5-state model)
Improve async initialization and cleanup
Separate thread responsibilities
Improvements: