Skip to content

fix: use QLibrary to load and check library version#71

Merged
BLumia merged 1 commit intolinuxdeepin:masterfrom
BLumia:crash-port
Oct 30, 2025
Merged

fix: use QLibrary to load and check library version#71
BLumia merged 1 commit intolinuxdeepin:masterfrom
BLumia:crash-port

Conversation

@BLumia
Copy link
Member

@BLumia BLumia commented Oct 28, 2025

dlopen 的形式加载插件,目前在 Arch Linux 和 openSUSE 观察到崩溃在 dlclose 时. 此处改为使用 QLibrary 加载插件并检查版本

Copy link

Copilot AI left a 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 removes the dependency on QEventLoop in the plugin loader to prevent crashes when plugin loading fails on Arch Linux and openSUSE. The synchronous blocking approach using QEventLoop::exec() is replaced with an asynchronous sequential loading pattern using callbacks and a queue.

Key changes:

  • Replaced synchronous QEventLoop with asynchronous sequential loading mechanism
  • Introduced state tracking variables to manage sequential plugin loading
  • Added loadNextPolicy() slot to handle sequential processing

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/deepin-service-manager/pluginloader.h Added state tracking members (m_pendingPolicies, m_policyLoadInProgress, m_remainingPolicies) and loadNextPolicy() slot
src/deepin-service-manager/pluginloader.cpp Replaced QEventLoop with asynchronous sequential loading pattern using timer callbacks

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@BLumia BLumia force-pushed the crash-port branch 2 times, most recently from 814a7a1 to 3c71648 Compare October 29, 2025 07:55
@deepin-ci-robot
Copy link

deepin pr auto review

我来对这个代码修改进行审查:

  1. 代码逻辑改进:
    原代码使用QEventLoop和QTimer来处理延迟加载,存在以下问题:
  • 每次循环都创建新的QEventLoop,资源开销较大
  • 事件循环嵌套可能导致难以调试的问题
  • 代码结构不够清晰

新代码改用QSemaphore和递归调用loadNextPolicy()的方式,改进了这些问题:

  • 使用单一信号量控制同步,减少了资源开销
  • 避免了事件循环嵌套
  • 代码结构更清晰,通过递归调用实现顺序加载
  1. 代码质量改进:
    优点:
  • 添加了更多的日志输出,便于调试
  • 错误处理更完善,添加了创建服务失败时的警告日志
  • 函数职责划分更清晰,loadNextPolicy专门处理单个策略的加载

建议改进:

  • loadNextPolicy使用递归调用,当策略数量很大时可能导致栈溢出,建议改为迭代实现
  • 可以添加加载进度的回调通知,便于上层监控加载进度
  1. 性能改进:
  • 减少了QEventLoop的创建和销毁开销
  • 使用信号量替代事件循环,降低了CPU使用率
  • 递归调用相比循环可能带来额外的函数调用开销,但影响不大
  1. 安全性改进:
  • 添加了空指针检查(semaphore)
  • 改进了错误处理和日志记录
  • 保持了原有的插件路径检查机制

建议的进一步改进:

void PluginLoader::loadNextPolicy(const QList<Policy *> &pendingPolicies, QSemaphore *semaphore)
{
    if (pendingPolicies.isEmpty()) {
        if (semaphore) {
            semaphore->release();
        }
        return;
    }

    Policy *policy = pendingPolicies.first();
    QList<Policy *> remainingPolicies = pendingPolicies.mid(1);

    // 添加加载进度通知
    emit loadProgressChanged(pendingPolicies.size(), remainingPolicies.size());

    const int delay = policy->startDelay * 1000;
    QTimer::singleShot(delay, this, [this, policy, remainingPolicies, semaphore] {
        ServiceBase *srv = createService(policy);
        if (srv == nullptr) {
            qCWarning(dsm_PluginLoader) << "Failed to create service for policy:" << policy->name;
            // 可以添加失败重试机制
        } else {
            if (!policy->pluginPath.isEmpty()) {
                addPlugin(srv);
            }
        }
        
        // 使用QTimer::singleShot避免递归调用
        QTimer::singleShot(0, this, [this, remainingPolicies, semaphore] {
            loadNextPolicy(remainingPolicies, semaphore);
        });
    });
}

总体来说,这次修改是正向的,提高了代码的可维护性和性能,但还可以通过添加进度通知和避免递归调用等方式进一步改进。

@BLumia BLumia marked this pull request as draft October 29, 2025 07:56
dlopen 的形式加载插件,目前在 Arch Linux 和 openSUSE 观察到崩溃在
dlclose 时. 此处改为使用 QLibrary 加载插件并检查版本.

Log:
@BLumia BLumia changed the title chore: remove the need of QEventLoop in pluginloader fix: use QLibrary to load and check library version Oct 30, 2025
@BLumia BLumia requested a review from 18202781743 October 30, 2025 11:52
@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: 18202781743, BLumia

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@BLumia BLumia marked this pull request as ready for review October 30, 2025 11:55
@BLumia BLumia merged commit beebf9a into linuxdeepin:master Oct 30, 2025
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants