fix(submit-chain): 完善异常与取消传播#705
Conversation
确保链任务和重复调度在失败或取消时终止,避免 Future 与协程永久挂起。
FxRayHughes
left a comment
There was a problem hiding this comment.
Code Review:PR #705 fix(submit-chain): 完善异常与取消传播
感谢提交!executeRepeat 的抽取和状态机设计相当扎实,AtomicBoolean + compareAndSet 把"重复 resume"、"取消/提交竞态"几条路径都覆盖到了。测试用 fake PlatformTask 注入的思路也很干净。
无阻止合并的严重问题,但有一处建议在合并前处理。
🟡 中等问题
1. launch → async 导致链异常不再上报控制台
文件: module/basic/basic-submit-chain/src/main/kotlin/taboolib/expansion/Chain.kt
这是 kotlinx.coroutines 的文档化行为差异:
launch的未捕获异常会走CoroutineExceptionHandler→ 默认打印到控制台async的异常按设计存入 Deferred,等待await()取出,不走CoroutineExceptionHandler
改动前后对比:
旧(launch) |
新(async) |
|
|---|---|---|
| Future 状态 | ❌ 永久 pending | ✅ 异常完成 |
| 控制台可见性 | ✅ 打印堆栈 | ❌ 完全静默 |
submitChain { ... } 的 fire-and-forget 用法(不消费返回 Future)在插件里非常常见。改动后这类调用的异常既不打印、也无人观察(CompletableFuture 同样不会上报未处理异常),等于彻底吞掉,排查难度反而比修复前更高。
建议:在 invokeOnCompletion 的 else 分支同时记录日志,兼顾两者:
else -> {
future.completeExceptionally(cause)
// 保留控制台可见性,避免 fire-and-forget 调用静默失败
PrimitiveIO.error(...) // 或项目现有日志方式
}2. 重复链中单次迭代抛异常,现在会终止整个重复任务
文件: module/basic/basic-submit-chain/src/main/kotlin/taboolib/expansion/RepeatChainable.kt
} catch (ex: Throwable) {
cancel() // ← 立即取消整个重复任务
if (completed.compareAndSet(false, true)) {
continuation.resumeWithException(ex)
}
}修复"永久挂起"是对的,但顺带把语义改成了任意一次迭代抛错 → 整个重复链终止。旧实现中异常逃逸到平台调度器后,部分平台的重复任务仍会继续轮转,因此如果有调用方依赖"某次迭代失败被跳过、重复继续"的行为,这是一处破坏性变化。
PR 描述的"兼容性与行为变化"章节没有提到这一点,建议补充说明,或者确认这就是期望语义。
🔵 小建议
1. CancellationException → future.cancel(false) 丢失原始 cause
文件: Chain.kt
如果链内部因业务逻辑主动抛出 CancellationException(而非真的被取消),调用方只会看到一个空的 CancellationException,原始信息丢失。可考虑保留 cause,或仅在 task.isCancelled 为真时才映射为 cancel。
2. future 强引用整条协程图
文件: Chain.kt
future.whenComplete { ... task.cancel() } 让 future → task → scope Job 形成强引用链,长期持有 Future 的调用方会一并保留协程对象图。影响很小,知道即可。
3. now = true 路径下 task.cancel() 可能被调用两次
文件: RepeatChainable.kt
若平台实现的 now = true 在 submitTask 返回前同步执行了回调并完成,则 taskReference.set(task) 之后的 if (completed.get()) task.cancel() 会再取消一次。行为无害(幂等),但建议加一行注释说明这是为覆盖竞态而有意为之,避免后续维护者误删。
🟢 确认正确的改动
| 改动 | 评价 |
|---|---|
Chain.run 用 invokeOnCompletion 桥接 Future |
正确修复 Future 永久 pending |
future.whenComplete 反向传播取消到协程 |
正确,取消语义双向打通 |
抽取 executeRepeat 统一同步/异步重复链 |
消除了两份重复逻辑与行为不一致 |
suspendCoroutine → suspendCancellableCoroutine |
必要,否则 invokeOnCancellation 无从挂载 |
completed: AtomicBoolean + compareAndSet |
严密,重复 resume 的 IllegalStateException 被彻底堵住 |
taskReference.set(task) 后再查 completed 二次确认 |
正确覆盖了"取消发生在 task 赋值之前"的竞态 |
submitTask 抛异常时 resumeWithException |
修复了插件关闭期调度器拒绝任务导致的永久挂起 |
submit(executor = ...) 命名参数 |
签名匹配(已核对 common-platform-api 的 Executor.kt) |
internal fun run(dispatcher) 供测试注入 |
不破坏公开 API,同模块测试可访问 |
RepeatChainTest 的 fake PlatformTask |
无需平台环境即可测调度交互,设计得好 |
总结
整体质量很好,无阻止合并的问题。
- 问题 1(异常静默) 建议在合并前处理,否则修复的收益会打折扣
- 问题 2 至少需要在 PR 描述里补充行为变化说明
其余均为可选优化。
原有问题
Submit Chain 对协程、平台调度任务和
CompletableFuture的完成状态桥接不完整:典型触发场景与后果
本 PR 修改
CompletableFuture,链执行失败时让 Future 异常完成。suspendCancellableCoroutine完成模型。修改目的
让 Chain 的成功、异常和取消语义保持一致,调用方始终能观察到最终结果,并确保取消后不再有后台任务继续执行。
兼容性与行为变化
验证
./gradlew :module:basic:basic-submit-chain:test --rerun-tasks --no-parallel./gradlew :module:basic:basic-submit-chain:build --rerun-tasks --no-parallelgit diff --checkRefs #703