Skip to content

refactor(rwlock): move guard permits into access tokens - #351

Merged
tisonkun merged 6 commits into
apache:mainfrom
jeffoodchain:refactor/rwlock-access-tokens
Oct 10, 2026
Merged

tisonkun merged 6 commits into
apache:mainfrom
jeffoodchain:refactor/rwlock-access-tokens

Conversation

@jeffoodchain

@jeffoodchain jeffoodchain commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Rework #289 on current main. RwLock's eight guard types now hold a private access token that owns their permits and releases them on drop, so projecting or downgrading a guard moves its token instead of suppressing the guard's destructor. This removes all 20 ManuallyDrop wrappers and the 10 unsafe ptr::read calls that moved the Arc out of owned guards. Unsafe blocks under rwlock/ drop from 36 to 12, each one a Deref or DerefMut body with a SAFETY line; the new access module has none. Public API signatures, auto traits, guard sizes, and FIFO ordering are unchanged.

As in #289, apply ASF source headers to the nine rewritten files while keeping the Tokio copyright, MIT attribution, and pinned upstream links, update the RwLock paragraph in LICENSE, and remove the RwLock exemptions from the header checker. Replace the Tokio-derived documentation with the module-level guide and short item docs from #289; the three module examples replace the per-method examples, and each acquire method keeps the project's # Cancel safety section.

Add tests for real events around projected guards: a rejected filter_map that hands the guard back, a cancelled reader or writer that holds a mapped guard while another request is queued, a projection that panics inside a spawned task, and a suspended task that owns a mapped guard and is then aborted. Together they use all eight guard types. Also keep #289's tests for downgrade at reader limits of 1, 3, and usize::MAX, cancellation of a granted owned request, get_mut and into_inner, and mapped guard Send bounds. All of these tests also pass against the previous implementation.

Design Notes

Tokens are the design from #289 with one change: a token stores its owner directly (a borrowed lock, a borrowed semaphore for borrowed projections, or an Arc) rather than in an Option. This keeps the pointer niche, so Option<Guard> for the four unmapped guards is the guard's own size, and Deref has no None check. Borrowed projections copy the reference and mem::forget the old token, which holds only that reference and a permit count. Owned downgrade clones the Arc and lets the old token release the remaining permits on drop. Downgrade creates the read token before it releases the other permits.

Three things from #289 are left out because the Waker Contract no longer supports recovery from panicking wakers: the CHANGELOG bug-fix entry, the sentence about wake-callback unwinding in LICENSE, and the panicking-waker regression test. The two macro-based projection tests are replaced by the scenario tests above.

Validation: cargo x test (593 passed), cargo x check, cargo x lint, and Miri on unsafe_paths_test.

Comment thread asyncband/src/rwlock/write_guard.rs Outdated
@QwQBiG

QwQBiG commented Oct 8, 2026

Copy link
Copy Markdown
Member

Thanks for reworking this. The tokens make permit ownership easier to follow.

I agree with @orthur2’s suggestion to use Deref/DerefMut for the projections and keep the safety explanations with the remaining raw accesses.

I didn’t find a correctness issue. My validation and benchmark results for that revision are below.

Validation

Environment Checks Result
Linux / WSL on Windows Stable and Rust 1.86 tests, feature checks, targeted Miri Passed
macOS / Apple M4 RwLock, trait, and doc tests on Rust 1.99 Passed
GitHub CI / Linux, macOS, Windows Stable and MSRV tests Passed

Tradeoffs

Comparing 822d0d9 with f27cd415 on M4 / Rust 1.99, with two batches of five alternating pairs:

Check Result
Four unmapped Option<Guard> types Each grew by 8 bytes; guard sizes unchanged
readers_then_queued_writer(16) Paired medians were 1.43% and 1.61% slower
eight_reads_then_one_write 2.75% and 0.93% slower; magnitude varied
Queued writer with 1 or 4 readers Inconclusive

These are small, workload-specific differences, not evidence of a general slowdown. For the simpler ownership handling.

Documentation nit

Could we shorten the repeated rewrite explanations in the guard headers and LICENSE? Keep the provenance, licensing, and modification notices, but move details such as FIFO being unchanged by this refactor to the PR description. :3

Comment thread LICENSE Outdated

@orthur2 orthur2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks.The ownership transfers and downgrade ordering look sound to me.

On the owner representation, I'd prefer storing the owner directly, and doing it in this PR. That keeps the niche for Option, so the four try_* methods return the same size as before, and it removes the owner check from Deref and DerefMut on the unmapped guards. The cost moves to owned downgrade, which pays an extra Arc clone and drop, and to the two borrowed into_semaphore conversions, which need a mem::forget on a token that holds only a reference and a count. That seems a better place for the cost than ordinary guard use, and it still keeps the token design and drops every ptr::read.

Comment thread asyncband/src/rwlock/read_guard.rs
Comment thread tests-integration/tests/rwlock_test.rs
@jeffoodchain
jeffoodchain force-pushed the refactor/rwlock-access-tokens branch from 75c73bb to eb5cd4f Compare October 9, 2026 13:15
@jeffoodchain

Copy link
Copy Markdown
Contributor Author

@orthur2, @QwQBiG, @ariesdevil sorry for spamming, but I changed the code according to your reviews and advices. I think the code is ready for now.

@ariesdevil ariesdevil left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@QwQBiG

QwQBiG commented Oct 9, 2026

Copy link
Copy Markdown
Member

LGTM, thanks! :3

@orthur2
orthur2 self-requested a review October 10, 2026 01:24

@orthur2 orthur2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, looks good to me now.

@tisonkun tisonkun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks!

@tisonkun
tisonkun merged commit 44fc2d8 into apache:main Oct 10, 2026
9 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.

5 participants