docs(thread_aware): add a thread-aware authoring guide - #742
docs(thread_aware): add a thread-aware authoring guide#742Pato Sandaña (psandana) wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
_documentation is currently exported unconditionally, which unintentionally commits it as part of the crate’s normal public API instead of keeping it doc/test-only like other crates’ _documentation modules.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a task-oriented authoring guide for thread_aware by introducing a dedicated _documentation module intended to render on docs.rs and complement the crate’s existing reference-style docs.
Changes:
- Exposes a new
_documentationmodule fromthread_awareto host longer-form guidance. - Adds a comprehensive authoring guide covering implementation choices, common pitfalls, testing patterns, and debugging/telemetry.
File summaries
| File | Description |
|---|---|
| crates/thread_aware/src/lib.rs | Exposes the new _documentation module from the crate root. |
| crates/thread_aware/src/_documentation/mod.rs | New documentation module containing the authoring guide and doctest examples. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project check has failed because the head coverage (98.6%) is below the target coverage (100.0%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #742 +/- ##
=========================================
- Coverage 100.0% 98.6% -1.4%
=========================================
Files 583 626 +43
Lines 62930 86406 +23476
=========================================
+ Hits 62930 85279 +22349
- Misses 0 1127 +1127
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| /// A guide to authoring thread-aware types: how to implement, test, and debug them, and the | ||
| /// anti-patterns to avoid. See [the guide](_documentation). | ||
| pub mod _documentation; |
There was a problem hiding this comment.
- this should go into thread_aware_core
- rename to just
documentation(also could you apply the same rename across all our crates) - protect with
#[cfg(any(doc, test))]so this module doesn't become part of our public API
There was a problem hiding this comment.
Thanks! I've gated it with #[cfg(any(doc, test))] (705d1a9) so the module is doc/test-only and out of the normal public API, matching the existing _documentation modules in recoverable and fetch.
On the other two points:
- Move to
thread_aware_core: this guide is written around thethread_awarefacade —#[derive(ThreadAware)],Unaware, the strategy-partitionedArc,ThreadBuilder— none of which live inthread_aware_core. Placing it there would needthread_awareas a dev-dependency ofcore(which risks the cyclic-deps gate) and would break the intra-doc links to those facade types. Keeping it where those APIs live seems most useful to readers. If you'd instead like a separate, trait-contract-focused guide incore, I'm happy to split it — just let me know. - Rename
_documentation→documentationacross all crates: glad to, but since every crate currently uses the_documentationname it's a repo-wide convention change; I'd rather land it as its own sweep so it isn't tangled with this guide. I'll open a follow-up unless you'd prefer it here.
There was a problem hiding this comment.
Gated with #[cfg(any(doc, test))] (705d1a9). Opened AB#7857350 for the repo-wide _documentation -> documentation rename so it can land as its own sweep. On the core move: this guide leans on the thread_aware facade - the derive, Unaware, strategy Arc, Relocator - none of which live in thread_aware_core, so moving it there breaks those intra-doc links (and would need thread_aware as a dev-dependency of core, risking the cyclic-deps gate). Happy to instead split out a separate, trait-contract-focused guide for core if you'd prefer.
There was a problem hiding this comment.
🔵 Needs a closer look
The new _documentation module export and its links should be aligned with the workspace’s doc/test-only _documentation pattern and avoid broken intra-doc links under default feature docs.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
crates/thread_aware/src/_documentation/mod.rs:230
- This doc comment links to
Relocatorvia an intra-doc link (crate::Relocator), butRelocatoris behind thetest-utilsfeature in this crate. When building docs with default features (withouttest-utils), this becomes a broken intra-doc link.
Consider referring to it as code (or conditionally documenting it) so the guide doesn’t produce broken links in default doc builds.
crates/thread_aware/src/lib.rs:181
_documentationmodules in this workspace are typically doc/test-only so they render on docs.rs and in doctests without becoming part of the crate’s normal public API. Other crates gate the public_documentationmodule with#[cfg(any(doc, test))](e.g., crates/recoverable/src/lib.rs:81-82, crates/fetch/src/lib.rs:860-862), butthread_awarecurrently exports it unconditionally here. Also, the(_documentation)markdown link is a relative URL and will resolve incorrectly when this doc comment is rendered on the module’s own page.
Consider gating the module and dropping the relative self-link (or converting it to an intra-doc link) to match the established pattern.
/// A guide to authoring thread-aware types: how to implement, test, and debug them, and the
/// anti-patterns to avoid. See [the guide](_documentation).
pub mod _documentation;
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
Addresses review feedback on PR #742: export the `_documentation` module only for rustdoc and tests, matching the established pattern in `recoverable` and `fetch`, so it does not become part of the crate's public API in normal builds. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The new guide contains a couple of documentation correctness issues (notably conflicting bounds semantics and missing std feature context for Arc) that should be reconciled before publishing.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
crates/thread_aware/src/_documentation/mod.rs:90
- This section claims the derive bounds the relocated field type (e.g. emitting
where Vec<T>: ThreadAware), but the derive macro's own docs incrates/thread_aware/src/lib.rscurrently describe per-parameterT: ThreadAwarebounds (see "# Generic Bounds" around lib.rs:211-217). To avoid contradicting the crate’s existing reference docs, consider rephrasing this section to describe bounds in terms of the derive’s traversal and link to the authoritative rules.
crates/thread_aware/src/_documentation/mod.rs:122 Arc/PerThread/PerProcessare only exported when thestdfeature is enabled (seecrates/thread_aware/src/lib.rs:263-266), but the decision table currently presents them without that constraint. Adding an explicit "(stdfeature)" note would make the guidance accurate forno_stdusers reading this guide.
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
| //! | ||
| //! ## Prefer the derive | ||
| //! | ||
| //! In almost all cases, implement [`ThreadAware`](crate::ThreadAware) with the derive macro. It |
There was a problem hiding this comment.
could we link to actual macro (in external crate)
There was a problem hiding this comment.
Done in 53ca92f - now links to macro@crate::ThreadAware.
| //! call. The lessons in [Anti-patterns](#anti-patterns) are drawn from migrating a large | ||
| //! production service onto an Oxidizer-backed runtime. | ||
| //! | ||
| //! # Why thread-awareness exists |
There was a problem hiding this comment.
this should just link to main lib.rs docs, too much duplication
There was a problem hiding this comment.
Trimmed to a pointer at the crate-level Theory of Operation (53ca92f); dropped the duplication.
| //! "this field does not implement `ThreadAware` yet" - reach for [`Unaware`](crate::Unaware) or | ||
| //! [`Arc`](crate::Arc) for that, so the intent is visible in the type. | ||
| //! | ||
| //! ## What the generated bounds mean |
There was a problem hiding this comment.
this might be too much detail, I myself had trouble grasping what is this traying to say
There was a problem hiding this comment.
Agreed - cut it down to a short pointer at the derive's Generic Bounds reference (53ca92f).
| //! (`PhantomData<fn(*const T)>`), owe no bound at all. See | ||
| //! [the derive's reference](crate::ThreadAware#generic-bounds) for the full rules. | ||
| //! | ||
| //! ## Implementing the trait by hand |
There was a problem hiding this comment.
I am missing thread_aware::Arc guide here, when to implement this. (when we want to maintain separated PerThread instances)
There was a problem hiding this comment.
Added a "Per-worker state with Arc" section (53ca92f): when to reach for Arc<T, PerThread> to keep separate per-worker instances, vs PerProcess / PerNumaNode.
| //! Relocating a subtree while its parent was built from a stale clone (see above) is how affinity | ||
| //! goes stale in practice. | ||
| //! | ||
| //! # Testing |
There was a problem hiding this comment.
check thread_aware::Relocator (under "test-util") that could be used for relocation testing. Also too much detail too, make it more concise.
There was a problem hiding this comment.
Made Testing more concise and now lead with the test-utils Relocator helper for driving relocations (53ca92f).
| //! `test-utils` feature additionally offers a [`Relocator`](crate::Relocator) helper for driving | ||
| //! relocations in tests. | ||
| //! | ||
| //! # Debugging and telemetry |
There was a problem hiding this comment.
we don't have any answer for this right now, I would just omit this section
There was a problem hiding this comment.
Removed the Debugging and telemetry section (53ca92f).
Per @martintmk's review of PR #742: - Trim "Why thread-awareness exists" to a pointer at the crate-level Theory of Operation instead of duplicating it. - Link "the derive macro" to the actual macro (`macro@crate::ThreadAware`). - Simplify "What the generated bounds mean" - defer the detail to the derive's Generic Bounds reference rather than restating it. - Add a "Per-worker state with `Arc`" section explaining when to reach for `Arc<T, PerThread>` (separate per-worker instances) vs `PerProcess`/`PerNumaNode`. - Make "Testing" more concise and lead with the `test-utils` `Relocator` helper. - Drop "Debugging and telemetry" - the telemetry story is not defined yet. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new docs introduce several feature-gated / derive-macro intra-doc links that will be broken or misleading in non-all-features doc builds and should be adjusted before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
crates/thread_aware/src/lib.rs:180
- The module doc comment uses a Markdown URL link (
[the guide](_documentation)), which is easy to interpret as a relative URL rather than a rustdoc intra-doc link. Since this comment is on the_documentationmodule itself, the extra link is also redundant; removing it avoids brittle/ambiguous linking.
crates/thread_aware/src/_documentation/mod.rs:79
- This section header link targets the derive macro docs; it should use the
derive@disambiguator (notmacro@) to match how derive macros are linked elsewhere in the repo and to avoid ambiguity with non-derive macros.
//! [Generic Bounds](macro@crate::ThreadAware#generic-bounds) reference has the rules.
crates/thread_aware/src/_documentation/mod.rs:121
- These table rows link to
crate::Arc, which isstd-feature gated. In non-stddoc builds this becomes a broken intra-doc link and the table reads as ifArcwere always available. Consider marking these rows asstd-only and using inline code instead of intra-doc links.
//! | Shared state that should differ per worker | [`Arc<T, PerThread>`](crate::Arc) | Materializes a separate `T` per destination. |
//! | Shared state that is the same everywhere | [`Arc<T, PerProcess>`](crate::Arc) | Behaves as a vanilla `Arc`. |
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
…e configs Addresses Copilot review comments on PR #742: - Use the `derive@` disambiguator for the derive-macro links (matches the repo convention, e.g. `internity`), replacing `macro@`. - The `Arc` strategy section pointed at `crate::Arc` / `crate::PerThread` / `crate::PerNumaNode`, which are `std`-gated, so the links broke under `--no-default-features`. Name the `std` feature and drop the feature-gated intra-doc links in favour of plain code spans. - `Relocator` is `test-utils`-gated; its intra-doc link broke in doc builds without that feature. Reword to a plain code span. Doctests still pass under default and all-feature builds; the guide no longer contributes any broken-intra-doc-link warnings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The new guide contains feature-gated intra-doc usage (crate::Arc, #[derive(ThreadAware)]) that should be shimmed/worded to remain correct across the crate’s supported feature sets.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
crates/thread_aware/src/_documentation/mod.rs:46
- This example uses
#[derive(ThreadAware)], but the derive macro is feature-gated (pub use ::thread_aware_macros::ThreadAwareis behindfeature = "derive"). Wrapping the doctest body in a hidden#[cfg(feature = "derive")]shim keeps the guide’s examples copy/paste-able across feature sets (and matches the crate-level docs’ pattern).
This issue also appears in the following locations of the same file:
- line 58
- line 197
crates/thread_aware/src/_documentation/mod.rs:72
Arcis only exported when thestdfeature is enabled (seelib.rs’s#[cfg(any(test, feature = "std"))] pub use ... Arc). Linking to it here makes this paragraph only correct instdbuilds; prefer plain code and explicitly mention thestdfeature, as you do later in the guide.
crates/thread_aware/src/_documentation/mod.rs:62
- This doctest also depends on the
derivefeature for#[derive(ThreadAware)]. Consider gating the doctest body with a hidden#[cfg(feature = "derive")]block so the documentation remains valid whenderiveis disabled.
//! ```rust
//! use thread_aware::ThreadAware;
//!
//! #[derive(ThreadAware)]
//! struct Request {
crates/thread_aware/src/_documentation/mod.rs:201
- This doctest uses
#[derive(ThreadAware)]forUnderTest, which is unavailable when thederivefeature is off. Gating just the derived portion with a hidden#[cfg(feature = "derive")]shim keeps the rest of the example (theTrackerobservation pattern) visible while making the doctest resilient to different feature sets.
//! #[derive(ThreadAware)]
//! struct UnderTest {
//! tracked: Tracker,
//! #[thread_aware(skip)]
//! skipped: Tracker,
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Thanks — addressed in c90d9f4:
|
There was a problem hiding this comment.
🟢 Approval recommended
The changes are isolated to documentation (plus a docs/test-only module export) and align with existing repo patterns without introducing behavioral or API changes in normal builds.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Adds a `_documentation` module with a task-oriented guide for authors of thread-aware types, complementing the existing (reference-style) API docs. Covers what thread-awareness is and why it exists, how to author a type (derive, `#[thread_aware(skip)]`, hand-written impls, `Unaware`, strategy `Arc`), how to choose among them, how to test that relocation reaches the right fields, how to debug and read relocation telemetry, and how to validate correctness. The anti-patterns section folds in the migration experience of moving a large production service onto an Oxidizer runtime - `Clone` copying stored affinity rather than relocating, `#[thread_aware(skip)]` on a sole field silently no-op'ing, not trusting inherited markings, and relocating the whole dependency graph once at a boundary. Follows the `recoverable::_documentation` pattern. All examples are doctested under both default and all-feature configurations. Refs AB#7552151, AB#7722787. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
`format_code_in_doc_comments` measures doc-comment code at the reduced width left by the `//! ` prefix, so the two `assert_eq!` calls in the testing example must wrap. Matches `cargo +nightly fmt --config-path ./unstable-rustfmt.toml`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses review feedback on PR #742: export the `_documentation` module only for rustdoc and tests, matching the established pattern in `recoverable` and `fetch`, so it does not become part of the crate's public API in normal builds. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Per @martintmk's review of PR #742: - Trim "Why thread-awareness exists" to a pointer at the crate-level Theory of Operation instead of duplicating it. - Link "the derive macro" to the actual macro (`macro@crate::ThreadAware`). - Simplify "What the generated bounds mean" - defer the detail to the derive's Generic Bounds reference rather than restating it. - Add a "Per-worker state with `Arc`" section explaining when to reach for `Arc<T, PerThread>` (separate per-worker instances) vs `PerProcess`/`PerNumaNode`. - Make "Testing" more concise and lead with the `test-utils` `Relocator` helper. - Drop "Debugging and telemetry" - the telemetry story is not defined yet. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…e configs Addresses Copilot review comments on PR #742: - Use the `derive@` disambiguator for the derive-macro links (matches the repo convention, e.g. `internity`), replacing `macro@`. - The `Arc` strategy section pointed at `crate::Arc` / `crate::PerThread` / `crate::PerNumaNode`, which are `std`-gated, so the links broke under `--no-default-features`. Name the `std` feature and drop the feature-gated intra-doc links in favour of plain code spans. - `Relocator` is `test-utils`-gated; its intra-doc link broke in doc builds without that feature. Reword to a plain code span. Doctests still pass under default and all-feature builds; the guide no longer contributes any broken-intra-doc-link warnings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… guide Follow-up to 2ce36aa: the "Skipping a field" section still linked `[Arc](crate::Arc)`, which is `std`-gated and breaks under `--no-default-features`. Replace it with a plain code span that names the `std` feature, matching the treatment applied to the other `Arc` references. The guide now contributes no broken-intra-doc-link warnings in the `--no-default-features` doc build; doctests still pass under default and all-feature builds. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
c90d9f4 to
9681471
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The guide contains derive@... intra-doc links that can be feature-gated away (when derive is off), leaving docs builds without default features with unresolved link targets.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
crates/thread_aware/src/_documentation/mod.rs:80
- Same root issue as above: the
derive@crate::ThreadAware#generic-boundslink is feature-gated (the macro isn’t exported withoutderive), so it can become a broken intra-doc link in docs built without default features. Consider rewording to refer to the section without linking, and call out that it’s available whenderiveis enabled.
//! You rarely need to reason about this: the derive adds exactly the `ThreadAware` bounds its
//! generated body needs and no more, so a correct type "just derives". When it matters - a generic
//! wrapper, or a marker field that should stay bound-free - the derive's
//! [Generic Bounds](derive@crate::ThreadAware#generic-bounds) reference has the rules.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
| //! In almost all cases, implement [`ThreadAware`](crate::ThreadAware) with | ||
| //! [the derive macro](derive@crate::ThreadAware). It generates a | ||
| //! [`relocate`](crate::ThreadAware::relocate) that forwards the notification to every field, which |
What & why
Adds a
_documentationmodule tothread_awarewith a task-oriented authoring guide, the piece the crate''s (otherwise reference-style) docs are missing. Follows therecoverable::_documentationpattern and renders on docs.rs.Addresses 7552151 (publish the thread-aware authoring guide) and folds in 7722787 (the 3S / oxidizer-spawner migration experience).
Contents
#[thread_aware(skip)]; what the field-type bounds mean; hand-written impls.Unaware] vs. strategy [Arc], as a decision table.Clonecopies stored affinity instead of relocating;#[thread_aware(skip)]on a sole field is a silent no-op; don''t trust inherited markings; relocate the whole graph once at the boundary.Trackerobservation pattern (assert relocation reaches non-skipped fields and not skipped ones).*.thread_mismatch-style warnings, and why their absence doesn''t prove correctness.Notes
Refs AB#7552151, AB#7722787.