Skip to content

Comments

feat: protocol traits + macro infrastructure#151

Open
ElFantasma wants to merge 2 commits intomainfrom
feat/145-protocol-trait
Open

feat: protocol traits + macro infrastructure#151
ElFantasma wants to merge 2 commits intomainfrom
feat/145-protocol-trait

Conversation

@ElFantasma
Copy link
Collaborator

Summary

  • Adds #[actor] proc macro (spawned-macros crate) with #[send_handler], #[request_handler], #[handler] attributes
  • Adds send_messages!, request_messages!, actor_api! declarative macros for ergonomic message/API definitions
  • Adds Receiver<M> trait (object-safe) and Recipient<M> = Arc<dyn Receiver<M>> for type-erased messaging
  • Adds global name registry (registry::register, whereis, unregister, registered)
  • Renames API methods: send_request()request(), request()request_raw(), send_request_with_timeout()request_with_timeout()
  • Protocol trait pattern for breaking circular actor dependencies (chat_room, ping_pong examples)

What changed

  • New crate: macros/ — proc-macro crate for #[actor]
  • concurrency/src/message.rs — added send_messages!, request_messages!, actor_api!
  • concurrency/src/tasks/actor.rs — method renames + Receiver<M>/Recipient<M> + bridge impls
  • concurrency/src/threads/actor.rs — same for sync version
  • concurrency/src/registry.rs — global Any-based name registry with 6 tests
  • All examples — updated to new API naming
  • New examples: chat_room_threads (macros + protocol traits, sync), service_discovery (registry + Recipient)
  • Rewritten: ping_pong_threads from old Process<M> API to Handler<M> + protocol traits

Test plan

  • cargo build --workspace — compiles clean
  • cargo test --workspace — 34 tests pass
  • cargo run -p chat_room — protocol traits working
  • cargo run -p chat_room_threads — macros + protocol traits (sync)
  • cargo run -p ping_pong / ping_pong_threads — bidirectional messaging
  • cargo run -p bank — request/response with typed results
  • cargo run -p service_discovery — registry + Recipient type erasure
  • cargo run -p blocking_genserver — Backend::Thread isolation

@github-actions
Copy link

🤖 Kimi Code Review

This is a substantial PR that completely re-architects the spawned actor framework. Here are my findings:

Major Issues

1. Breaking API Changes Without Migration Path

The PR completely replaces the existing Actor trait with a new Handler<M>-based approach. This is a breaking change that will break all existing code. While the examples are updated, there's no migration guide or compatibility layer.

2. Missing Error Handling in Registry

In concurrency/src/registry.rs, line 91:

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 Tests

The 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 APIs

The new Handler<M> trait, Context, Recipient<M>, and other new types lack comprehensive documentation. The existing documentation in CLAUDE.md is helpful but doesn't cover the new APIs.

Code Quality Issues

1. Inconsistent Naming

  • ActorStart vs Actor - the trait name is confusing since Actor is now a marker trait
  • Recipient<M> is a type alias for Arc<dyn Receiver<M>> which could be clearer

2. Redundant Trait Bounds

In concurrency/src/tasks/actor.rs, the Handler<M> trait has:

pub trait Handler<M: Message>: Actor {

But Actor already requires Send + Sized + 'static, and Message requires Send + 'static. This creates redundant bounds.

3. Context Methods Could Be More Ergonomic

The Context methods require explicit type parameters:

ctx.send::<MyMessage>(msg)

Consider using inference where possible.

Performance Considerations

1. Boxed Envelopes

Every message is boxed (Box<dyn Envelope<A> + Send>), which adds allocation overhead. This is necessary for type erasure but could impact performance in high-throughput scenarios.

2. Double Arc in Recipient

Recipient<M> is Arc<dyn Receiver<M>> where Receiver<M> is already object-safe. This creates a double indirection.

Suggestions

  1. Add a migration guide showing how to convert from the old enum-based API to the new Handler-based API
  2. Consider a compatibility layer that provides the old API on top of the new one for gradual migration
  3. Add more comprehensive documentation for the new APIs
  4. Consider adding a spawned-derive crate to reduce boilerplate for common patterns
  5. Add benchmarks comparing the old vs new API performance

Positive Aspects

  1. Type Safety: The new Handler<M> approach provides much better type safety than the enum-based approach
  2. Circular Dependency Solution: The protocol trait pattern in examples/chat_room is elegant
  3. Registry: The global registry is a useful addition for service discovery
  4. Macros: The spawned-macros crate reduces boilerplate significantly
  5. Consistency: Both async and sync versions now have consistent APIs

Conclusion

While 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:

  1. Creating a migration release with both old and new APIs
  2. Adding comprehensive migration documentation
  3. Considering a longer deprecation period for the old API

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

@github-actions
Copy link

🤖 Codex Code Review

Findings

  1. #[actor] proc-macro generates impl Handler<...> for ... and uses Context without qualification. This makes the expansion depend on whatever Handler/Context are in scope (or require explicit use), and breaks if spawned-concurrency is renamed in Cargo.toml. Consider fully-qualifying and resolving the crate name via proc_macro_crate, and pick tasks vs threads based on async (e.g., ::spawned_concurrency::tasks::Handler/Context for async, ::spawned_concurrency::threads::Handler/Context for sync). macros/src/lib.rs:83-99.
  2. Global registry has no lifecycle guard; if an actor crashes or stops without manual unregister, the name stays occupied and future register calls will fail, and whereis will return stale handles. This is a correctness/lifecycle risk for service discovery. Consider returning a guard that unregisters on Drop, or provide register_or_replace and strongly document that actors must unregister in stopped. concurrency/src/registry.rs:18-39.

Notes

  • I didn’t run tests.

Summary
Mostly solid changes and the new message/handler APIs look consistent. The main risks are the proc-macro hygiene issue and registry lifecycle management; everything else looks good.


Automated review by OpenAI Codex · custom prompt

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 19, 2026

Greptile Summary

Adds comprehensive macro infrastructure and protocol trait patterns to the spawned actor framework, significantly improving ergonomics and breaking circular dependencies between actors.

Major additions:

  • New spawned-macros crate with #[actor] proc macro and #[send_handler]/#[request_handler] attributes
  • Declarative macros (send_messages!, request_messages!, actor_api!) for concise message and API definitions
  • Receiver<M> trait (object-safe) and Recipient<M> = Arc<dyn Receiver<M>> for type-erased messaging
  • Global name registry (register, whereis, unregister, registered) for service discovery
  • Protocol trait pattern demonstrated in chat_room and ping_pong examples using Arc<dyn Trait> to decouple actors
  • API method renames: send_request()request(), request()request_raw() for clarity

Migration impact:

  • Breaking changes to method names require updates across all examples
  • Old Process<M> API pattern completely replaced with Handler<M> trait approach
  • All 64 files updated consistently across both async (tasks) and sync (threads) backends

Testing:

  • Registry has 6 unit tests covering type safety and concurrent access
  • Examples updated and reportedly passing (34 tests total per PR description)
  • New examples demonstrate macro usage and protocol traits

Confidence Score: 4/5

  • This PR is safe to merge with minor considerations around global state in the registry
  • The implementation is well-structured with comprehensive macro support, proper error handling, and consistent patterns across both async and sync backends. The global registry uses appropriate synchronization primitives. Score is 4 (not 5) due to the inherent risks of global mutable state in the registry, lack of ability to verify test execution, and the large scope of breaking changes across 64 files
  • Pay close attention to concurrency/src/registry.rs (global mutable state) and verify all examples compile and run correctly with the new API

Important Files Changed

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_requestrequest), 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]
Loading

Last reviewed commit: cd0f05b

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.

1 participant