Introduce better ZoneState locks#606
Conversation
bal-e
force-pushed
the
better-zone-state-locks
branch
from
c24d902 to
e3fb33a
Compare
| .map_err(|e| SignerError::SigningError(format!("zone.state.lock() failed: {e}")))?; | ||
| policy = zone_state.policy.clone().expect("should be there"); | ||
| }; | ||
| let policy = { zone.read().policy.clone().unwrap() }; |
There was a problem hiding this comment.
The {} is important here right? Maybe we should have a comment saying that it shouldn't be removed?
There was a problem hiding this comment.
Actually no; the lock would be dropped at the end of the statement anyway. But I can at least document that.
There was a problem hiding this comment.
Yeah I guess that would be nice. Do you just put them there for extra clarity?
There was a problem hiding this comment.
Yeah, I think it's nice to explicitly see that. And rustfmt doesn't try to remove it.
| /// consistent with each other, and that changes to the zone happen in a | ||
| /// single (sequentially consistent) order. | ||
| pub state: Mutex<ZoneState>, | ||
| /// The state is locked for consistency; see [`ZoneStateLock`]. |
There was a problem hiding this comment.
Could you link to ZoneState here? That would be nice for goto definition from rust-analyzer.
| /// | ||
| /// This is a convenience type. By owning the write lock, it can simplify | ||
| /// construction of `ZoneHandle`s for quick uses. | ||
| pub struct OwnedZoneHandle<'a> { |
There was a problem hiding this comment.
This name is very confusing to me because it doesn't actually own the zone but only a write lock to it. Maybe WriteZoneHandle?
2 participants
This PR introduces
ZoneStateLock, a wrapper around theZoneStatelock with higher-level methods.ZoneStateLockalleviates boilerplate and inconsistencies around how we use theZoneStatelock.Previously, access to zone state was obtained via
zone.state.lock().unwrap(). Some call sites used.expect()or?instead of.unwrap(), introducing inconsistencies. Most importantly: most call sites did not call.mark_dirty(), so changes to the zone were not being persisted with regularity.ZoneStateLockwraps anRwLockinstead of aMutexbecause some call sites only read the zone state, and it seemed better to expose that functionality rather than use write locks everywhere. Rather than a simple.lock()method, a write lock must be obtained with the obtuse name.write_cleanly(), and that method tells users to preferZone::write().Zone::write()marks the state as dirty automatically so that callers don't have to remember to.In addition, a new
OwnedZoneHandletype is introduced. This works around some borrowing limitations ofZoneHandle. The newZone::write_handle()method obtains a write lock over the zone state, marks it as dirty, and returns anOwnedZoneHandleso that handle-related methods can be called conveniently.All call sites of
zone.state.lock()have been updated to use the new locking methods, and most of them (particularly those that neededZoneHandle) are much simpler now.Note: this PR is very prone to (causing and incurring) merge conflicts.
Resolves #599.
Cargo.*,crates/,etc/,integration-tests/,src/):actthrough theact-wrapper(as described inTESTING.md)?