[Communication] PR4: IAgent and AgentBase Supports Mailbox Delivery#215
[Communication] PR4: IAgent and AgentBase Supports Mailbox Delivery#215Joseph0120 wants to merge 2 commits intojoseph/communication_delay_PR3from
Conversation
|
🧱 Stack PR · Part of stack (9 PRs total) Stack Structure:
|
![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 implements a messaging system for agents by integrating AgentBase with a Mailbox service. It adds registration logic, message handling, and a new NodeType property to the IAgent interface. Feedback was provided regarding the performance of the broadcast-based message delivery and the need for a null check to handle destroyed Unity objects in the message handler.
| private void HandleMailboxDelivery(IAgent receiver, Message message) { | ||
| if (!ReferenceEquals(receiver, this)) { return; } | ||
| OnMessage(message); | ||
| } |
There was a problem hiding this comment.
The current message delivery implementation uses a broadcast pattern where every agent receives every message and filters it locally. This results in Mailbox to use a more targeted delivery system (e.g., a Dictionary of subscribers) if performance becomes an issue.
Additionally, adding a check for this == null is recommended to ensure that messages are not processed by 'zombie' objects that have been destroyed but not yet unsubscribed from the event. In Unity, ReferenceEquals is preferred for identity checks on potentially destroyed objects to avoid the 'both are null' false positive of the overloaded == operator.
private void HandleMailboxDelivery(IAgent receiver, Message message) {
if (this == null || !ReferenceEquals(receiver, this)) { return; }
OnMessage(message);
}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a read-only Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Assets/Scripts/Agents/AgentBase.cs`:
- Around line 358-362: HandleMailboxDelivery is currently invoked for every
subscriber causing O(n) broadcasts; change the Mailbox to dispatch directly to
the intended receiver instead of broadcasting: add a lookup (e.g.,
Dictionary<IAgent, Action<Message>> or Dictionary<IAgent, Queue<Message>>)
inside Mailbox and register agents when they subscribe, update the
Mailbox.Send/Deliver method to find the target IAgent via that dictionary and
call its registered handler (OnMessage) or enqueue the Message, and remove the
broadcast loop so HandleMailboxDelivery is no longer invoked for non-target
agents; ensure the dictionary uses thread-safe access (lock or
ConcurrentDictionary) and update subscription/unsubscription code to maintain
the map.
- Around line 222-225: The unsubscribe uses Mailbox.Instance but registration
used Mailbox.GetOrCreateInstance(), so store the mailbox reference when
registering (e.g., add a private field like _mailboxInstance and set it in
TryRegisterMailbox to Mailbox.GetOrCreateInstance()), use that stored
_mailboxInstance for adding/removing the HandleMailboxDelivery handler and for
null checks, set _mailboxRegistered = false and clear _mailboxInstance after
unsubscribing to avoid stale references and ensure consistent unsubscription.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7a15b87a-74af-4a86-a02b-7e725059abe0
📒 Files selected for processing (2)
Assets/Scripts/Agents/AgentBase.csAssets/Scripts/Agents/IAgent.cs
| if (_mailboxRegistered && Mailbox.Instance != null) { | ||
| Mailbox.Instance.OnMessageDelivered -= HandleMailboxDelivery; | ||
| _mailboxRegistered = false; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor inconsistency: Mailbox.Instance vs Mailbox.GetOrCreateInstance().
The subscription in TryRegisterMailbox() uses Mailbox.GetOrCreateInstance() and stores the handler on that instance, but the unsubscription uses Mailbox.Instance directly. If the Mailbox singleton were ever recreated between registration and destruction, the unsubscription would target a different object.
Given that Mailbox uses DontDestroyOnLoad, this is unlikely to cause issues in practice, but for consistency you could store a reference to the mailbox instance during registration:
♻️ Optional: Store mailbox reference for consistent unsubscription
// Make sure the same agent does not subscribe to the mailbox event more than once (keeping track).
private bool _mailboxRegistered = false;
+ private Mailbox _registeredMailbox = null;
// In TryRegisterMailbox():
protected void TryRegisterMailbox() {
if (_mailboxRegistered) { return; }
Mailbox mailbox = Mailbox.GetOrCreateInstance();
if (mailbox == null) { return; }
mailbox.OnMessageDelivered += HandleMailboxDelivery;
+ _registeredMailbox = mailbox;
_mailboxRegistered = true;
}
// In OnDestroy():
- if (_mailboxRegistered && Mailbox.Instance != null) {
- Mailbox.Instance.OnMessageDelivered -= HandleMailboxDelivery;
+ if (_mailboxRegistered && _registeredMailbox != null) {
+ _registeredMailbox.OnMessageDelivered -= HandleMailboxDelivery;
+ _registeredMailbox = null;
_mailboxRegistered = false;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Assets/Scripts/Agents/AgentBase.cs` around lines 222 - 225, The unsubscribe
uses Mailbox.Instance but registration used Mailbox.GetOrCreateInstance(), so
store the mailbox reference when registering (e.g., add a private field like
_mailboxInstance and set it in TryRegisterMailbox to
Mailbox.GetOrCreateInstance()), use that stored _mailboxInstance for
adding/removing the HandleMailboxDelivery handler and for null checks, set
_mailboxRegistered = false and clear _mailboxInstance after unsubscribing to
avoid stale references and ensure consistent unsubscription.
| // Delivers a message to the right receiver. Check if self is the right receiver, if so then handle the message. | ||
| private void HandleMailboxDelivery(IAgent receiver, Message message) { | ||
| if (!ReferenceEquals(receiver, this)) { return; } | ||
| OnMessage(message); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider performance implications of broadcast pattern.
The HandleMailboxDelivery method correctly uses ReferenceEquals for identity comparison. However, per the design described in the PR, every message delivery invokes this handler on all subscribed agents, resulting in O(n) invocations per message.
This is acceptable for a small number of agents but may become a bottleneck with many agents. If message volume or agent count grows significantly, consider a dictionary-based lookup in the Mailbox to dispatch directly to the intended receiver rather than broadcasting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Assets/Scripts/Agents/AgentBase.cs` around lines 358 - 362,
HandleMailboxDelivery is currently invoked for every subscriber causing O(n)
broadcasts; change the Mailbox to dispatch directly to the intended receiver
instead of broadcasting: add a lookup (e.g., Dictionary<IAgent, Action<Message>>
or Dictionary<IAgent, Queue<Message>>) inside Mailbox and register agents when
they subscribe, update the Mailbox.Send/Deliver method to find the target IAgent
via that dictionary and call its registered handler (OnMessage) or enqueue the
Message, and remove the broadcast loop so HandleMailboxDelivery is no longer
invoked for non-target agents; ensure the dictionary uses thread-safe access
(lock or ConcurrentDictionary) and update subscription/unsubscription code to
maintain the map.
1 participant
Files Modified:
AgentBase -> TryRegisterMailbox() when Start() or Awake(). Unsubscribe to mailbox when OnDestroy().
TryRegisterMailbox() subscribes to the mailbox, and prevents redundant registration.
HandleMailboxDelivery(IAgent receiver, Message message) Check if self is the right receiver, if so then handle the message. Because a popped message is broadcasted to all subscribed agents and it is the agent's responsibility to check if it is the recipient. (Mailbox sends mail to everyone, and only the right recipient has the "key" to unlock that message)
IAgent -> Added NodeType, which tells the mailbox what type of Agent it is reading from.