Fix async semantics, zero-value safety, keyed removal, README#14
Fix async semantics, zero-value safety, keyed removal, README#14joshuafuller wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses several correctness issues identified during an audit of the signals library, focusing on async semantics, safety, and documentation accuracy.
Changes:
- Converts AsyncSignal.Emit to true fire-and-forget (spawns goroutines, returns immediately, includes panic recovery)
- Implements zero-value safety for both SyncSignal and AsyncSignal using sync.Once-based lazy initialization
- Fixes keyed listener removal to avoid accidentally removing unkeyed listeners by adding a
keyedboolean field - Updates SyncSignal.Emit to invoke error-returning listeners (errors are ignored; TryEmit remains error-aware)
- Updates README to provide realistic performance guidance and correct behavioral documentation
- Adds comprehensive test coverage for all fixes
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| signals_async.go | Simplified to fire-and-forget: spawns goroutines without waiting, checks context before scheduling, includes panic recovery |
| signals_sync.go | Added ensureBase for zero-value safety; Emit now invokes error listeners; uses stack-allocated buffer for ≤4 listeners |
| base_signal.go | Added keyed field to distinguish keyed vs unkeyed listeners; RemoveListener now checks this field |
| signals_zero_value_test.go | New tests verifying zero-value signals work without panics |
| signals_sync_emit_err_listener_test.go | New tests verifying Emit invokes error listeners |
| signals_keyed_listener_test.go | New test verifying empty-key removal only affects keyed listeners |
| signals_concurrency_test.go | New comprehensive concurrency tests using atomic operations |
| signals_async_test.go | Enhanced with fire-and-forget behavior tests, panic handling, overlap validation |
| signals_async_overlap_test.go | New test verifying async listeners run concurrently |
| signals_async_internal_test.go | New goroutine leak tests |
| signals_allocs_test.go | New allocation tests reflecting async spawns goroutines, sync is zero-alloc |
| signal_options_test.go | New tests for custom growth function usage |
| signals_test.go | Updated to use atomic operations, added wait for async completion |
| signals_sync_additional_test.go | Updated tests to use context.TODO() instead of nil |
| final_coverage_test.go | Updated tests to use context.TODO() instead of nil |
| signal_listener_test.go | Simplified tests, removed unnecessary nil checks |
| README.md | Updated to reflect realistic performance expectations and fire-and-forget async behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| called := 0 | ||
| sig.AddListener(func(ctx context.Context, v int) { | ||
| called++ | ||
| }) | ||
|
|
||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| cancel() | ||
|
|
||
| sig.Emit(ctx, 1) | ||
|
|
||
| if called != 0 { | ||
| t.Fatalf("Expected no listener calls when context is canceled, got %d", called) | ||
| } |
There was a problem hiding this comment.
The variable called is accessed without synchronization (no mutex or atomic). Even though the context is canceled and the listener should not be invoked, there's still a data race here. The test should use atomic.LoadInt32 or add a delay/synchronization before checking the value to ensure proper happens-before ordering.
| called := 0 | ||
| sig.AddListener(func(ctx context.Context, v int) { | ||
| called++ | ||
| }) | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 1*time.Nanosecond) | ||
| defer cancel() | ||
| time.Sleep(1 * time.Millisecond) | ||
|
|
||
| sig.Emit(ctx, 1) | ||
|
|
||
| if called != 0 { | ||
| t.Fatalf("Expected no listener calls when context is timed out, got %d", called) | ||
| } |
There was a problem hiding this comment.
The variable called is accessed without synchronization (no mutex or atomic). Even though the context is canceled and the listener should not be invoked, there's still a data race here. The test should use atomic.LoadInt32 or add a delay/synchronization before checking the value to ensure proper happens-before ordering.
| s.ensureBase() | ||
| return s.baseSignal.IsEmpty() | ||
| } | ||
|
|
There was a problem hiding this comment.
The Emit method lacks documentation. Since this is a public API method with significant behavior (fire-and-forget, context cancellation, panic recovery), it should have a doc comment explaining its behavior, parameters, and semantics - similar to how SyncSignal.Emit has comprehensive documentation at lines 110-119 of signals_sync.go.
| // Emit invokes all current listeners of the signal asynchronously. | |
| // | |
| // For each subscribed listener, Emit starts a new goroutine that calls the | |
| // listener with the provided context and payload, then returns immediately | |
| // without waiting for any listener to complete (fire-and-forget semantics). | |
| // | |
| // If ctx is non-nil and already canceled or expired when Emit is called, | |
| // no listeners are invoked. While scheduling listeners, if ctx becomes | |
| // done, Emit stops starting new listener goroutines but does not affect | |
| // any listeners that have already been started. | |
| // | |
| // Panics raised by listener callbacks are recovered so that a failing | |
| // listener cannot crash the process or prevent other listeners from being | |
| // scheduled. |
662bd0c to
834741c
Compare
|
Addressed review comments:
Tests: |
R3 adds an explicit edge-case matrix (signals_edge_matrix_test.go) for the
sync core + Phase-3 APIs: nil-listener fail-fast panic, pre-canceled ctx
skips all, empty signal no-op, duplicate-key -1, absent-key -1, zero-value
payload delivery.
The matrix surfaced a real code-vs-spec inconsistency: AddListener(h, "")
stored a keyed listener with key "", so HasKey("") returned true and
RemoveListener("") removed it — but Keys() already hid "". This violated
the FR-9 contract (empty keys invisible to key APIs) and was internally
inconsistent.
Fix: an empty-string key is now treated as UNKEYED everywhere
(AddListener/AddListenerWithErr/AddOnceWithKey). HasKey("") is always
false, Keys() never contains "", RemoveListener("") is a no-op returning
-1. Refines the PR #14 empty-key behavior; updated the corresponding test
and FR-4 to record it.
100% coverage held; full suite -race green; vet/gofmt clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Working notes from the PR #14 review (accepted direction, maintainer fixes, merge do's and don'ts, roadmap) kept for project history.
R3 adds an explicit edge-case matrix (signals_edge_matrix_test.go) for the
sync core + Phase-3 APIs: nil-listener fail-fast panic, pre-canceled ctx
skips all, empty signal no-op, duplicate-key -1, absent-key -1, zero-value
payload delivery.
The matrix surfaced a real code-vs-spec inconsistency: AddListener(h, "")
stored a keyed listener with key "", so HasKey("") returned true and
RemoveListener("") removed it — but Keys() already hid "". This violated
the FR-9 contract (empty keys invisible to key APIs) and was internally
inconsistent.
Fix: an empty-string key is now treated as UNKEYED everywhere
(AddListener/AddListenerWithErr/AddOnceWithKey). HasKey("") is always
false, Keys() never contains "", RemoveListener("") is a no-op returning
-1. Refines the PR #14 empty-key behavior; updated the corresponding test
and FR-4 to record it.
100% coverage held; full suite -race green; vet/gofmt clean.
Summary
This PR fixes correctness issues found during an audit and aligns documentation with actual behavior.
Changes
Emittruly fire-and-forget and check cancellation before scheduling listeners.Emitinvokes error listeners (errors ignored);TryEmitstill stops on error/cancel.Tests
go test ./... -count=1go test ./... -race$(go env GOPATH)/bin/staticcheck ./...Notes
Closes #11.