Skip to content

Comments

Approach A: Recipient<M> + actor_api! pattern#152

Open
ElFantasma wants to merge 14 commits intomainfrom
feat/approach-a
Open

Approach A: Recipient<M> + actor_api! pattern#152
ElFantasma wants to merge 14 commits intomainfrom
feat/approach-a

Conversation

@ElFantasma
Copy link
Collaborator

Summary

  • Rewrites all cross-boundary examples (chat_room, ping_pong, ping_pong_threads) to use pure Recipient<M> + actor_api! for type erasure
  • Removes Response<T> and protocol trait patterns — cross-boundary communication uses Recipient<M> exclusively
  • chat_room_threads already used this pattern, unchanged

Shared infrastructure from feat/actor-macro-registry: Handler<M>, #[actor] proc macro, send_messages!/request_messages!, Receiver<M>/Recipient<M>, named registry, actor_api! macro.

Key pattern: Actors that need to communicate across boundaries pass Recipient<M> (type-erased message inbox). One-way dependency is acceptable (e.g. user.rs imports from room.rs).

Test plan

  • cargo test -p spawned-concurrency — 34 tests passing
  • cargo run -p chat_room
  • cargo run -p chat_room_threads
  • cargo run -p ping_pong
  • cargo run -p ping_pong_threads
  • cargo run -p bank
  • cargo run -p service_discovery
  • git grep "Response<" -- concurrency/ → 0 results
  • git grep "protocol_impl" → 0 results

… API

Introduces a unified API redesign addressing #144, #145, and #129:

- Handler<M> trait with per-message typed results (RPITIT, not object-safe)
- Receiver<M>/Recipient<M> for type-erased cross-actor messaging
- #[actor] proc macro that generates Handler<M> impls from #[handler] methods
- Any-based named registry (register/whereis/unregister)
- messages! declarative macro for defining message structs
- Context<A> replaces ActorRef<A> in handler signatures
- New examples: chat_room (Recipient), service_discovery (registry)
- All existing examples migrated to new API
…rait

# Conflicts:
#	Cargo.lock
#	Cargo.toml
#	concurrency/src/message.rs
#	concurrency/src/tasks/mod.rs
#	concurrency/src/threads/mod.rs
#	examples/bank/src/main.rs
#	examples/bank/src/messages.rs
#	examples/bank/src/server.rs
#	examples/bank_threads/src/main.rs
#	examples/bank_threads/src/messages.rs
#	examples/bank_threads/src/server.rs
#	examples/blocking_genserver/main.rs
#	examples/busy_genserver_warning/main.rs
#	examples/chat_room/Cargo.toml
#	examples/chat_room/src/main.rs
#	examples/chat_room/src/room.rs
#	examples/chat_room/src/user.rs
#	examples/name_server/src/main.rs
#	examples/name_server/src/messages.rs
#	examples/name_server/src/server.rs
#	examples/service_discovery/Cargo.toml
#	examples/service_discovery/src/main.rs
#	examples/signal_test/src/main.rs
#	examples/signal_test_threads/src/main.rs
#	examples/updater/src/main.rs
#	examples/updater/src/messages.rs
#	examples/updater/src/server.rs
#	examples/updater_threads/src/main.rs
#	examples/updater_threads/src/messages.rs
@github-actions
Copy link

🤖 Kimi Code Review

This PR represents a major API redesign for the spawned actor framework. The changes are substantial and well-structured, addressing critical issues #144 and #145 through a new Handler + Recipient approach.

Key Improvements

✅ Type Safety (#144): The new Handler<M> trait provides per-message type safety, eliminating the need for large enum matches with impossible variants.

✅ Circular Dependencies (#145): Recipient<M> enables type-erased communication between actors without circular module dependencies.

✅ Ergonomic APIs: The actor_api! macro and message macros (messages!, send_messages!, request_messages!) significantly reduce boilerplate.

✅ Registry Support: New global registry for named actors with type-safe discovery.

Code Quality Issues

1. Potential Race Condition in Registry (registry.rs:91)

pub fn registered() -> Vec<String> {
    let store = global_store().read().unwrap_or_else(|p| p.into_inner());
    store.keys().cloned().collect()
}

Issue: The RwLock is held across the entire collection operation. If another thread writes to the registry while this is running, it could block. Consider using try_read() or limiting lock scope.

2. Missing Error Handling in Macro (macros/src/lib.rs:112)

The #[actor] macro doesn't handle the case where a handler method has incorrect signature. It should provide better compile-time diagnostics.

3. Context Clone in Timer Functions (tasks/time.rs:12, threads/time.rs:3)

Both send_after and send_interval take Context<A> by value, which may be expensive to clone for every timer. Consider taking &Context<A> or optimizing clone behavior.

4. Inconsistent Naming

  • ActorStart trait vs Actor trait naming could be confusing
  • Recipient<M> vs Receiver<M> trait naming is inconsistent

5. Documentation Gaps

  • The Response<T> type mentioned in docs isn't implemented in this PR
  • Missing examples for protocol traits approach (Approach B)

Performance Considerations

✅ Good: The envelope pattern with Box<dyn Envelope<A>> is efficient for type erasure.
⚠️ Watch: Each Recipient<M> involves an Arc<dyn Receiver<M>> which adds allocation overhead. This is acceptable for the flexibility gained.

Testing

The extensive test suite (34+ tests) provides good coverage for:

  • All three backends (Async, Blocking, Thread)
  • Message passing correctness
  • Registry functionality
  • Timer behavior
  • Actor lifecycle

Migration Path

The examples show a clean migration from the old enum-based API to the new Handler approach. The breaking changes are well-justified by the improvements in type safety and expressiveness.

Summary

This is a high-quality PR that successfully addresses the fundamental API issues. The implementation is solid, though minor improvements could be made in error handling and documentation. The new API is significantly more ergonomic and type-safe than the previous version.


Automated review by Kimi (Moonshot AI) · custom prompt

@github-actions
Copy link

🤖 Codex Code Review

Findings

  1. macros/src/lib.rs:45-52
    #[actor] only removes the first matching handler attribute. If a method is accidentally annotated with both #[send_handler] and #[request_handler] (or #[handler] plus one of the others), the remaining attribute will cause an “unknown attribute” compile error. Recommend removing all handler-related attributes via retain (e.g., method.attrs.retain(|attr| !is_handler_attr(attr))).

  2. concurrency/src/registry.rs:18-39
    The global registry never auto-unregisters, so if an actor stops without calling unregister, stale ActorRef/Recipient values remain forever. That’s a lifecycle footgun and a memory leak in long-running systems. Consider returning a RegistrationGuard that unregisters on Drop, or provide a register_with(ctx) that hooks into the actor CancellationToken/stopped() path.

Notes

  • The message/handler split and the updated examples look consistent and idiomatic overall.
  • I didn’t run tests in this review.

Possible next steps

  1. I can implement the #[actor] attribute cleanup change.
  2. I can sketch a small RegistrationGuard API and update the service discovery example to use it.

Automated review by OpenAI Codex · custom prompt

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 20, 2026

Greptile Summary

This PR implements a major architectural refactor to adopt the "Approach A" pattern (Recipient<M> + actor_api!) for cross-boundary actor communication. The changes replace the previous protocol trait patterns with a more ergonomic type-erased messaging system.

Key architectural changes:

  • Introduced Message trait with associated Result type, replacing separate request/message types
  • Added Handler<M> trait for per-message type handling (non-object-safe, uses RPITIT)
  • Implemented Recipient<M> = Arc<dyn Receiver<M>> for type-erased actor references
  • Created envelope-based message dispatch using trait objects for internal type erasure
  • Simplified Actor trait to just started/stopped lifecycle hooks
  • Added declarative macros (messages!, send_messages!, request_messages!, actor_api!)
  • Introduced #[actor] proc macro for generating Handler implementations
  • Added global actor registry for named actor lookup

Pattern benefits:

  • Type-safe message passing with compile-time verification
  • Ergonomic API generation via actor_api! macro
  • Clean separation between actor implementation and API
  • Supports both async (tasks) and sync (threads) runtimes
  • Enables one-way dependencies (e.g., User imports Room types, not vice versa)

Test coverage: All 34 tests passing, examples verified working

Confidence Score: 4/5

  • This PR is safe to merge with proper testing—it's a well-structured refactor with comprehensive test coverage
  • Score reflects solid implementation quality with all tests passing, but reduced by 1 point due to the magnitude of changes (4220+ additions, 2358 deletions) affecting core actor infrastructure across 61 files. While the refactor is well-executed and examples demonstrate correctness, thorough integration testing in production-like scenarios is recommended given the scope.
  • Pay close attention to concurrency/src/tasks/actor.rs and concurrency/src/threads/actor.rs as they implement the core message dispatch and type erasure mechanisms

Important Files Changed

Filename Overview
concurrency/src/message.rs added new Message trait and macro infrastructure for defining messages (messages!, send_messages!, request_messages!, actor_api!)
concurrency/src/registry.rs added global actor registry with register, whereis, unregister, and registered functions using type-erased storage
concurrency/src/tasks/actor.rs major refactor to Recipient<M> pattern with Handler<M> trait, envelope-based type erasure, simplified Actor trait with started/stopped lifecycle hooks
concurrency/src/threads/actor.rs parallel refactor to tasks version, implementing same Recipient<M> pattern for sync (threads-based) actors
macros/src/lib.rs added #[actor] proc macro that generates Handler<M> implementations from #[send_handler] and #[request_handler] annotated methods
examples/chat_room/src/room.rs rewrote to use Recipient<Deliver> for type-erased member references, actor_api! for API trait generation
examples/chat_room/src/user.rs rewrote to use ctx.recipient::<Deliver>() and actor_api! pattern, one-way dependency on room module
examples/ping_pong/src/producer.rs rewrote to hold Recipient<Ping> instead of typed actor reference
concurrency/src/error.rs simplified error types, removed unused variants, renamed Server to ActorStopped

Class Diagram

%%{init: {'theme': 'neutral'}}%%
classDiagram
    class Message {
        <<trait>>
        +type Result
    }
    
    class Actor {
        <<trait>>
        +started(ctx)
        +stopped(ctx)
    }
    
    class Handler~M~ {
        <<trait>>
        +handle(msg, ctx) Result
    }
    
    class ActorRef~A~ {
        -sender
        -cancellation_token
        -completion_rx
        +send(msg)
        +request(msg)
        +recipient() Recipient~M~
        +context() Context~A~
    }
    
    class Context~A~ {
        -sender
        -cancellation_token
        +send(msg)
        +request(msg)
        +recipient() Recipient~M~
        +stop()
    }
    
    class Receiver~M~ {
        <<trait>>
        +send(msg)
        +request_raw(msg)
    }
    
    class Recipient~M~ {
        <<type alias>>
        Arc~dyn Receiver~M~~
    }
    
    class Envelope~A~ {
        <<trait>>
        +handle(actor, ctx)
    }
    
    class MessageEnvelope~M~ {
        -msg: M
        -tx: Option~Sender~
        +handle(actor, ctx)
    }

    Actor <|.. Handler~M~
    Handler~M~ ..> Message
    Envelope~A~ <|.. MessageEnvelope~M~
    ActorRef~A~ ..|> Receiver~M~
    Context~A~ ..|> Receiver~M~
    Receiver~M~ <.. Recipient~M~
    ActorRef~A~ --> Context~A~
    MessageEnvelope~M~ ..> Handler~M~
Loading

Last reviewed commit: 7693d45

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