fix(common): 修复共享基础组件并发竞态#707
Conversation
保证事件监听、类访问器缓存与延迟集合并发安全,并让命令树只构建一次且隔离每次执行结果。
FxRayHughes
left a comment
There was a problem hiding this comment.
Code Review:PR #707 fix(common): 修复共享基础组件并发竞态
这个 PR 质量很高。修复的三处竞态都真实存在,我逐一核对了 Kotlin/Java 标准库的实际行为,结论都站得住。测试用 CyclicBarrier / CountDownLatch 握手确定性地制造交错,设计得比"跑一万次看会不会挂"扎实得多。
没有阻止合并的问题,但有两处行为变化建议补充到说明里。
说明:以下结论基于源码与标准库文档化行为推导,未实跑 gradle 测试。
🟡 中等问题(行为变化,建议补充说明)
1. commandBuilder 从"每次执行都跑"改为"注册时只跑一次"
文件: common-platform-api/src/main/kotlin/taboolib/common/platform/command/CommandRegister.kt
改动前,CommandExecutor.execute 和 CommandCompleter.execute 内部各自 CommandBase().also(commandBuilder)——每次执行命令、每敲一个 Tab 键都重建整棵命令树。改动后树在注册阶段构建一次并复用。
性能上是明显收益(Tab 补全不再每次重建树)。但存在语义变化:若插件在 commandBuilder 里依赖运行时状态构建节点,例如
command("test") {
// 从配置读取的列表
configList.forEach { literal(it) { ... } }
}改动前每次执行都会重新读取 configList,配置热重载后命令树自动跟着变;改动后树在注册时冻结,重载配置不再生效。
影响面有限——TabooLib 生态里动态内容多数走 dynamic { suggestion { ... } },而 suggestion lambda 仍是每次执行时调用,不受影响。但 literal(*运行时列表) 这类写法会静默失效。
PR 描述写的是"保持现有公开注册和命令 API"——签名确实没变,但语义变了。建议在兼容性章节明确写出这一点,让插件作者知道需要检查。
2. ClassVisitorHandler.getClasses() 现在返回 unmodifiableSet
文件: common-util/src/main/java/taboolib/common/inject/ClassVisitorHandler.java
current = Collections.unmodifiableSet(new LinkedHashSet<>(initialized));这是 public static API。框架内部两个调用点(injectAll 的 for 遍历、PlatformFactory.inject 的 for 遍历)我都核对过,均为只读,不受影响。但外部插件若曾对返回集合做增删,改动后会拿到 UnsupportedOperationException。
用 LinkedHashSet 拷贝保留了插入顺序,这点是对的(injectAll 依赖遍历顺序稳定)。建议同样写入兼容性说明。
🔵 小建议
新增的 ClassVisitorHandlerConcurrencyTest 通过反射改写静态字段 classes,虽然在 finally 里恢复了,但若同 JVM 内有其他测试并行调用 getClasses(),仍可能互相干扰。当前 Gradle 配置是 --no-parallel,暂时安全,知道即可。
🟢 确认正确的改动
事件总线的丢失更新(核心修复)
Kotlin 的 getOrPut 是 get-then-put,非原子:
public inline fun <K, V> MutableMap<K, V>.getOrPut(key: K, defaultValue: () -> V): V {
val value = get(key)
return if (value == null) {
val answer = defaultValue()
put(key, answer) // ← 两个线程都走到这里,后者覆盖前者
answer
} else value
}原代码 registeredListeners 声明为 ConcurrentHashMap<Class<*>, MutableMap<...>>,.getOrPut 解析到的是 MutableMap 扩展函数(而非任何 ConcurrentMap 原子方法),内外两层都存在丢失更新:
- 外层:两个线程为同一事件类各建一个
ConcurrentSkipListMap,后写覆盖前写 → 前者注册的监听器整批丢失 - 内层:两个线程为同一 priority 各建一个
CopyOnWriteArrayList,同样覆盖
改为 computeIfAbsent 后两层都原子化,这正是 PR 描述的"同优先级并发注册互相覆盖"。真实 bug,修得对。
Java 8 Collections.synchronizedMap 不覆写 computeIfAbsent
原代码:
delayedClasses = Collections.synchronizedMap(new HashMap<>());
...
delayedClasses.computeIfAbsent(delayTo, k -> Collections.synchronizedSet(new HashSet<>()))Collections.SynchronizedMap 在 Java 8 中没有覆写 computeIfAbsent,因此走的是 Map 接口的默认实现——完全不在锁内。项目 jvmTarget = 1.8,这是真实竞态。改为 ConcurrentHashMap 后原子性由实现保证。
propertyMap 的 CME 风险
injectAll 里 for (Map.Entry<Byte, VisitorGroup> entry : propertyMap.entrySet()) 遍历时并未持有 synchronizedNavigableMap 的锁——若此时有 register() 并发写入,会抛 ConcurrentModificationException。改为 ConcurrentSkipListMap 后迭代器是弱一致的,且 Byte 键的自然顺序语义不变。
其余
| 改动 | 评价 |
|---|---|
classes 加 volatile + 双重检查锁 + 不可变拷贝 |
修复不安全发布(旧代码可能读到并行流未填充完的集合)与重复扫描 |
| 赋值放在集合完全构建后 | 关键:旧代码 classes = candidates.parallelStream()... 是先赋值再填充的写法 |
delayedClasses.get → remove 前置 |
领取动作原子化,避免并发重复处理同一批延迟类 |
isListening 改为空安全链式 |
消除旧代码 registeredListeners[cls]!! 在 containsKey 与取值之间被清空时的 NPE 窗口 |
ThreadLocal<ArrayDeque<Boolean>> 结果栈 |
正确隔离并发执行与嵌套调用;finally 里 removeLast + 空栈时 remove() 避免 ThreadLocal 泄漏(CommandBase 现在是注册期长生命周期对象,这个清理很有必要) |
共享 CommandBase 消除每次击键重建树 |
顺带的性能收益 |
| 双重检查锁无重入死锁 | 已核对:scanClasses 内的 checkPlatform / checkRequires 不回调 getClasses(),并行流工作线程不会争抢同一监视器 |
| 测试用 barrier/latch 握手强制交错 | 确定性复现竞态,比概率性压测可靠 |
总结
三处竞态修复都切中真实问题,尤其事件总线的 getOrPut 丢失更新和 Java 8 synchronizedMap.computeIfAbsent 失效这两处,属于很难靠日志排查出来的隐蔽 bug。
建议合并前把两处行为变化(commandBuilder 只跑一次、getClasses() 返回不可变集合)补进兼容性说明,方便插件作者自查。
原有问题
多个全局共享基础组件使用了非原子的“读取—修改—写回”或无保护可变集合:
ClassVisitorHandler的访问器、延迟类集合和懒加载类集在并行扫描、注册与读取时缺少安全发布。典型触发场景与后果
ConcurrentModificationException、漏扫类或读取到未完整初始化的集合。本 PR 修改
ClassVisitorHandler的共享集合增加线程安全访问和可靠发布,统一延迟初始化流程。修改目的
消除框架启动和运行阶段的时序依赖,使事件注册、类扫描和命令系统在并发环境下保持确定性,不再出现难以复现的监听器丢失或状态串扰。
兼容性与行为变化
验证
./gradlew :common-util:test :common-platform-api:test --rerun-tasks --no-parallel./gradlew :common-util:build :common-platform-api:build --rerun-tasks --no-parallelgit diff --checkRefs #703