actor: add SingletonRef for service keys with one actor#10759
actor: add SingletonRef for service keys with one actor#10759gijswijs wants to merge 2 commits intolightningnetwork:masterfrom
Conversation
Introduce SingletonRef, an ActorRef implementation that acts as a lookup proxy for service keys expected to have exactly one registered actor. Unlike Router + RoundRobinStrategy, which is overkill (and semantically misleading) for the "one-actor-per-key" pattern, SingletonRef performs a direct receptionist lookup on each Tell/Ask with no routing strategy or extra allocations. Two ServiceKey helpers tie this together: - SpawnSingleton unregisters any existing actor under the key and spawns a replacement, so it is safe to call repeatedly (e.g. when a per-channel actor is re-initialized). It is documented as neither concurrent-atomic nor transactional on registration failure, with a TODO to harden the latter. - Singleton returns a SingletonRef wired to the system's receptionist and dead-letter office, letting consumers reach the actor without holding a direct ActorRef. When more than one actor is registered (a transient race or caller bug), SingletonRef logs the invariant violation and forwards to the first registered actor, so the system remains functional rather than failing closed. Tests cover: no-actor Tell/Ask, basic Tell/Ask, ID format, repeated spawn replacing the prior actor, dynamic register/unregister, multiple-registration tolerance, no-DLO-configured, context cancellation, the Singleton helper, ActorRef interface satisfaction, and repeated-spawn freshness via actor-ID echo.
Summary of ChangesHello, 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 introduces a specialized mechanism for managing singleton actors within the actor system. By replacing the generic Router and RoundRobinStrategy with a dedicated SingletonRef, the implementation reduces allocation overhead and enforces a clear 'one-actor-per-key' invariant. The changes provide a more semantic and efficient API for services that only require a single active instance, while also improving resilience against transient registration races. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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. Footnotes
|
🟡 PR Severity: MEDIUM
🟡 Medium (1 file)
⬜ Excluded from scoring (2 files)
AnalysisThis PR introduces a new Bump thresholds were not triggered: only 1 non-test/non-generated file changed (threshold: >20) and 159 lines added (threshold: >500 lines). To override, add a |
![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 singleton pattern to the actor system, providing a SingletonRef for dynamic actor lookups and a SpawnSingleton method for managing single-actor lifecycles. Feedback was provided to enhance error visibility in SpawnSingleton by logging a warning if registration fails, preventing the service from silently entering a dead state after a non-transactional replacement.
| func (sk ServiceKey[M, R]) SpawnSingleton(as *ActorSystem, id string, | ||
| behavior ActorBehavior[M, R], | ||
| opts ...ActorOption[M, R]) (ActorRef[M, R], error) { | ||
|
|
||
| // Stop and unregister any previous actor for this key so that only one | ||
| // instance exists after we return. | ||
| sk.UnregisterAll(as) | ||
|
|
||
| return RegisterWithSystem(as, id, sk, behavior, opts...) | ||
| } |
There was a problem hiding this comment.
The SpawnSingleton method performs a destructive 'stop-then-register' sequence. As noted in the documentation, this is not transactional. If RegisterWithSystem fails (for example, if the provided id is already in use by another service), the previous actor(s) for this ServiceKey will have already been stopped and unregistered, leaving the service in a 'dead' state. It would be beneficial to log a warning when RegisterWithSystem fails in this context to provide better visibility into why a singleton service might have disappeared.
References
- Minimize lines for log and error messages, while adhering to the 80-character limit. (link)
- Ensure errors are logged effectively, especially in lifecycle operations where failure leads to an inconsistent or degraded system state.
gijswijs
force-pushed
the
actor-singleton-ref
branch
from
e26f092 to
46526db
Compare
1 participant
Summary
Adds a
SingletonReftype to theactorpackage for service keys that are expected to have at most one registered actor, along withServiceKey.SpawnSingletonandServiceKey.Singletonhelpers.SingletonRefis anActorRefimplementation that performs a direct receptionist lookup on each Tell/Ask and forwards to the single registered actor. Compared toRouter + RoundRobinStrategy, it:Public API additions
SingletonRef[M, R]— lookup-proxy type implementingActorRef[M, R]NewSingletonRef(...)— standalone constructor; any consumer can hold oneServiceKey.SpawnSingleton(...)—UnregisterAll+Spawnin one call; safe to invoke repeatedly for the same keyServiceKey.Singleton(as)— convenience that returns aSingletonRefwired to the system's receptionist and DLODesign notes
SpawnSingletonis not atomic across concurrent callers and not transactional on failure. Both caveats are called out in its godoc, and a TODO captures the latter as future hardening.getActorpicks the first registered actor when it seeslen > 1, and logs the invariant violation loudly so the underlying bug is visible.Motivation
Split out from PR #9821, which fronts a singleton RBF-close actor behind
Router + RoundRobinStrategy. That works, but it's both semantically wrong (round-robin over N=1) and more ceremony than the call site needs.To keep this PR focused on the actor package itself, it does not modify #9821. Once that PR lands, a follow-up commit on this branch (or a separate PR) can replace
RbfChanCloserRouter/RoundRobinStrategy/RbfChanCloseActorinpeer/rbf_close_wrapper_actor.gowithSpawnSingleton/Singleton, deleting the router-specific plumbing.The
onionmessagepackage also uses a per-peer singleton (one actor per peer pubkey), but it already holds theActorRefdirectly onBrontiderather than routing through a lookup — so it doesn't needSingletonRef. A separate follow-up could still swap itsserviceKey.SpawnforSpawnSingletonto harden reconnect races (where the old actor hasn't been unregistered yet when the peer reconnects), but that's an independent concern.Test plan
go test ./actor/... -racepassesSingletonhelper,ActorRefinterface satisfaction, repeated-spawn freshness via actor-ID echogo vetandgofmtclean; no lines exceed the 80-column limit (tabs counted as 8)ExampleServiceKey_SpawnSingleton) demonstrates the intended usage for godoc consumers