Skip to content

fix: serialize NetworkManagerQt on one thread - #649

Merged
caixr23 merged 1 commit into
linuxdeepin:masterfrom
caixr23:master
Oct 23, 2026
Merged

caixr23 merged 1 commit into
linuxdeepin:masterfrom
caixr23:master

Conversation

@caixr23

@caixr23 caixr23 commented Oct 22, 2026

Copy link
Copy Markdown
Contributor
  1. libKF6NetworkManagerQt keeps its device table in a lock-free container owned by whichever thread first touches the singleton. NetworkThread, StatusChecker and NetworkManager::Notifier were running on three different threads, so any device add/remove raced with a concurrent read and crashed in std::_Rb_tree_increment (SIGBUS/SIGABRT) inside deepin-service-manager.
  2. Add system/nmaccess.{h,cpp}: NetworkManagerProxy is now the only entry to NetworkManagerQt. It caches DeviceSnapshot values behind a QReadWriteLock, queues every write operation onto one process-wide "nmqt-access" thread, and republishes statusChanged/devicesChanged/primaryConnectionChanged so callers stop connecting the library's Notifier directly. A NMQT_ASSERT_THREAD() sentinel logs any access from a foreign thread.
  3. Move the blocking HTTP connectivity probe off the NM thread into connectivityprober.{h,cpp}, passing ConnectivityProbeRequest and ConnectivityProbeResult by value, so curl can no longer stall the NM event loop.
  4. Port every consumer to the proxy: NetworkThread, StatusChecker, NMConnectionvityChecker, NetworkInitialization, SystemIPConflict, InternetChecker, NetworkHandler, NetworkDBus, ConnectivityProcesser and the account-network variants. Objects that belong on the NM thread are created without a parent and destroyed through BlockingQueuedConnection, since moveToThread silently fails on parented objects.
  5. In session (non-root) plugins the proxy is anchored on the plugin thread instead of spawning a second thread, so the single-owner invariant also holds where NM Qt is used from one thread only.
  6. Rebuild the caches when NetworkManager restarts (serviceDisappeared/serviceAppeared), prune stale per-device connections, and make SettingConfig/NetworkEnabledConfig safe for concurrent readers.
  7. Restore behaviour lost while restructuring: keep the Notifier::statusChanged subscription so VPN auto-connect still fires when the network comes up, require the target device to be activated before lowering other route metrics, reconnect the switchSuccess-driven recheck, and expire the cached portal URL once the primary connection changes.

Log: deepin-service-manager no longer crashes during reboot stress or Wi-Fi switching; network status, portal URL and VPN auto-connect behave as before.

Influence:

  1. Run the reboot stress script for 1000 rounds and confirm coredumpctl list deepin-service-manager stays empty.
  2. Toggle Wi-Fi off/on about 35 times, then restart NetworkManager: the daemon PID must not change and the journal must not contain "NM Qt wrong thread".
  3. In control center verify Connectivity, PortalUrl, VpnEnabled and IsDeviceEnabled return sane values, and that a VPN with autoconnect starts once the network is up.

fix: 将 NetworkManagerQt 访问收敛到单一线程

  1. libKF6NetworkManagerQt 的设备表是无锁容器,归首个触碰该单例的 线程所有。NetworkThread、StatusChecker 与 NetworkManager::Notifier 分别跑在三条线程上,设备增删与并发读取竞争,崩溃在 std::_Rb_tree_increment(SIGBUS/SIGABRT),进程为 deepin-service-manager。
  2. 新增 system/nmaccess.{h,cpp}:NetworkManagerProxy 成为 NetworkManagerQt 的唯一入口。它用 QReadWriteLock 缓存 DeviceSnapshot,把全部写操作排队到进程内唯一的 "nmqt-access" 线程,并转发 statusChanged/devicesChanged/primaryConnectionChanged, 调用方不再直接连接库的 Notifier。NMQT_ASSERT_THREAD() 哨兵会记录 任何来自其它线程的访问。
  3. 把阻塞的 HTTP 连通性探测移出 NM 线程,放到 connectivityprober.{h,cpp},按值传递 ConnectivityProbeRequest 与 ConnectivityProbeResult,curl 不再阻塞 NM 事件循环。
  4. 所有使用方改为经代理访问:NetworkThread、StatusChecker、 NMConnectionvityChecker、NetworkInitialization、SystemIPConflict、 InternetChecker、NetworkHandler、NetworkDBus、ConnectivityProcesser 以及账户网络相关实现。需要归属 NM 线程的对象一律不带 parent 创建 (带 parent 时 moveToThread 会静默失败),析构经 BlockingQueuedConnection 回到该线程执行。
  5. session(非 root)插件中代理锚定在插件线程而非另起线程,使 “单一线程持有”的不变式在只有一条线程使用 NM Qt 的场景下同样成立。
  6. NetworkManager 重启时(serviceDisappeared/serviceAppeared)重建 缓存,按存活集合裁剪设备连接记录,并让 SettingConfig/NetworkEnabledConfig 支持并发读取。
  7. 补回重构中丢失的行为:保留 Notifier::statusChanged 订阅,使网络 就绪后自动 VPN 仍会触发;下调其它网卡路由 metric 前要求目标网卡 已激活;恢复 switchSuccess 触发的重新检测;主连接变化后让缓存的 portal 地址失效。

Log: deepin-service-manager 在重启压测与 Wi-Fi 反复切换下不再崩溃; 网络状态、门户地址与 VPN 自动连接行为保持不变。

Influence:

  1. 执行 1000 轮重启压测脚本,确认 coredumpctl list deepin-service-manager 无新增记录。
  2. Wi-Fi 反复断开/连接约 35 轮,再重启 NetworkManager:守护进程 PID 不应变化,日志中不应出现 "NM Qt wrong thread"。
  3. 在控制中心确认 Connectivity、PortalUrl、VpnEnabled 与 IsDeviceEnabled 返回值正常,且网络就绪后设置了自动连接的 VPN 能够 自动连上。

PMS: BUG-380323

@sourcery-ai

sourcery-ai Bot commented Oct 22, 2026

Copy link
Copy Markdown

Sorry @caixr23, your pull request is larger than the review limit of 150,000 diff characters

1. libKF6NetworkManagerQt keeps its device table in a lock-free
   container owned by whichever thread first touches the singleton.
   NetworkThread, StatusChecker and NetworkManager::Notifier were
   running on three different threads, so any device add/remove raced
   with a concurrent read and crashed in std::_Rb_tree_increment
   (SIGBUS/SIGABRT) inside deepin-service-manager.
2. Add system/nmaccess.{h,cpp}: NetworkManagerProxy is now the only
   entry to NetworkManagerQt. It caches DeviceSnapshot values behind
   a QReadWriteLock, queues every write operation onto one
   process-wide "nmqt-access" thread, and republishes
   statusChanged/devicesChanged/primaryConnectionChanged so callers
   stop connecting the library's Notifier directly. A
   NMQT_ASSERT_THREAD() sentinel logs any access from a foreign
   thread.
3. Move the blocking HTTP connectivity probe off the NM thread into
   connectivityprober.{h,cpp}, passing ConnectivityProbeRequest and
   ConnectivityProbeResult by value, so curl can no longer stall the
   NM event loop.
4. Port every consumer to the proxy: NetworkThread, StatusChecker,
   NMConnectionvityChecker, NetworkInitialization, SystemIPConflict,
   InternetChecker, NetworkHandler, NetworkDBus, ConnectivityProcesser
   and the account-network variants. Objects that belong on the NM
   thread are created without a parent and destroyed through
   BlockingQueuedConnection, since moveToThread silently fails on
   parented objects.
5. In session (non-root) plugins the proxy is anchored on the plugin
   thread instead of spawning a second thread, so the single-owner
   invariant also holds where NM Qt is used from one thread only.
6. Rebuild the caches when NetworkManager restarts
   (serviceDisappeared/serviceAppeared), prune stale per-device
   connections, and make SettingConfig/NetworkEnabledConfig safe for
   concurrent readers.
7. Restore behaviour lost while restructuring: keep the
   Notifier::statusChanged subscription so VPN auto-connect still
   fires when the network comes up, require the target device to be
   activated before lowering other route metrics, reconnect the
   switchSuccess-driven recheck, and expire the cached portal URL
   once the primary connection changes.

Log: deepin-service-manager no longer crashes during reboot stress or
Wi-Fi switching; network status, portal URL and VPN auto-connect behave
as before.

Influence:
1. Run the reboot stress script for 1000 rounds and confirm
   `coredumpctl list deepin-service-manager` stays empty.
2. Toggle Wi-Fi off/on about 35 times, then restart NetworkManager:
   the daemon PID must not change and the journal must not contain
   "NM Qt wrong thread".
3. In control center verify Connectivity, PortalUrl, VpnEnabled and
   IsDeviceEnabled return sane values, and that a VPN with
   autoconnect starts once the network is up.

fix: 将 NetworkManagerQt 访问收敛到单一线程

1. libKF6NetworkManagerQt 的设备表是无锁容器,归首个触碰该单例的
   线程所有。NetworkThread、StatusChecker 与 NetworkManager::Notifier
   分别跑在三条线程上,设备增删与并发读取竞争,崩溃在
   std::_Rb_tree_increment(SIGBUS/SIGABRT),进程为
   deepin-service-manager。
2. 新增 system/nmaccess.{h,cpp}:NetworkManagerProxy 成为
   NetworkManagerQt 的唯一入口。它用 QReadWriteLock 缓存
   DeviceSnapshot,把全部写操作排队到进程内唯一的 "nmqt-access"
   线程,并转发 statusChanged/devicesChanged/primaryConnectionChanged,
   调用方不再直接连接库的 Notifier。NMQT_ASSERT_THREAD() 哨兵会记录
   任何来自其它线程的访问。
3. 把阻塞的 HTTP 连通性探测移出 NM 线程,放到
   connectivityprober.{h,cpp},按值传递 ConnectivityProbeRequest 与
   ConnectivityProbeResult,curl 不再阻塞 NM 事件循环。
4. 所有使用方改为经代理访问:NetworkThread、StatusChecker、
   NMConnectionvityChecker、NetworkInitialization、SystemIPConflict、
   InternetChecker、NetworkHandler、NetworkDBus、ConnectivityProcesser
   以及账户网络相关实现。需要归属 NM 线程的对象一律不带 parent 创建
   (带 parent 时 moveToThread 会静默失败),析构经
   BlockingQueuedConnection 回到该线程执行。
5. session(非 root)插件中代理锚定在插件线程而非另起线程,使
   “单一线程持有”的不变式在只有一条线程使用 NM Qt 的场景下同样成立。
6. NetworkManager 重启时(serviceDisappeared/serviceAppeared)重建
   缓存,按存活集合裁剪设备连接记录,并让
   SettingConfig/NetworkEnabledConfig 支持并发读取。
7. 补回重构中丢失的行为:保留 Notifier::statusChanged 订阅,使网络
   就绪后自动 VPN 仍会触发;下调其它网卡路由 metric 前要求目标网卡
   已激活;恢复 switchSuccess 触发的重新检测;主连接变化后让缓存的
   portal 地址失效。

Log: deepin-service-manager 在重启压测与 Wi-Fi 反复切换下不再崩溃;
网络状态、门户地址与 VPN 自动连接行为保持不变。

Influence:
1. 执行 1000 轮重启压测脚本,确认
   `coredumpctl list deepin-service-manager` 无新增记录。
2. Wi-Fi 反复断开/连接约 35 轮,再重启 NetworkManager:守护进程 PID
   不应变化,日志中不应出现 "NM Qt wrong thread"。
3. 在控制中心确认 Connectivity、PortalUrl、VpnEnabled 与
   IsDeviceEnabled 返回值正常,且网络就绪后设置了自动连接的 VPN 能够
   自动连上。

PMS: BUG-380323
@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

🤖 AI 代码审查报告

总体评分: 96 分 (通过阈值: 70分)

Pass


📊 总体评价

项目 结果
审查结论 代码审查通过
评分详情 本提交将所有 NetworkManagerQt 调用序列化到单一 NM 线程上,修复了严重的线程安全问题和空指针解引用 bug,四维度均通过审查,未发现安全漏洞。代码结构清晰,注释详尽,性能通过将阻塞操作移出 NM 事件循环得到显著改善。

🔍 详细分析

1. 语法逻辑 ✅

评价: 语法正确,逻辑清晰 ✅ 通过

潜在问题:
✅ 未发现明显问题

建议: ['NMConnectionvityChecker 的 m_connectivity 初始化可改为 m_connectivity.store(static_cast(network::service::Connectivity::Full)) 以更明确表达原子操作意图']


2. 代码质量 ✅

评价: 代码结构清晰,注释完整 ✅ 通过

潜在问题:
✅ 未发现明显问题

建议: ['可考虑将 onDevicesChanged 中的设备清理逻辑提取为独立函数,提升可读性', 'applyPrimaryDeviceMetrics 中的 metric 设置逻辑可提取为辅助方法']


3. 代码性能 ✅

评价: 性能良好,资源使用合理 ✅ 通过

潜在问题:
✅ 未发现明显问题

建议: ['未来可考虑增量快照更新,仅刷新发生变化的设备,减少拷贝开销']


4. 代码安全 🔒

评价: 存在0个安全漏洞 ✅ 通过

🔐 发现 0 个安全漏洞

安全漏洞详情:
✅ 未发现安全漏洞

建议: ['线程安全改造已较为完善,建议在 CI 中加入 ThreadSanitizer 构建以持续监控竞态回归']


💡 改进建议代码示例

// NMConnectionvityChecker 构造函数中 atomic 初始化可更明确:
NMConnectionvityChecker::NMConnectionvityChecker(QObject *parent)
    : ConnectivityChecker(parent)
{
    m_connectivity.store(static_cast<int>(network::service::Connectivity::Full));
    moveToThread(networkManagerThread());
    QMetaObject::invokeMethod(this, &NMConnectionvityChecker::initMember, Qt::QueuedConnection);
    QMetaObject::invokeMethod(this, &NMConnectionvityChecker::initConnection, Qt::QueuedConnection);
}

本报告由 AI 代码审查工具自动生成

@deepin-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

@caixr23
caixr23 merged commit f489b92 into linuxdeepin:master Oct 23, 2026
14 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