feat: protocol traits + macro infrastructure#151
Conversation
🤖 Kimi Code ReviewThis is a substantial PR that completely re-architects the spawned actor framework. Here are my findings: Major Issues1. Breaking API Changes Without Migration PathThe PR completely replaces the existing 2. Missing Error Handling in RegistryIn pub fn registered() -> Vec<String> {
let store = global_store().read().unwrap_or_else(|p| p.into_inner());
store.keys().cloned().collect()
}This should handle the poisoned mutex case more gracefully, but the current approach is acceptable. 3. Potential Race Condition in Registry TestsThe registry tests use unique names per test, but they're still sharing global state. Consider using a test-specific registry instance or proper isolation. 4. Missing Documentation for New APIsThe new Code Quality Issues1. Inconsistent Naming
2. Redundant Trait BoundsIn pub trait Handler<M: Message>: Actor {But 3. Context Methods Could Be More ErgonomicThe ctx.send::<MyMessage>(msg)Consider using inference where possible. Performance Considerations1. Boxed EnvelopesEvery message is boxed ( 2. Double Arc in Recipient
Suggestions
Positive Aspects
ConclusionWhile this PR significantly improves the framework's type safety and ergonomics, the breaking nature of the changes requires careful consideration. The implementation is solid, but I'd recommend:
The new design is superior to the old enum-based approach, but the transition needs to be managed carefully. Automated review by Kimi (Moonshot AI) · custom prompt |
🤖 Codex Code ReviewFindings
Notes
Summary Automated review by OpenAI Codex · custom prompt |
Greptile SummaryAdds comprehensive macro infrastructure and protocol trait patterns to the spawned actor framework, significantly improving ergonomics and breaking circular dependencies between actors. Major additions:
Migration impact:
Testing:
Confidence Score: 4/5
|
| Filename | Overview |
|---|---|
| macros/src/lib.rs | Implements #[actor] proc macro for generating Handler trait implementations from annotated methods - clean implementation with proper error handling |
| concurrency/src/message.rs | Added declarative macros (send_messages!, request_messages!, actor_api!) for ergonomic message and API definitions - well-structured macro implementations |
| concurrency/src/registry.rs | Implemented global Any-based registry with register, whereis, unregister - has comprehensive tests but uses global mutable state |
| concurrency/src/tasks/actor.rs | Added Receiver<M> trait and Recipient<M> type alias for type-erased messaging, renamed methods (send_request → request), bridge implementations for Context/ActorRef |
| concurrency/src/threads/actor.rs | Sync version of actor changes - same Receiver/Recipient pattern and method renames, consistent with async implementation |
| examples/chat_room/src/protocols.rs | Protocol traits pattern for breaking circular dependencies - actors communicate through Arc<dyn Trait> instead of concrete types |
| examples/chat_room_threads/src/room.rs | Demonstrates macro usage with #[actor], #[send_handler], #[request_handler] and declarative message macros - clean integration |
| examples/service_discovery/src/main.rs | Shows registry and Recipient usage - registers type-erased recipients by name for service discovery |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Actor Implementation] --> B{Message Type?}
B -->|Send| C[send_handler attribute]
B -->|Request| D[request_handler attribute]
C --> E[actor macro]
D --> E
E --> F[Generate Handler impl]
F --> G[ActorRef]
F --> H[Context]
G --> I{Type Erasure?}
H --> I
I -->|Yes| J[Recipient type alias]
I -->|No| K[Direct send/request]
J --> L[Registry register]
L --> M[Global Name Service]
M --> N[Registry whereis]
K --> O[Message Handler]
J --> O
O --> P{Protocol Trait?}
P -->|Yes| Q[Arc dyn Trait breaks circular deps]
P -->|No| R[Direct ActorRef usage]
Last reviewed commit: cd0f05b
Summary
#[actor]proc macro (spawned-macroscrate) with#[send_handler],#[request_handler],#[handler]attributessend_messages!,request_messages!,actor_api!declarative macros for ergonomic message/API definitionsReceiver<M>trait (object-safe) andRecipient<M> = Arc<dyn Receiver<M>>for type-erased messagingregistry::register,whereis,unregister,registered)send_request()→request(),request()→request_raw(),send_request_with_timeout()→request_with_timeout()chat_room,ping_pongexamples)What changed
macros/— proc-macro crate for#[actor]concurrency/src/message.rs— addedsend_messages!,request_messages!,actor_api!concurrency/src/tasks/actor.rs— method renames +Receiver<M>/Recipient<M>+ bridge implsconcurrency/src/threads/actor.rs— same for sync versionconcurrency/src/registry.rs— global Any-based name registry with 6 testschat_room_threads(macros + protocol traits, sync),service_discovery(registry + Recipient)ping_pong_threadsfrom oldProcess<M>API toHandler<M>+ protocol traitsTest plan
cargo build --workspace— compiles cleancargo test --workspace— 34 tests passcargo run -p chat_room— protocol traits workingcargo run -p chat_room_threads— macros + protocol traits (sync)cargo run -p ping_pong/ping_pong_threads— bidirectional messagingcargo run -p bank— request/response with typed resultscargo run -p service_discovery— registry + Recipient type erasurecargo run -p blocking_genserver— Backend::Thread isolation