fix: make hook dispatch race-free

[1m55555555]

Summary

• Make Pool hook replacement safe while tasks are in flight using atomic snapshots.
• Make bundled hook callback registration and dispatch race-free using copy-on-write snapshots.
• Keep hook dispatch lock-free and reentrant.

Why

Pool.SetHook previously read and wrote the Hook interface without synchronization. The bundled dispatcher also appended callback slices under a lock while dispatch ranged over them without synchronization. Both patterns can trigger data races under dynamic instrumentation.

Semantics

Each lifecycle event uses the hook and callback snapshot current when dispatch starts. Hook changes or registrations occurring later apply to subsequent events.

Validation

• go test ./...
• go test -race -count=1 . ./hook -run Test(SetHookConcurrentWithTaskDispatch|ConcurrentRegistrationAndDispatch|CallbackCanRegisterAnotherCallback)
• go vet ./...

Scope

No changes to task scheduling, worker lifecycle, buffering, scaling, or public Hook interfaces.

[Yiming1997]

@Bury-Lee pls review

[Bury-Lee]

@Bury-Lee pls review

OK

[Bury-Lee]

关于 PR #102 fix: make hook dispatch race-free 的评审意见

结论：不批准合并（Request changes）。

本 PR 用 atomic.Pointer 快照让「分发期间注册回调 / 分发期间 SetHook」变为 race-free，实现本身是干净的，但它解决的是文档明确排除在契约之外的用法，并且改变了默认实现的语义边界。理由如下。

1. 数据面：没有可测收益

环境：Intel i7-14700HX（28 逻辑核）/ 15.8GB / Go 1.26.4 windows/amd64（bench-data/results/env.txt）。
对比版本：main=40366f5，pr102=6ac2043，30 组配对基准。

指标	main	pr102	差异
总耗时（30 组）	1342.1s	1343.0s	+1.0s（+0.07%）
单组耗时	—	—	29 组 ≤ ±0.06%，唯一例外 run 5 +0.94%（106.1s vs 107.1s，sleep 主导）
heap_mean	10.70MB	10.85MB	+1.4%
heap_peak	33.3MB	35.2MB	单点极值，无统计意义
sys_mean	90.1MB	89.7MB	−0.4%
GC 次数	两侧逐组一致

使用traceID 场景三次重复的 wall_s：

场景	main	pr102
nohook	7.667 / 7.653 / 7.649	7.662 / 7.654 / 7.664
hook	7.665 / 7.657 / 7.652	7.658 / 7.659 / 7.654

hook 场景耗时：

场景	main	pr102	差异
henqueue	12.216s	12.204s	−0.10%
hblock	2.328s	2.319s	−0.39%
hpanic num=3000	7.995s	8.006s	+0.14%
hchurn	0.098s	0.111s	+13%（亚 100ms，调度抖动）
hreenter	0.039s	0.046s	+18%（亚 100ms，调度抖动）

所有 hook 场景的不变量断言两侧均 PASS（[hchurn]、[horder]、[hreenter] 等输出完全一致）。

结论：该改动在数据面上没有可观察的改善，不判断为性能修复。

2. 被修复的是契约外的用法

现有契约（main 分支）写得非常明确：

• readme.md:279 / README_zh-CN.md:279：Register before the pool starts processing tasks: the hook set is read without synchronization on every dispatch.
• pool.go:598-601（main）：Register callbacks before the pool starts processing tasks ... replacing it while tasks are in flight is a data race.
• hook/hook.go 包注释：Create one with NewHooks, register callbacks, and hand it to Pool.SetHook before submitting tasks.
• docs/test-harness.md:305-312：adding a callback while events are being dispatched is outside the contract and would race on the callback slices.
• test/hchurn.go:3-7：dispatching already-running tasks with a changing callback list is outside the contract and is never done here.

契约内的用法（多 goroutine 并发 Add*，注册完成后再 SetHook/Submit）在 main 上本来就是 race-free，-race 下的 hchurn 注册风暴（4 goroutine × 100 hooks）也是全绿的。

同时引入了实际成本：热路径每次 dispatch 多一次 atomic load 与分支（每任务 4 次）；appendCallback 每次注册都复制整表（n 次注册为 O(n²)）；SetHook 多一次堆分配。收益只存在于契约外用法的 race 报告里。

而 PR 新增的测试恰好都在构造契约外用法：

• hook/hook_test.go TestConcurrentRegistrationAndDispatch：dispatch 期间 Add*（测试注释自己写明 the old append-and-range implementation reports a race here）；
• hook/hook_test.go TestCallbackCanRegisterAnotherCallback：回调内 Add*；
• hook_test.go TestSetHookConcurrentWithTaskDispatch：任务在飞时 SetHook。

对应的 race 报告也只出现在这些测试里（bench-data/results/extra/race_main_hookpkg.log、race_main_root.log）。

即：这不是在修契约内的 bug，而是在为一个明确声明不支持的用法兜底。

3. 一旦支持，语义将变得难以定义

若接受该 PR，以下行为会进入「支持」范围，但 PR 并没有给出定义：

• 同一任务的事件可能看到不同快照。 每次 Dispatch* 各自 Load，Submitted 可能看到集合 A、Started 看到集合 B、Completed 看到集合 C。测试套件里 hcount/hchurn/horder 断言的「每个回调精确看到每个事件」将不再是一条保证，而只是恰好如此。
• 回调内再 Add 的行为由实现细节决定。 新回调在同一次事件中触发 0 次还是 1 次，取决于注册时机与快照发布的先后；PR 的测试只断言 calls > 0 与两次 dispatch 后等于 3，并没有定义契约。
• 任务在飞时 SetHook。 任务可能前半程用旧 hook 集、后半程用新 hook 集；PoolClosed 用哪一套同样取决于时序。
• 快照粒度是「每次 dispatch」而非「每个任务」。 若要做到每任务一致，还需要任务级快照的传递，成本与复杂度更高。

这正是设计时刻意规避的行为。钩子是观测设施，契约把注册窗口固定在启动阶段，换来的是热路径零同步，以及「事件计数精确」这类可以直接写成断言的强语义。把默认实现改成「运行时可改」会削弱这组保证，且无法向用户解释清楚事件与回调集合的对应关系。

4. 运行时可注入应作为独立实现

agilepool.Hooks（hook.go:18-22）本身就是扩展点，注释写明 custom implementations only need the five dispatch methods below。需要运行时可注入函数的用户，应当自己实现一套 Hooks（或由项目另开一个可选包，例如 hook/dynamic）：

• 自行定义一致性语义（每次 dispatch 使用稳定快照？同一任务四个事件是否同集合？）；
• 自行承担 atomic/快照/RCU 的成本与 -race 测试；
• 默认 hook.Hooks 与 Pool.SetHook 的契约保持不变。

需要特别说明的是：如果需求是「任务在飞时 SetHook」，那确实必须改 pool.go；但这类需求应当通过新增 API 表达（例如独立的包装类型或显式命名的原子替换方法），而不是静默改变既有 SetHook 的文档语义。

5. 处理决定

本 PR 不批准合并。 具体理由如下：

1. 修改了 pool.go 与 hook/hook.go 的实现，以及三个契约外测试；现有契约与文档被破坏。
2. 若确有运行时可注入的需求，另开独立 PR：新增一个实现 agilepool.Hooks 的可选包（如 hook/dynamic），自带一致性语义文档与 -race 测试，默认实现不动。

附：数据出处

test测试工具测试

[1m55555555]

@Bury-Lee
Thank you for the detailed review and benchmark results.

I understand the main concern now. The current Hook contract intentionally requires callbacks to be registered before task processing starts, and PR #102 was addressing unsupported runtime mutation rather than a bug within the supported contract.

I also understand that making the default implementation dynamically mutable would add synchronization and snapshot semantics to the hot path, while changing the meaning of the existing API.

I agree that the current PR should not change the default Hook implementation. I will close PR #102 and, if runtime hook injection is still useful, consider a separate opt-in implementation with explicitly documented consistency semantics and dedicated race tests.

Thank you for pointing this out.

[1m55555555]

@Bury-Lee
Thanks again for the detailed feedback on PR #102.

I agree that the default hook implementation should keep its current startup-only contract and low-overhead dispatch path.

Before opening another PR, I would like to confirm a possible opt-in design for runtime hook registration:

• Add a separate package such as hook/dynamic.
• Keep hook.Hooks, Pool.SetHook, and the existing default semantics unchanged.
• Allow callbacks to be registered while the pool is running.
• Use immutable callback snapshots so registration is race-free and does not block dispatch.
• A callback registered during an event dispatch would take effect only for later dispatches.
• Different lifecycle events of the same task may observe different snapshots, and this would be explicitly documented.
• Include dedicated unit tests and -race coverage.

Would this design be aligned with the project direction?

If so, I can prepare a small draft PR for review. I am also happy to adjust the API or consistency semantics before opening it.

[Yiming1997]

@1m55555555 u can simply open an issue instead of texting in a closed pr

[Bury-Lee]

@1m55555555 I agree, it's a good idea. Thanks for taking this on. I'll review it as soon as you open it.