Skip to content

fix: add per-device internet connectivity check for network switching#557

Merged
deepin-bot[bot] merged 1 commit into
linuxdeepin:masterfrom
ut003640:master
Jun 5, 2026
Merged

fix: add per-device internet connectivity check for network switching#557
deepin-bot[bot] merged 1 commit into
linuxdeepin:masterfrom
ut003640:master

Conversation

@ut003640

@ut003640 ut003640 commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Add InternetChecker module to detect and switch the primary network interface when the current one cannot access the internet.

  • Add InternetChecker class with switchInternetAccess() logic: traverse activated devices, check which ones can access internet, compare with current primary connection, and switch via never-default (updateUnsaved + reapplyConnection, not persisted to disk)
  • Subscribe to NetworkManager::primaryConnectionChanged signal, retry connectivity checks after switch to wait for DHCP/route settling
  • Reset all connections' never-default=false after successful switch or when all devices exhausted (via NetworkManager::Settings::listConnections
    • device active connections)
  • Add needCheckNetwork config option to enable/disable the feature
  • Integrate InternetChecker into LocalConnectionvityChecker, trigger on Limited/Noconnectivity/Unknownconnectivity states

增加检测单独网卡是否可以上网并自动切换网络的功能。

  • 新增InternetChecker类,实现switchInternetAccess()逻辑:遍历所有已激活的网卡, 检测哪些网卡可以上网,与当前主连接网卡对比,若当前网卡不可上网则通过 never-default属性切换默认路由(使用updateUnsaved+reapplyConnection, 不持久化到磁盘)
  • 监听NetworkManager::primaryConnectionChanged信号,切换后重试检测网络连通性, 等待DHCP/路由生效
  • 切换成功后或所有设备都尝试过以后,重置所有连接的never-default为false (通过NetworkManager::Settings::listConnections遍历所有已保存连接, 并补充设备当前激活的连接,确保不遗漏)
  • 新增needCheckNetwork配置项控制是否启用该功能
  • 在LocalConnectionvityChecker中集成InternetChecker,在网络状态变为 Limited/Noconnectivity/Unknownconnectivity时触发检测和切换

PMS: BUG-351163

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @ut003640, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@ut003640 ut003640 force-pushed the master branch 2 times, most recently from 5c93bb9 to e494d11 Compare June 4, 2026 12:24
@ut003640

ut003640 commented Jun 4, 2026

Copy link
Copy Markdown
Contributor Author

@deepin-ci-robot
线程阻塞问题(严重):这里QThread::sleep(1),只有1毫秒,时间非常短,对事件队列的影响基本上可以忽略不计
HTTP请求的同步与异步问题(严重):HttpManager内部的网络请求我使用的是curl的接口来实现的,整个过程就是同步的,根本不会有你担心的情况
m_tryIndex 初始化与越界风险:onPrimaryConnectionChanged槽函数中,前面已经判断了m_tryIndex < m_tryDevices.size(),这里永远不会越界
配置项变更缺失:checkDeviceConnection() 和 enableLocalNotify() 原来只是在头文件中做了声明,并未在cpp中进行定义,因此这里需要将未定义的声明删除,否则生成的so库会出现未定义的符号链接
频繁的配置更新与设备重应用:设备的量很少,不存在堵塞,如果按照你说的找一个变量记录,很容易和实际的不一致,而且代码不好维护

@ut003640 ut003640 force-pushed the master branch 10 times, most recently from 262463c to 381038c Compare June 5, 2026 06:09
Add InternetChecker module to detect and switch the primary network
interface when the current one cannot access the internet.

- Add InternetChecker class with switchInternetAccess() logic:
  traverse activated devices, check which ones can access internet,
  compare with current primary connection, and switch via never-default
  (updateUnsaved + reapplyConnection, not persisted to disk)
- Subscribe to NetworkManager::primaryConnectionChanged signal,
  retry connectivity checks after switch to wait for DHCP/route settling
- Reset all connections' never-default=false after successful switch
  or when all devices exhausted (via NetworkManager::Settings::listConnections
  + device active connections)
- Add needCheckNetwork config option to enable/disable the feature
- Integrate InternetChecker into LocalConnectionvityChecker, trigger
  on Limited/Noconnectivity/Unknownconnectivity states

增加检测单独网卡是否可以上网并自动切换网络的功能。

- 新增InternetChecker类,实现switchInternetAccess()逻辑:遍历所有已激活的网卡,
  检测哪些网卡可以上网,与当前主连接网卡对比,若当前网卡不可上网则通过
  never-default属性切换默认路由(使用updateUnsaved+reapplyConnection,
  不持久化到磁盘)
- 监听NetworkManager::primaryConnectionChanged信号,切换后重试检测网络连通性,
  等待DHCP/路由生效
- 切换成功后或所有设备都尝试过以后,重置所有连接的never-default为false
  (通过NetworkManager::Settings::listConnections遍历所有已保存连接,
  并补充设备当前激活的连接,确保不遗漏)
- 新增needCheckNetwork配置项控制是否启用该功能
- 在LocalConnectionvityChecker中集成InternetChecker,在网络状态变为
  Limited/Noconnectivity/Unknownconnectivity时触发检测和切换

PMS: BUG-351163
@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

这份代码实现了一个网络自动切换功能:当检测到当前网络受限或无连接时,自动遍历其他可用的网络设备,修改其路由优先级(never-default),尝试找到一个可用的网络并切换。

整体逻辑清晰,但在线程安全、资源泄漏、阻塞调用以及平台兼容性等方面存在一些较严重的问题。以下是详细的审查意见:

1. 语法与逻辑问题

1.1 m_tryDevices 越界风险
InternetChecker::onPrimaryConnectionTimeoutonPrimaryConnectionChanged 中:

if (m_tryIndex < m_tryDevices.size()) {
    // ...
    NetworkManager::Device::Ptr nextDevice = m_tryDevices.at(m_tryIndex);
    // ...
    m_tryIndex++;
}

m_tryIndex 等于 m_tryDevices.size() - 1 时,进入 if 分支,m_tryDevices.at(m_tryIndex) 能正确获取最后一个元素。随后 m_tryIndex++ 变为 m_tryDevices.size()。如果此时 NM 又触发信号或超时,代码会进入 else 分支进行 resetAllNeverDefault(),逻辑上没问题。但建议增加防御性编程,使用 at() 替代 [](当前已使用at(),值得肯定),或在递增前做好状态判断,避免逻辑复杂化后引发越界。

1.2 HttpManager::get 逻辑分支导致 curl 未初始化即返回
HttpManager::get(const QString &url, int timeoutSec) 中:

} else if (ret == EAI_AGAIN || ...) {
    // ...
    return reply; // 提前返回
} else {
    // ...
    return reply; // 提前返回
}

CURL *curl = curl_easy_init();

如果 DNS 解析失败,函数会提前返回 reply,这是正确的。但需要注意,此时 replym_httpCode 为 0,且没有设置错误码(除了 errorMessage),调用方仅通过 httpCode() != 0 来判断网络是否可用将会失效。建议在 DNS 解析失败时,将 httpCode 设置为一个非0的特定错误码,或者确保调用方优先检查 isTimeout()

2. 代码质量

2.1 析构函数中的 deleteLater() 误用
LocalConnectionvityChecker::~LocalConnectionvityChecker()InternetChecker::~InternetChecker() 中:

m_statusChecker->deleteLater();
m_thread->deleteLater();

deleteLater() 会将删除操作投递到事件循环中,但在析构函数执行完毕后,对象本身已被销毁,其所属的事件循环可能已经停止或不再处理该对象的事件。这会导致内存泄漏。
改进意见: 在调用了 m_thread->wait() 确保线程结束后,应直接使用 delete m_statusChecker;delete m_thread;

2.2 拼写错误
LocalConnectionvityChecker 中的 Connectionvity 应为 Connectivity。虽然这是原有代码的问题,但建议顺带修正。

2.3 日志级别使用不当

qWarning() << "get primary device: " << primaryDevice->interfaceName();

获取主设备是正常的业务逻辑,不应使用 qWarning,应改为 qCInfo(DSM)qCDebug(DSM)

3. 代码性能

3.1 在工作线程中使用 QThread::sleep 阻塞
InternetChecker::checkInternetAccessibleWithRetry 中:

for (int i = 0; i < maxRetry; ++i) {
    QThread::sleep(1); // 阻塞线程1秒
    // ...
}

InternetChecker 被移动到了独立的子线程中,虽然在此线程中 sleep 不会阻塞 UI,但使用 QThread::sleep 进行轮询等待是一种反模式,会降低线程的响应能力。
改进意见: 建议使用 QTimer::singleShot 延迟触发下一次重试,或者让底层的 HTTP 请求自带重试逻辑,避免强制阻塞线程。

3.2 同步 HTTP 请求阻塞 NetworkManager 线程
HttpManager::get 是一个同步函数,内部使用 curl_easy_perform 阻塞等待结果。InternetChecker 在子线程调用它,虽然不卡 UI,但会导致该子线程长时间阻塞,无法响应退出信号(如 quit()),必须等待 HTTP 请求超时才能结束线程。
改进意见: 如果可能,考虑使用异步 HTTP 请求。如果必须同步,请确保 timeoutSec 设置合理,并在析构时做好线程强制终止的安全兜底。

4. 代码安全

4.1 严重:getaddrinfo 超时线程泄漏与资源浪费
httpmanager.cpp 中实现 getaddrinfo_with_timeout,使用 pthread 创建线程并用 pthread_timedjoin_np 等待:

if (joinRet == ETIMEDOUT) {
    pthread_detach(thread); // 脱离管理
    return ETIMEDOUT;
}

如果 DNS 请求卡死(例如路由器无响应,glibc 的 getaddrinfo 可能会阻塞长达数十秒甚至几分钟),这个子线程将一直存活并在后台占用资源。高频调用此接口会导致线程泄漏,甚至耗尽进程的 PID/线程描述符。
改进意见:

  1. 不推荐使用 pthread_cancel,因为它极易导致 getaddrinfo 内部分配的锁和内存泄漏(死锁)。
  2. 推荐使用 c-ares 库:c-ares 是一个异步 DNS 解析库,原生支持超时,完美替代阻塞的 getaddrinfo
  3. 如果必须用当前方案:建议在系统中启动一个专门的 DNS 解析单例线程,带缓存机制,避免每次 HTTP 请求都新建 pthread。

4.2 严重:pthread_timedjoin_np 不可移植
pthread_timedjoin_np 是 GNU 扩展(_np 代表 non-portable),在非 Linux/Glibc 环境(如 musl libc 或跨平台编译)下会编译失败。
改进意见: 如果代码仅需运行在 Deepin/Linux 上,可以保留,但建议在编译期增加宏判断。若考虑跨平台,应使用条件变量(pthread_cond_timedwait)配合标志位来实现超时等待。

4.3 curl_slist 内存泄漏风险
HttpManager::get(const QString &url, int timeoutSec) 中:

CURLcode curlRes = curl_easy_perform(curl);
// ... 处理结果 ...

if (resolveList)
    curl_slist_free_all(resolveList);
curl_easy_cleanup(curl);
return reply;

如果在 curl_easy_perform 之前发生异常或提前 return(虽然当前代码没有,但未来维护可能增加),resolveListcurl 都会泄漏。
改进意见: 建议使用 RAII 机制管理 CURL 和 curl_slist 的生命周期,例如使用 std::unique_ptr 配合自定义 deleter:

auto slist_deleter = [](curl_slist *s) { curl_slist_free_all(s); };
std::unique_ptr<curl_slist, decltype(slist_deleter)> resolveGuard(resolveList, slist_deleter);

auto curl_deleter = [](CURL *c) { curl_easy_cleanup(c); };
std::unique_ptr<CURL, decltype(curl_deleter)> curlGuard(curl, curl_deleter);

4.4 IPv6 地址格式未做合规处理

if (result->ai_family == AF_INET6) {
    // ...
    resolvedIp = QString::fromLatin1(ipStr);
}
// ...
std::string resolveStr = host.toStdString() + ":" + std::to_string(port) + ":" + resolvedIp.toStdString();

当解析到 IPv6 地址时,resolvedIp 形如 fe80::1。在 CURLOPT_RESOLVE 的字符串格式 <host>:<port>:<ip> 中,IPv6 地址不需要用方括号 [] 包裹,当前代码的拼接方式是正确的。但建议在旁边增加注释说明,以免后续维护者误以为 IPv6 需要加 [] 导致 Bug。

总结与修改建议代码片段

1. 修复析构函数内存泄漏:

LocalConnectionvityChecker::~LocalConnectionvityChecker()
{
    m_thread->quit();
    m_thread->wait();
    delete m_statusChecker; // 修改: deleteLater -> delete
    delete m_thread;        // 修改: deleteLater -> delete
    if (m_internetCheckerThread) {
        m_internetCheckerThread->quit();
        m_internetCheckerThread->wait();
        delete m_internetCheckerThread; // 修改: deleteLater -> delete
        // m_internetChecker 已通过 finished 信号关联 deleteLater,
        // 但由于线程已停止,事件循环不再运行,deleteLater 不会生效!
        // 必须显式 delete
        delete m_internetChecker; 
    }
}

2. 优化 HTTP 请求的 RAII 管理:

HttpReply *HttpManager::get(const QString &url, int timeoutSec)
{
    HttpReply *reply = new HttpReply(this);
    // ... DNS 解析部分 ...

    CURL *curl = curl_easy_init();
    if (!curl) {
        // ...
        return reply;
    }
    
    // 使用 RAII 确保 curl 资源释放
    auto curl_deleter = [](CURL *c) { curl_easy_cleanup(c); };
    std::unique_ptr<CURL, decltype(curl_deleter)> curlGuard(curl, curl_deleter);

    curl_slist *resolveList = nullptr;
    if (!resolvedIp.isEmpty()) {
        std::string resolveStr = host.toStdString() + ":" + std::to_string(port) + ":" + resolvedIp.toStdString();
        resolveList = curl_slist_append(nullptr, resolveStr.c_str());
        curl_easy_setopt(curl, CURLOPT_RESOLVE, resolveList);
    }
    
    // 使用 RAII 确保 slist 资源释放
    auto slist_deleter = [](curl_slist *s) { curl_slist_free_all(s); };
    std::unique_ptr<curl_slist, decltype(slist_deleter)> resolveGuard(resolveList, slist_deleter);

    // ... 设置 curl 参数并执行 ...
    
    return reply; // 离开作用域自动清理 curl 和 resolveList
}

@deepin-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mhduiy, ut003640

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

@ut003640

ut003640 commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

/force

@ut003640

ut003640 commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

/merge

@deepin-bot

deepin-bot Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

This pr cannot be merged! (status: unstable)

@ut003640

ut003640 commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

/forcemerge

@deepin-bot

deepin-bot Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

This pr force merged! (status: unstable)

@deepin-bot deepin-bot Bot merged commit 75859aa into linuxdeepin:master Jun 5, 2026
17 of 19 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