assorted cleanups, improve docs, and add more benchmarks#2
assorted cleanups, improve docs, and add more benchmarks#2
Conversation
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
There was a problem hiding this comment.
Pull request overview
This PR modernizes the pubsub package’s public API/docs and reorganizes test/benchmark coverage to be more robust and externally focused.
Changes:
- Converted tests to an external test package (
pubsub_test), added cleanup, and made tests less prone to hanging (timeouts). - Improved
PublisherAPI documentation and switched internal/public types toany-based signatures. - Added a dedicated benchmark suite covering publish/subscribe/evict/close and topic-filtered publishing.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| publisher.go | Updates docs and refactors types/signatures to use any, clarifying Publisher semantics. |
| publisher_test.go | Moves tests to pubsub_test, adds cleanup/timeouts, and adjusts race test logic. |
| publisher_bench_test.go | Introduces a comprehensive set of benchmarks and helper drainers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
topicFunc was not exported, so just inlining the signature; also rename the arguments to slightly clarify the intent of the function and improve GoDoc. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- use t.Cleanup to close the publisher - add timeouts - prevent potential deadlock in newTestSubscriber - make TestPubSubRace slightly more deterministic Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Add/rewrite benchmarks for separate parts to reduce some noise Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // The match function acts as a per-subscriber filter and is invoked | ||
| // synchronously by Publish for each subscriber. It must be fast and | ||
| // non-blocking: a slow or blocking match will delay Publish from returning | ||
| // and will keep the Publisher's read lock held longer, and may delay | ||
| // starting delivery to other subscribers in the same Publish call. |
There was a problem hiding this comment.
The SubscribeTopic comment says match is invoked synchronously by Publish and may delay starting delivery to other subscribers. In the current implementation, match runs inside sendTopic goroutines (though Publish waits while holding the read lock), so the main impact is delaying Publish completion and blocking Subscribe/Evict/Close while the read lock is held, not delaying starting delivery to other subscribers. Please adjust the doc comment to reflect the actual concurrency/locking behavior to avoid misleading API users.
| // The match function acts as a per-subscriber filter and is invoked | |
| // synchronously by Publish for each subscriber. It must be fast and | |
| // non-blocking: a slow or blocking match will delay Publish from returning | |
| // and will keep the Publisher's read lock held longer, and may delay | |
| // starting delivery to other subscribers in the same Publish call. | |
| // The match function acts as a per-subscriber filter and is invoked from | |
| // goroutines started by Publish while the Publisher holds a read lock. | |
| // It must be fast and non-blocking: a slow or blocking match will delay | |
| // Publish from returning and will keep the Publisher's read lock held | |
| // longer, which can in turn delay operations that require the write lock | |
| // (such as Subscribe, Evict, or Close). |
| match := func(m any) bool { | ||
| v := m.(statsLike) | ||
| sink = v | ||
| return true | ||
| } |
There was a problem hiding this comment.
This benchmark's match function writes to the package-level sink from multiple sendTopic goroutines concurrently, which will be reported as a data race if benchmarks are run under -race. Consider removing the shared write (the type assertion itself is enough to keep the work) or storing via synchronization (e.g., atomic.Value) if you need to retain a value.
| match := func(m any) bool { | ||
| v := m.(statsLike) | ||
| sink = v | ||
| return true | ||
| } |
There was a problem hiding this comment.
This benchmark's match function writes to the package-level sink from multiple goroutines concurrently, which is a data race under go test -race -bench. Consider avoiding shared state in match (or use synchronization) so the benchmarks remain race-clean.
| match := func(m any) bool { | ||
| v := m.(*statsLike) | ||
| sink = v | ||
| return true | ||
| } |
There was a problem hiding this comment.
This benchmark's match function assigns to the global sink from multiple goroutines, which is a data race under the race detector. If you only want to ensure the assertion isn't optimized away, you can drop the sink write; otherwise use an atomic/synchronized store.
| match := func(m any) bool { | ||
| v := m.(*statsLike) | ||
| sink = v | ||
| return true | ||
| } |
There was a problem hiding this comment.
This match function updates the package-level sink from multiple goroutines concurrently, which will trigger the race detector when running benchmarks with -race. Consider removing the write or switching to a concurrency-safe mechanism (atomic.Value/mutex) if retaining the value is required.
see individual commits for details