Conversation
Summary of ChangesHello @sputn1ck, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request establishes a foundational actor model within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Comprehensive PR Review: Actor Model + ProtoFSM ImplementationOverviewThis PR introduces a substantial addition to the codebase: a generic actor model implementation and a protobuf-based finite state machine (FSM) framework. The implementation is well-structured, thoroughly documented, and includes comprehensive test coverage. However, there are several areas that need attention before merging. Critical Issues1. Goroutine Leak Risk in
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a well-designed and comprehensive actor model implementation for Go, along with a protofsm integration. The code is well-structured, with clear separation of concerns, good use of generics, and extensive testing. The documentation, including the README.md and the PROTOFSM_ACTOR_GUIDE.md, is excellent and provides valuable context and usage patterns.
My review has identified a few critical and high-severity issues that should be addressed:
- A potential deadlock in the
StateMachine.Receivemethod. - A panic that can occur when registering an actor on a shut-down
ActorSystem. - Silent message loss due to a
nilDead Letter Office in theRoutedOutboxEventdispatch logic.
I've also included a few medium-severity suggestions for improving documentation clarity and fixing a potential build issue with the specified Go version in go.mod.
Overall, this is a high-quality contribution that will be a great addition to the baselib.
| func (s *StateMachine[InternalEvent, OutboxEvent, Env]) Receive( | ||
| ctx context.Context, e ActorMessage[InternalEvent]) fn.Result[[]OutboxEvent] { | ||
|
|
There was a problem hiding this comment.
The Receive method on StateMachine is intended to implement the actor.ActorBehavior interface. However, it calls s.AskEvent(ctx, e.Event), which sends a message to the syncEvents channel and waits for a response. If this StateMachine instance were used as an actor's behavior, its Receive method would be executed by the actor's single processing goroutine. This same goroutine is also responsible for reading from syncEvents in the driveMachine loop. This creates a deadlock: the goroutine blocks sending to syncEvents and can never read from it. This method is dangerously misleading and should be removed to prevent accidental use. The ActorStateMachine wrapper in actor_wrapper.go provides the correct, non-deadlocking implementation.
| func RegisterWithSystem[M Message, R any](as *ActorSystem, id string, key ServiceKey[M, R], | ||
| behavior ActorBehavior[M, R], | ||
| ) ActorRef[M, R] { |
There was a problem hiding this comment.
Calling RegisterWithSystem on a system that has been shut down will cause a panic. This is because Shutdown() sets as.actors to nil, and RegisterWithSystem then attempts a write to this nil map. The system should gracefully handle operations after shutdown.
A check should be added at the beginning of the function to see if the system is shut down. For example:
if as.ctx.Err() != nil {
// To avoid returning nil and causing a panic, we can create and
// return a reference to a dummy actor that is already stopped.
// This ensures that any calls to the returned ref will fail
// with ErrActorTerminated.
dummyCfg := ActorConfig[M, R]{ID: id}
dummyActor := NewActor(dummyCfg)
dummyActor.Stop()
return dummyActor.Ref()
}| // Create a router for the service key. | ||
| router := actor.NewRouter( | ||
| system.Receptionist(), e.key, actor.NewRoundRobinStrategy[M, R](), | ||
| nil, // No DLO yet. |
There was a problem hiding this comment.
The Dead Letter Office (DLO) for the router is hardcoded to nil. If a Tell message is dispatched via this router and no actors are available for the service key, the message will be dropped silently instead of being routed to the system's DLO for diagnostics. The system's DLO should be used here.
Additionally, creating a new router on every Dispatch call is inefficient and could be a performance bottleneck. Consider caching router instances.
| nil, // No DLO yet. | |
| system.DeadLetters(), |
There was a problem hiding this comment.
I think this is a good suggestion.
| Concurrency is managed by the actor system, allowing many actors to | ||
| This model inherently promotes loose coupling, as actors do not share state | ||
| execute concurrently without explicit lock management by the developer for actor | ||
| state. |
There was a problem hiding this comment.
This paragraph appears to be malformed, likely from an editing error, which makes it difficult to read. The sentences are merged and do not flow correctly.
| Concurrency is managed by the actor system, allowing many actors to | |
| This model inherently promotes loose coupling, as actors do not share state | |
| execute concurrently without explicit lock management by the developer for actor | |
| state. | |
| Concurrency is managed by the actor system, allowing many actors to execute concurrently without explicit lock management by the developer for actor state. This model inherently promotes loose coupling, as actors do not share state. |
| @@ -0,0 +1,45 @@ | |||
| module github.com/lightninglabs/ark/baselib | |||
|
|
|||
| go 1.23.12 | |||
There was a problem hiding this comment.
The Go version is set to 1.23.12, which appears to be a typo or a version that is not yet released. This could cause compatibility issues for other developers. The go.sum file contains entries for Go 1.21. It's recommended to use a recent, stable Go version that the code is compatible with, such as 1.21 or 1.22.
| go 1.23.12 | |
| go 1.21 |
|
I think this piece of slop represents the PR really well: Actor System OverviewThe baselib provides two core components that work together:
This is a message-passing concurrency framework inspired by the actor model: Core Concepts:
Key Implementation Details (from actor/actor.go):
This is a type-safe, event-driven finite state machine: Core Concepts:
Key Implementation Details (from protofsm/state_machine.go):
This is where the magic happens - it bridges FSM and actors: ActorStateMachine (actor_wrapper.go:102) Wraps a StateMachine as an ActorBehavior, enabling:
Key Components: type ActorStateMachine[InternalEvent, OutboxEvent, Env] struct {
sm *StateMachine[...] // The FSM
system *actor.ActorSystem // Access to actor system
currentState State[...] // Current FSM state
}How it works (actor_wrapper.go:149): func (sm *ActorStateMachine) Receive(ctx, e ActorMessage[Event]) Result[ActorResponse] {
// 1. Process event through FSM
newState, outBoxEvents, err := sm.sm.applyEvents(ctx, sm.currentState, e.Event)
// 2. Update state
sm.currentState = newState
// 3. Dispatch outbox events to actors
for _, out := range outBoxEvents {
out.Dispatch(ctx, sm.system) // Routes to other actors!
}
return fn.Ok(ActorResponse{CurrentState: sm.currentState})
}RoutedOutboxEvent (actor_wrapper.go:51) Enables location-transparent message routing: type RoutedOutboxEvent[M, R] struct {
key actor.ServiceKey[M, R] // Where to send
msg M // What to send
mode DeliveryMode // Tell or Ask
}Dispatch (actor_wrapper.go:78):
Environment Integration (actor_wrapper.go:108) The FSM environment can receive an ActorRef: type TellRefEnv[InternalEvent] interface {
SetTellOnlyRef(actor.TellOnlyRef[ActorMessage[InternalEvent]])
GetTellOnlyRef() actor.TellOnlyRef[ActorMessage[InternalEvent]]
}This allows actors to send events back to the FSM (actor_wrapper.go:135-142). Practical Example Flow Looking at example/example_protofsm.go:
Key Benefits
The actor wrapper is essentially the glue that lets you build restart-safe, event-driven workflows |
bhandras
left a comment
bhandras
left a comment
There was a problem hiding this comment.
Really nothing major on my end, LGTM!
I like this new Actor wrapped FSM concept a lot. It took a while to understand but once I got it it's pretty simple actually and very elegant, really nice work! 🎉
| mu sync.RWMutex | ||
|
|
||
| // ctx is the main context for the actor system. | ||
| ctx context.Context |
There was a problem hiding this comment.
Why do we need to store this context?
| extraInfo = "(full ref env)" | ||
| } | ||
|
|
||
| cfg.Logger.Debugf("Setting up FSM %s", extraInfo) |
There was a problem hiding this comment.
If we log, we could log more info about the fsm itself, and what we do exactly. So maybe a name or type name, that we wrap the fsm in an actor and that the actor has full ref or just tell ref env.
There was a problem hiding this comment.
i'm it's the fsms creators responsibility to set up the logger with prefixes. I changed this call to be debugS so the caller can use the ctx to log whatever they like
| // Create a router for the service key. | ||
| router := actor.NewRouter( | ||
| system.Receptionist(), e.key, actor.NewRoundRobinStrategy[M, R](), | ||
| nil, // No DLO yet. |
There was a problem hiding this comment.
I think this is a good suggestion.
| sm := &ActorStateMachine[InternalEvent, OutboxEvent, Env]{ | ||
| sm: &machine, | ||
| system: system, | ||
| currentState: cfg.InitialState, |
There was a problem hiding this comment.
So iiuc this initial state is never published to subscribers right?
PR Review: Actor System + Proto FSM ImplementationOverviewThis PR introduces a comprehensive actor model and finite state machine framework to the codebase. The implementation is well-structured and follows solid engineering principles. Overall, this is excellent work with thorough documentation and testing. ✅ Strengths1. Excellent Documentation
2. Strong Type Safety
3. Robust Concurrency Design
4. Comprehensive Test Coverage
5. Production-Ready Features
|
This commit adds the actor system based on the actor branch in lnd, hash dc5d57f28adda2c894c094f37e29f79d52a99ed0
This commit adds lnd protofsm from the actor branch commit hash dc5d57f28adda2c894c094f37e29f79d52a99ed0
Pull Request Review: baselib - protofsm+actor ImplementationOverviewThis PR introduces a comprehensive actor model implementation and finite state machine (FSM) framework for Go. The implementation is well-architected and provides strong abstractions for concurrent, event-driven workflows. Overall, this is high-quality work with excellent documentation and test coverage. Strengths1. Excellent Documentation
2. Strong Type Safety
3. Good Test Coverage
4. Clean Architecture
Code Quality IssuesCRITICAL: Structured Logging ViolationsPer
|
3 participants
This pull request introduces a comprehensive implementation of a generic actor model for Go, complete with core actor logic, robust testing, and illustrative usage examples. The main changes include the creation of the core
Actortype and supporting infrastructure, a suite of tests validating actor behaviors and lifecycle, and example files demonstrating basic and router-based actor usage.Core actor implementation:
Actortype inactor.go, supporting both "tell" (fire-and-forget) and "ask" (request-response) message patterns, mailbox management, lifecycle control (start/stop), and integration with a Dead Letter Office (DLO) for undeliverable messages. This includes theActorConfigstruct, envelope handling, andActorRef/TellOnlyRefinterfaces.Testing and validation:
actor_test.gowith extensive tests covering actor creation, message handling (tell/ask), error propagation, DLO interactions, and lifecycle management. Includes custom test harnesses, behaviors, and DLO observers for thorough coverage.Usage examples:
example_basic_actor_test.godemonstrating how to create, use, and unregister a simple actor with direct messaging.example_router_test.goillustrating how to spawn multiple actors under a service key and use a router with round-robin dispatching for message distribution.