Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a wait_blocking() conditional-compilation issue on wasm targets and improves feature-matrix testability and correctness checks across WaitGroup variants.
Changes:
- Gate
future::WaitGroup::wait_blocking()behindstd && !wasmto avoid wasm build failures. - Refactor
tests/future.rsso alloc-only configurations compile/run (std-dependent tests are now#[cfg(feature = "std")]). - Make
add()overflow detection panic in release builds forsync,spin, andfuturevariants; expand CI to cover multi-feature combinations and--all-featurescross checks.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
tests/future.rs |
Split std-only vs alloc-only tests; add wasm gating around blocking tests; adjust manual-poll test to use core/alloc. |
src/sync.rs |
Change add() to use checked_add(...).expect(...) so overflow is caught in all builds. |
src/spin.rs |
Change add() to use fetch_update + checked_add to prevent atomic counter wrap in release builds. |
src/future.rs |
Same overflow hardening for add(); fix wait_blocking() cfg to exclude wasm targets. |
CHANGELOG.md |
Add 1.0.1 entry documenting fixes and CI changes. |
.github/workflows/ci.yml |
Add explicit builds/tests for alloc,future and --all-features; add cargo check --all-features per cross target. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async fn wake_after_updating_in<S: RuntimeLite>() { | ||
| let wg = WaitGroup::new(); | ||
| for _ in 0..100000 { | ||
| let worker = wg.add(1); | ||
| S::spawn_detach(async move { | ||
| S::sleep(Duration::from_millis(10)).await; | ||
| let mut a = 0; | ||
| for _ in 0..1000 { | ||
| a += 1; | ||
| } | ||
| debug_assert_eq!(a, 1000); | ||
| S::sleep(Duration::from_millis(10)).await; | ||
| worker.done(); | ||
| }); | ||
| } | ||
| wg.wait().await; | ||
| } |
There was a problem hiding this comment.
wake_after_updating_in spawns 100,000 tasks and includes sleeps; this is very likely to be slow and/or flaky in CI (high memory pressure and scheduler overhead), and can dominate test runtime. Consider reducing the iteration count, making it a stress test behind an opt-in flag, or marking it #[ignore] and running it in a dedicated job.
| ## [1.0.1] | ||
|
|
There was a problem hiding this comment.
The changelog introduces a ## [1.0.1] section, but the crate version in Cargo.toml is still 1.0.0. To avoid confusing consumers, consider either bumping the crate version as part of this PR or renaming this section to an "Unreleased" heading until the release is actually cut.
| pub fn add(&self, num: usize) -> Self { | ||
| let mut ctr = self.inner.count.lock_me(); | ||
| // In debug builds, give a clear message on overflow. `+=` already | ||
| // panics in debug builds on overflow, but with a generic message. | ||
| debug_assert!( | ||
| ctr.checked_add(num).is_some(), | ||
| "WaitGroup counter overflow: {ctr} + {num}", | ||
| ctr = *ctr, | ||
| ); | ||
| *ctr += num; | ||
| // `checked_add` in all builds — not just debug. A wrap from | ||
| // usize::MAX + 1 → 0 would reset the counter and let `wait()` | ||
| // return prematurely. `+=` only panics on overflow in debug mode. | ||
| *ctr = ctr.checked_add(num).expect("WaitGroup counter overflow"); |
There was a problem hiding this comment.
add() now panics on counter overflow in all builds, which is a behavioral change worth pinning with a regression test. Consider adding a test that constructs a WaitGroup at/near usize::MAX (e.g., via From<usize>) and asserts that add(1) panics, so this doesn’t silently regress back to wrapping behavior.
| pub fn add(&self, num: usize) -> Self { | ||
| let prev = self.inner.counter.fetch_add(num, Ordering::Release); | ||
| // In debug builds, catch the (essentially unreachable) case where the | ||
| // counter wraps past `usize::MAX`. Silent wrap in release. | ||
| debug_assert!( | ||
| prev.checked_add(num).is_some(), | ||
| "WaitGroup counter overflow: {prev} + {num}" | ||
| ); | ||
| // Use `fetch_update` + `checked_add` so overflow is caught in ALL | ||
| // builds, not just debug. A plain `fetch_add` would silently wrap | ||
| // in release mode, which could reset the counter to zero and let | ||
| // `wait()` return prematurely or hang. | ||
| self | ||
| .inner | ||
| .counter | ||
| .fetch_update(Ordering::Release, Ordering::Relaxed, |prev| { | ||
| prev.checked_add(num) | ||
| }) | ||
| .expect("WaitGroup counter overflow"); |
There was a problem hiding this comment.
add() now guarantees overflow is caught in release builds via fetch_update + checked_add, which is a behavioral change worth covering with a regression test. Consider adding a test that sets the counter near usize::MAX (e.g., WaitGroup::from(usize::MAX)) and asserts add(1) panics, to ensure the counter never silently wraps again.
| pub fn add(&self, num: usize) -> Self { | ||
| let prev = self.inner.counter.fetch_add(num, Ordering::Release); | ||
| // In debug builds, catch the (essentially unreachable) case where the | ||
| // counter wraps past `usize::MAX`. Silent wrap in release. | ||
| debug_assert!( | ||
| prev.checked_add(num).is_some(), | ||
| "WaitGroup counter overflow: {prev} + {num}" | ||
| ); | ||
| // Use `fetch_update` + `checked_add` so overflow is caught in ALL | ||
| // builds, not just debug. A plain `fetch_add` would silently wrap | ||
| // in release mode, which could reset the counter to zero and let | ||
| // `wait()` return prematurely. | ||
| self | ||
| .inner | ||
| .counter | ||
| .fetch_update(Ordering::Release, Ordering::Relaxed, |prev| { | ||
| prev.checked_add(num) | ||
| }) | ||
| .expect("WaitGroup counter overflow"); |
There was a problem hiding this comment.
add() now guarantees overflow is caught in release builds via fetch_update + checked_add, which is a behavioral change worth covering with a regression test. Consider adding a test that sets the counter near usize::MAX (e.g., WaitGroup::from(usize::MAX)) and asserts add(1) panics, to ensure the counter never silently wraps again.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7 +/- ##
==========================================
+ Coverage 91.72% 91.85% +0.12%
==========================================
Files 3 3
Lines 133 135 +2
==========================================
+ Hits 122 124 +2
Misses 11 11
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.