phase 10: lock-handle abstraction (Arc<Mutex/RwLock<_>> → trait handles)#86
phase 10: lock-handle abstraction (Arc<Mutex/RwLock<_>> → trait handles)#86JustinKovacich wants to merge 1 commit into
Conversation
Introduces three new traits in transport.rs and subscription_manager.rs: - E2ERegistryHandle — wraps Arc<Mutex<E2ERegistry>> on std, allows alternative implementations for bare-metal targets - InterfaceHandle — wraps Arc<RwLock<Ipv4Addr>> on client - SubscriptionHandle — wraps Arc<RwLock<SubscriptionManager>> on server Client<M, R, I> and Server<R, S> / EventPublisher<R, S> are now generic over these handles with the existing Arc-backed types as defaults, so all existing call sites compile unchanged. Std implementations live in tokio_transport.rs. Gate: all production lock sites route through handle traits; cargo test --all-features passes (454 unit + 11 integration tests). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Refactors the client/server runtime state sharing to use trait-based “handle” abstractions instead of hard-coded Arc<Mutex/RwLock<_>>, enabling alternative lock/critical-section backends (e.g., for bare-metal targets) while keeping existing std/tokio call sites working via default type parameters.
Changes:
- Added
E2ERegistryHandleandInterfaceHandletraits totransport, plus std/tokio implementations intokio_transport. - Added
SubscriptionHandletrait and updated server-side subscription access to route through it. - Made
Client,Server, andEventPublishergeneric over these handles (with Arc-backed defaults) and updated internal code paths to call through the traits.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/transport.rs | Introduces E2ERegistryHandle/InterfaceHandle trait APIs and compile-only test implementations. |
| src/tokio_transport.rs | Provides std/tokio Arc<Mutex<_>> / Arc<RwLock<_>> implementations of the new handle traits. |
| src/server/subscription_manager.rs | Adds SubscriptionHandle trait and an Arc<tokio::RwLock<_>> implementation. |
| src/server/mod.rs | Makes Server generic over handle traits and routes subscription/E2E operations through handles. |
| src/server/event_publisher.rs | Makes EventPublisher generic over handle traits and routes subscription/E2E operations through handles. |
| src/lib.rs | Re-exports the new handle traits (and SubscriptionHandle under server). |
| src/client/socket_manager.rs | Updates socket loop / bind helpers to accept E2ERegistryHandle instead of Arc<Mutex<_>>. |
| src/client/mod.rs | Makes Client generic over E2ERegistryHandle/InterfaceHandle with std defaults; updates interface/E2E access. |
| src/client/inner.rs | Makes Inner generic over E2ERegistryHandle and clones/passes the handle into socket managers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -860,22 +874,12 @@ where | |||
| /// | |||
| /// Panics if the E2E registry mutex is poisoned. | |||
There was a problem hiding this comment.
The # Panics section says this panics if the E2E registry mutex is poisoned, but Client is now generic over R: E2ERegistryHandle and no longer directly uses a mutex here. This doc is now inaccurate for non-mutex handle implementations.
Suggested fix: either remove the # Panics section, or reword it to something like “May panic if the underlying handle implementation panics (e.g. a poisoned mutex in the std implementation).”
| /// Panics if the E2E registry mutex is poisoned. | |
| /// May panic if the underlying E2E registry handle implementation panics | |
| /// (for example, due to a poisoned mutex in the standard implementation). |
| message: &Message<P>, | ||
| ) -> Result<usize, Error> { | ||
| // Get subscribers | ||
| let subscribers = { | ||
| let mgr = self.subscriptions.read().await; | ||
| mgr.get_subscribers(service_id, instance_id, event_group_id) | ||
| }; | ||
| let subscribers = self | ||
| .subscriptions |
There was a problem hiding this comment.
publish_event no longer locks an Arc<Mutex<E2ERegistry>> directly (it now calls through R: E2ERegistryHandle), but the method’s doc comment still claims it panics if the E2E registry mutex is poisoned. Please update/remove that mutex-specific # Panics note so it matches the new handle-based API (panic behavior is now implementation-defined).
| let mut protected = [0u8; UDP_BUFFER_SIZE]; | ||
| let result = registry.protect( | ||
| let result = e2e_registry.protect( | ||
| key, | ||
| &buf[16..message_length], | ||
| upper_header, |
There was a problem hiding this comment.
In the E2E-protect path, a protect() failure is currently handled by logging and then proceeding to send the original (unprotected) message while still reporting Ok(()) to the caller. This can silently violate the contract implied by contains_key(&key) (i.e., that E2E protection is applied when configured).
Consider treating Some(Err(e)) as a send failure: reply on send_message.response with Err(Error::E2e(e)) (or another appropriate error) and continue so the datagram is not sent unprotected.
|
Closing without merge to declutter the stack: this phase's changes are carried in full by the consolidated lineage under PR #114 (phase 21), which the next development stack builds on. Branch is retained. |
This is a chained PR. Prev: #85 Next: #87
Introduces three new traits in transport.rs and subscription_manager.rs:
Client<M, R, I> and Server<R, S> / EventPublisher<R, S> are now generic over these handles with the existing Arc-backed types as defaults, so all existing call sites compile unchanged. Std implementations live in tokio_transport.rs. Gate: all production lock sites route through handle traits; cargo test --all-features passes (454 unit + 11 integration tests).