Skip to content

refactor: harden library ergonomics and test contracts - #248

Merged
tisonkun merged 32 commits into
apache:mainfrom
tisonkun:codex/polish-library-ergonomics
Aug 30, 2026
Merged

refactor: harden library ergonomics and test contracts#248
tisonkun merged 32 commits into
apache:mainfrom
tisonkun:codex/polish-library-ergonomics

Conversation

@tisonkun

Copy link
Copy Markdown
Member

Summary

  • Remove latch future types that callers could neither construct nor obtain through the public async methods.
  • Reject zero-sized bounded pools instead of allowing checkouts that can never complete.
  • Replace debug-string assertions, scheduler sleeps, and pseudo pressure benchmarks with deterministic tests of observable behavior.
  • Clarify lifecycle, cancellation, capacity, and ownership contracts across the public documentation, and correct misleading examples.

Design Notes

The audit treats Asyncband as a foundational published crate: public surface area must be usable, examples must teach sound async patterns, and tests should defend stable behavior rather than formatting or scheduler timing. Each concern is isolated in a small commit so it can be reviewed or dropped independently.

The patch removes 1,190 net lines. The only runtime behavior change is rejecting a bounded pool whose maximum size is zero; the rest narrows unusable API surface, strengthens the default-feature test workflow, or improves tests and documentation.

Validation

  • cargo x check
  • cargo x test
  • cargo x lint
  • cargo package -p asyncband --allow-dirty

Latch::wait and Latch::wait_owned are async functions, so callers receive compiler-generated futures rather than these concrete structs. Keeping types that safe callers can neither construct nor obtain adds semver surface without adding capability; make them implementation details while preserving the method APIs.
Debug output is diagnostic and intentionally non-contractual. Assertions on exact fields or rendered text freeze formatting without protecting user-visible behavior, so retain trait coverage and behavioral state tests instead.
Wall-clock delays test scheduler luck rather than latch semantics and make the suite slower. Poll wait futures directly to verify pending, wake, cancellation, and ready transitions while retaining a real concurrent-arrival case.
Sleep-based completion checks can pass or fail according to task scheduling. Drive the IntoFuture state and worker-handle drops explicitly so the tests document ownership, cancellation, and the last-handle transition.
Artificial delays were only keeping an initializer in flight long enough for another caller to join. Poll the initializer to Pending and coordinate cancellation, panic, and retry directly so the same contracts are covered deterministically.
Timer sleeps made initialization races dependent on executor scheduling. Explicitly drive the winning initializer and waiting callers through their states so retry, error, cancellation, and publication behavior remain deterministic.
These scenarios panic only after a guard has been created, so unwinding exercises the same ordinary Drop path as non-panicking scope exit. Removing the variants reduces noise while the mapping and lock-release invariants remain covered by tests that reach distinct code paths.
Yielding tasks cannot prove FIFO ordering because the executor chooses their registration order. Register a writer and a later reader by polling them directly while the lock is held, then verify that the writer becomes ready first.
Ten nearly identical tests repeated Weak counts and individual map or filter_map operations. A pair of mapping chains covers ownership retention, successful and failed projections, and final unlock behavior with substantially less scaffolding.
The pseudo memory-ordering and oversized stress cases duplicated basic locking behavior without defining deterministic contracts. Keep focused coverage for reader limits, concurrent readers and writers, writer priority, every downgrade form, mapped ownership, and zero-sized values.
@tisonkun
tisonkun force-pushed the codex/polish-library-ergonomics branch from 5602a20 to de0cb58 Compare August 30, 2026 14:32
A maximum size of zero creates a semaphore with no capacity, so every checkout can remain pending forever. Asyncband has no resize or closed-pool control that could make that state useful; reject it at Pool::new so configuration mistakes fail immediately and document the boundary.
WaitGroup completion is driven by handle drops rather than task completion itself, and awaiting consumes the coordinator handle. The previous prose obscured that ownership model, making it easy to await the wrong clone and wait forever; describe the lifecycle directly.
The crate promises an empty default feature set, but the crate-level mutex example failed to compile in that configuration. All-feature tests masked the problem, so gate the example correctly and make the repository test command exercise no-default-feature documentation explicitly.
The documentation still described threads and boolean return values even though this is an async task primitive returning BarrierWaitResult. Explain reusable generations and the single leader as observable API concepts, and replace print-only examples with assertions.
Launching one hundred tasks with staggered sleeps did not establish a meaningful order or concurrency invariant; it only rechecked that dropped guards return permits. Deterministic permit lifecycle tests already cover that contract, so remove the slow pseudo stress case.
Sleeping before send tried to guess that a receiver had parked and duplicated the adjacent direct-waker test. Remove that race, and use a channel timeout solely as a deadlock watchdog instead of repeatedly polling an atomic flag with sleeps.
The Option unwraps rely on ordinary moved-value state invariants and do not justify unsafe operations. Calling those comments SAFETY obscures actual unsafe review obligations, so label them as invariants without changing behavior.
The previous documentation repeated generic coordination prose, used print-only examples, and omitted important behavior. State that the countdown saturates at zero, cannot be reused, and has cancel-safe waits, then demonstrate those contracts with assertions.
Endpoint Options are cleared only by their destructors, and safe borrows cannot overlap those destructors. These unwraps therefore rely on a normal ownership invariant rather than an unsafe proof; naming that distinction keeps safety review focused.
Million-message loops with Instant and println are benchmarks without thresholds, while testing every capacity from 1 through 100 repeats the same control flow. Replace them with representative capacity boundaries and a focused multi-producer delivery case.
Callers need to know that bounded channels apply backpressure, unbounded channels can grow with a slow receiver, and a zero bounded capacity is rejected. Correct the send and recv wording as well so asynchronous waiting is not described as thread sleeping.
The file example held an async permit while calling std::fs, teaching users to block an executor thread and distracting from the semaphore contract. Replace it with a direct permit lifecycle example that is runtime-agnostic and safe to copy.
The OwnedSemaphorePermit documentation was copied from the borrowed permit and still constructed SemaphorePermit values, so its doctests compiled without exercising the documented API. Use Arc-based owned acquisition and name the correct permit type.
Printed output is not a checked contract, and the word nominally left the initialization guarantee ambiguous. Assert that competing initializers publish one shared value and consistently describe asynchronous callers as tasks.
The example called a Tokio task a thread and hand-wrote the predicate loop despite exposing wait_while for that purpose. Show the durable predicate pattern directly and join the notifier so the example does not leave detached work.
Public documentation exposed the internal latch and wait-group composition and printed from unjoined tasks. Focus on request and guard ownership semantics, join the example tasks, and clarify that an owned watch future does not itself keep completion pending while its source guard still does.
@tisonkun
tisonkun force-pushed the codex/polish-library-ergonomics branch from de0cb58 to b051463 Compare August 30, 2026 14:49
@tisonkun
tisonkun marked this pull request as ready for review August 30, 2026 15:01
@tisonkun
tisonkun requested a lite review from Copilot August 30, 2026 15:01
The previous documentation used an ambiguous 'do so' reference, making it unclear whether the returned future or the source guard delays completion. State directly that the future only observes the request while the guard continues to participate in completion.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This pull request refactors Asyncband’s public-facing ergonomics and contracts by trimming unusable public types, tightening invalid configuration behavior, and replacing timing-/format-dependent tests and examples with deterministic assertions that better defend observable behavior.

Changes:

  • Reject zero-capacity bounded pools at construction time and add coverage for the panic contract.
  • Replace stress/timing/debug-string assertions in integration tests with deterministic, contract-focused tests using direct polling where needed.
  • Clarify and harden public documentation/examples across multiple primitives (feature-gated doctests, lifecycle/ownership semantics, cancellation expectations).

Reviewed changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
xtask/src/main.rs Adds a dedicated doc-test command to the cargo x test workflow (with a naming mismatch noted).
tests-integration/tests/waitgroup_test.rs Replaces runtime/sleep-based tests with deterministic polling and explicit task joins.
tests-integration/tests/semaphore_test.rs Removes a timing-based stress test.
tests-integration/tests/rwlock_test.rs Replaces broad stress/panic-safety/debug-string tests with deterministic concurrency and ownership-contract assertions.
tests-integration/tests/pool_behavior_test.rs Adds coverage that bounded pools reject max_size = 0.
tests-integration/tests/once_test.rs Reworks Once tests to validate cancellation/panic retry and concurrent initialization deterministically.
tests-integration/tests/once_map_test.rs Removes debug-string assertion in constructors test.
tests-integration/tests/once_cell_test.rs Replaces sleep-based initialization tests with deterministic cancellation/retry and single-publication assertions.
tests-integration/tests/mpsc_test.rs Replaces pressure benchmarks with deterministic ordering/capacity/state tests.
tests-integration/tests/latch_test.rs Replaces timing assertions with deterministic waker-tracking and concurrency tests.
tests-integration/tests/broadcast_mpmc_unbounded_test.rs Replaces spin/sleep completion detection with a bounded blocking wait via recv_timeout.
CHANGELOG.md Documents removal of unconstructible latch wait types from the public API.
asyncband/src/waitgroup/mod.rs Updates docs to clarify coordinator/worker handle semantics and wait-future cloning behavior.
asyncband/src/shutdown/mod.rs Clarifies shutdown/guard lifecycle and improves example correctness (task joining/observable results).
asyncband/src/semaphore/mod.rs Tightens module docs and corrects owned-permit examples and wording.
asyncband/src/pool/unbounded.rs Updates internal safety comments from “SAFETY” to explicit invariants.
asyncband/src/pool/bounded.rs Enforces max_size > 0 at runtime and updates related docs/invariants.
asyncband/src/once/once_cell/mod.rs Improves docs to state retry semantics and clarifies example behavior deterministically.
asyncband/src/mpsc/unbounded.rs Clarifies doc wording (“waits” vs “sleeps”) and updates safety commentary to invariants.
asyncband/src/mpsc/mod.rs Expands module-level docs with bounded vs unbounded tradeoffs.
asyncband/src/mpsc/bounded.rs Documents panic contract for zero buffer and updates wording/invariants.
asyncband/src/lib.rs Feature-gates the mutex doctest so docs build under minimal/no-feature configurations.
asyncband/src/latch/mod.rs Clarifies latch semantics/contracts, improves examples, and makes internal wait-future types non-public.
asyncband/src/condvar/mod.rs Improves examples to await spawned tasks and uses wait_while patterns more directly.
asyncband/src/barrier/mod.rs Rewrites docs/examples to emphasize barrier generations and leader selection more clearly.
Suppressed comments (1)

xtask/src/main.rs:398

  • This command is built with --no-default-features, but the function name suggests it's testing the default feature set. Renaming the helper to make_no_default_features_doc_test_cmd would better match the actual cargo invocation.
fn make_default_feature_doc_test_cmd() -> StdCommand {

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread xtask/src/main.rs Outdated
This method differs from shutdown_requested only by returning a future that does not borrow the guard. Describe that distinction directly instead of repeating the broader ShutdownGuard lifecycle semantics.
Callers care that shutdown_requested_owned can be moved into a spawned task, not how it relates to the receiver borrow. Match Latch::wait_owned by documenting that practical distinction and cancel safety.
The practical distinction is that the returned future can be spawned. State that directly and omit lifecycle and cancellation details that obscure the method purpose.
The extra command reran doctests only for asyncband with every feature disabled, adding a second test invocation for a configuration with no distinct documented contract. Keep cargo x test focused on the workspace feature run and remove the now-unused command builder.
Comment thread asyncband/src/shutdown/mod.rs Outdated
@tisonkun
tisonkun enabled auto-merge (squash) August 30, 2026 15:15
@tisonkun
tisonkun merged commit 55ce955 into apache:main Aug 30, 2026
10 checks passed
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