Audit fixes: async semantics, zero-value safety, keyed removal

[joshuafuller]

Summary

This issue tracks a set of correctness and documentation fixes identified during a security/quality audit.

Problems Found

• Async Emit behavior was effectively blocking and did not consistently respect cancellation timing.
• Zero-value signals could be unsafe to use without explicit initialization.
• Keyed listener removal could remove unkeyed listeners.
• Sync Emit skipped error-returning listeners (TryEmit is the error-propagating path).
• README performance and behavior claims did not match actual behavior.

Fixes Implemented

• Async Emit is now fire-and-forget and checks context cancellation before scheduling listeners.
• Zero-value signals are safe to use.
• Keyed removal only removes keyed listeners.
• Sync Emit invokes error listeners (ignores their errors) while TryEmit still stops on error/cancel.
• README updated to reflect real behavior and benchmark guidance.

Tests

• go test ./... -count=1

Related

• PR: from fork joshuafuller:fix/all

[joshuafuller]

Audit Result: Fixes Implemented in PR #12

Below is a detailed summary of the problems found, the fixes applied, and how to validate them. This is intended to give reviewers a clear map of what changed and why.

Problems Found

• Async Emit behavior was not reliably fire-and-forget. This could block callers unexpectedly and create latency spikes under load.
• Cancellation timing was inconsistent in async paths, which made listener execution nondeterministic in a way that was not documented.
• Zero-value signals were unsafe to use without explicit initialization.
• Keyed listener removal could remove unkeyed listeners (if key was empty or not properly distinguished).
• Sync Emit skipped error-returning listeners, making its behavior diverge from the API contract and examples.
• README performance and behavior claims were not accurate (sub‑10ns, zero‑alloc, “Emit waits”).

Fixes Implemented (PR #12)

• AsyncSignal Emit now truly fire‑and‑forget and returns immediately after scheduling listeners.
• Context cancellation is checked before scheduling each listener in async emit.
• Zero‑value signals are safe to use without explicit init.
• Keyed removal only removes keyed listeners, eliminating accidental deletion of unkeyed ones.
• Sync Emit now invokes error listeners (ignoring errors); TryEmit still stops on error/cancel.
• Allocation tests reflect reality (async emits allocate; sync steady‑state remains low‑alloc).
• README updated to match actual behavior and benchmark guidance.

Tests / Verification

Run the full test suite:

go test ./... -count=1

Key tests added/updated:

• signals_async_test.go (async semantics, cancellation behavior, non‑blocking emit)
• signals_zero_value_test.go (zero‑value safety)
• signals_keyed_listener_test.go (keyed removal correctness)
• signals_sync_emit_err_listener_test.go (sync error listeners invoked)
• signals_allocs_test.go (allocation expectations)

PR Link

• PR: Fix async semantics, zero-value safety, keyed removal, README #12 (from fork joshuafuller:fix/all)

[aamironline]

Hi @joshuafuller

Thank you very much for the detailed PR and for taking the time to audit the project so thoroughly. I genuinely appreciate the depth of effort you’ve put into this – especially around correctness, race checks, staticcheck, and documentation alignment. 🙏

This PR touches a fairly large surface area of the library and also changes some behavioral aspects (for example around async emission, error handling, and listener semantics). A few of the items you’ve changed were actually deliberate design decisions in the original API, so I’ll need to carefully review those parts to make sure we don’t unintentionally change the intended contract of the library.

Because of that, it will take me some time to go through this properly and evaluate:

• which changes are pure correctness / safety fixes, and
• which ones introduce behavioral or design-level changes that may be breaking or require a separate design discussion.

Regarding the README and benchmark notes:
the benchmark numbers and claims in the original README were based on real runs on my own machine and local setup. They are not theoretical or estimated figures. That said, I agree that it’s important to document clearly how users should run benchmarks and interpret the results, so I’ll review your documentation changes in that light as well.

Thanks again for the high-quality contribution and for approaching this with an engineering mindset. I really value this kind of careful review and will follow up once I’ve completed a thorough pass over the changes.

[joshuafuller]

Hi Mohamed,

Thanks for the thoughtful reply. I completely understand the need to carefully review changes that touch core behavior, and I appreciate you taking the time to distinguish between correctness fixes and design-level changes. Let me walk through what we changed and why, which should help you evaluate what to keep, modify, or reject.

The Bug Fixes

Several changes address actual correctness issues that could cause problems in production:

The original signals panic if used as zero-values. If someone declares var sig signals.SyncSignal[int] as a struct field and calls AddListener without explicitly constructing it, they get a nil pointer dereference. We added lazy initialization via sync.Once so zero-value signals just work. This is purely additive and does not change behavior for properly constructed signals.

There is a subtle bug in RemoveListener where an anonymous listener (one added without a key) has an empty string as its key field. If someone calls RemoveListener(""), it could incorrectly match and remove that anonymous listener. We added a keyed boolean to distinguish between "explicitly keyed with empty string" and "not keyed at all." This is a correctness fix, though admittedly an edge case.

The growthFunc option in SignalOptions was never actually applied. The code accepted the function but then just used Go’s default slice growth on append. We added ensureCapacity() calls so the user-provided growth function is actually honored. If nobody was using this option, the fix is invisible. If someone was, their code now works as they expected.

We also fixed a race condition window in the emit paths. The subscriber slice needs to be copied while holding the read lock, but there were code paths where the lock was released before the copy completed, allowing concurrent modifications during iteration.

The Behavioral Changes

Now for the changes that alter the API contract, which I understand are the ones you need to think about carefully:

The biggest one is AsyncSignal.Emit. The original implementation spawns goroutines for listeners but then calls wg.Wait(), blocking until all listeners complete. We changed it to true fire-and-forget. It schedules the goroutines and returns immediately. Our reasoning was that "async" typically implies non-blocking, and the original behavior was more like "concurrent but synchronous." However, I recognize this is a design decision, not a bug fix. If users rely on Emit completing before continuing, this change breaks their code. We can absolutely revert this if the blocking behavior was intentional.

Related to that, we removed the worker pool and sync.Pool optimizations. The original used a channel-based worker pool for large subscriber counts and pooled task objects to reduce allocations. We simplified to one goroutine per listener with a recover wrapper. This trades some potential throughput for simpler, more predictable code. Again, if the pool was a deliberate performance optimization you want to keep, we can restore it.

We also removed the special-case zero-allocation fast path for single listeners. Instead, we use a small stack-allocated array [4]keyedListener[T] that handles the common case of few subscribers without heap allocation. The micro-benchmark profile will be slightly different, but the intent is similar.

One smaller behavioral change: Emit now invokes callbacks registered via AddListenerWithErr, ignoring any errors they return. Previously, those callbacks only ran during TryEmit. We thought it was surprising that registering a listener with the error-returning variant would cause it to be silently skipped during regular Emit calls.

The Documentation

For the README, we softened the benchmark claims because specific numbers like "5.66ns" are machine-dependent and some were invalidated by the async semantics change. We pointed users to run their own benchmarks instead. If you prefer to keep the original marketing language with your measured numbers, that is your call.

Path Forward

I am happy to approach this however works best for you. We could split the PR into a correctness-fixes-only branch that you can merge with confidence, and a separate behavioral-changes branch for discussion. Or, if you prefer, I can revert specific changes so you can evaluate them independently. Just let me know which of the design decisions were intentional and we can talk through the tradeoffs.

Thanks again for maintaining this library and for engaging constructively with the PR.

Josh