Skip to content

feat: add dynamic hook dispatcher - #105

Merged
Bury-Lee merged 2 commits into
Yiming1997:mainfrom
1m55555555:feat/dynamic-hook-dispatcher
Sep 25, 2026
Merged

Bury-Lee merged 2 commits into
Yiming1997:mainfrom
1m55555555:feat/dynamic-hook-dispatcher

Conversation

@1m55555555

Copy link
Copy Markdown
Contributor

Summary

  • Add hook/dynamic, an opt-in lifecycle dispatcher that supports callback registration while a pool is processing tasks.
  • Keep the default hook dispatcher and Pool.SetHook contract unchanged.
  • Document dispatch-level snapshot semantics in English and Chinese.

Semantics

Each lifecycle event reads one immutable callback snapshot when dispatch begins. Callbacks added during a dispatch apply only to later events. Different lifecycle events for the same task may observe different snapshots.

Implementation

  • Copy-on-write callback snapshots published with atomic pointers.
  • Registration is serialized; dispatch is lock-free and reentrant.
  • A panic in one callback is recovered without preventing later callbacks from running.

Validation

  • go test ./hook/dynamic -count=1
  • go test -race ./hook/dynamic -count=1
  • go test -short ./... -count=1
  • go vet ./...

Scope

No changes to pool.go, the default hook package, scheduler, task buffer, or existing Hook contract.

@Bury-Lee

Bury-Lee commented Sep 16, 2026 •

Copy link
Copy Markdown
Collaborator

@1m55555555
Thanks for this PR — the snapshot semantics and test coverage look good. A few review points:

  1. Drop the README changes. Please keep readme.md and README_zh-CN.md untouched in this PR — it should contain only the dispatcher and its tests; the docs will be handled separately.
  2. Use the library's Stack instead of runtime/debug.Stack(). The library already exposes agilepool.Stack(skip) (stack_trace.go:26), which is what the pool's own panic logging uses (Stack(2) in pool.go:623). Switching keeps the stack format consistent across the codebase and removes the extra import. Stack(2) starts at the same frame as debug.Stack() (the deferred recover closure). (The default pool implementation uses debug because of test leftovers that haven't been cleaned up yet; this will be changed soon as well.)
  3. The default hook package will be migrated to the library's Stack as well, so there is no reason to keep a second stack format in hook/dynamic.
  4. Please add doc comments to the unexported helpers — several of them carry non-obvious semantics:
    • callbackSnapshot: one immutable generation of callbacks.
    • snapshot(): the nil check is zero-value safety, since NewHooks always stores a snapshot.
    • appendCallback: explain why plain append is not used — it may reuse spare capacity and mutate the backing array of an already published snapshot, which lock-free dispatch depends on never happening. This one is easy to "simplify" back into a bug, so it deserves a comment.
    • invoke: per-callback recovery — a panicking callback must not stop the remaining callbacks of the same event.

Happy to re-review once these are addressed.

@1m55555555

Copy link
Copy Markdown
Contributor Author

@Bury-Lee
Thanks for the review and the clear guidance.

I will update the PR as follows:

  • Remove the changes to readme.md and README_zh-CN.md.
  • Replace runtime/debug.Stack() with agilepool.Stack(2).
  • Add doc comments for callbackSnapshot, snapshot, appendCallback, and invoke, documenting their concurrency and panic-isolation semantics.
  • Keep the implementation and tests otherwise unchanged.

I will run the focused tests, race tests, the short repository test suite, and go vet before requesting another review.

@1m55555555

Copy link
Copy Markdown
Contributor Author

Hi @Bury-Lee, I have addressed all requested changes in db9fec5:\n\n- Removed the readme.md and README_zh-CN.md changes.\n- Replaced runtime/debug.Stack() with agilepool.Stack(2).\n- Added comments for callbackSnapshot, snapshot, appendCallback, and invoke, documenting the immutable snapshot and per-callback recovery semantics.\n\nValidation completed:\n- go test ./hook/dynamic -count=1\n- go test -race ./hook/dynamic -count=1\n- go test -short ./... -count=1\n- go vet ./...\n\nCould you please re-review when convenient? Thanks!

@Bury-Lee
Bury-Lee merged commit 36694d4 into Yiming1997:main Sep 25, 2026
1 check passed
Comment thread hook/dynamic/dynamic.go
poolClosed []agilepool.PoolHook
}

var _ agilepool.Hooks = (*Hooks)(nil)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This line of code is redundant and unnecessary; please remove it in the next release.

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.

2 participants