-
Notifications
You must be signed in to change notification settings - Fork 26
Best Practices
Event-driven architecture is great, but has some common pitfalls. The guidelines below (in no specific order) should help avoid some of these. Most of them apply to any such design, albeit with Relay-specific examples.
Some quick notes on terminology.
Listener - the method added to the Relay. This can be normal, static, delegate, lambda or closure.
Observer - the object that owns/declared the listener.
- Always remove listeners at end-of-life
- Try to stick to AddOnce for lambdas
- Don't depend on invocation order
- Listeners can remove themselves, but not other listeners
- Listeners on disabled MonoBehaviours still get callbacks!
- Add/Remove vs toggle in-method? Whichever you prefer.
- Try to stick to RelayLink for passing references
The most common problem with events is "dangling" or "lapsed" listeners. This is when you don't clean up your listeners at end-of-life - as such a reference to the observer is kept by the event/Relay and the GC can't claim it, leading to "memory leaks" (quotes because no, they're not technically leaks, blah blah blah).
So always remove listeners when they're not needed any more.
class ExampleBehaviour : MonoBehaviour {
Relay _r;
void MyListener(){...}
public void Setup(Relay r){
_r = r;
_r.AddListener(MyListener);
}
void OnDestroy(){
if (_r != null){
_r.RemoveListener(MyListener);
}
}
}
Note that RemoveListener automatically checks if the Relay actually has that listener, so don't worry about using Contains() first.
What if you're using a lambda/anonymous method? Well -
Since lambdas cannot (easily) be removed, and C# maintains a reference to the object that created them, naively using them as listeners is playing with fire. However, AddOnce() works fine with lambdas so go crazy. For anything that's called frequently, consider moving to a regular method. Or create a delegate, store a reference to that, and remove the delegate at end-of-life.
Events are for systems with low coupling (and help encourage this). If your code depends on the order in which an event's listeners are invoked, you're doing it wrong (obviously lots of code does depend on explicit ordering, but listeners to the same event should not).
void SetupStuff(){...} // This must be called first
void DoStuff(){...} // This must be called second
...
Relay doTheStuff;
doTheStuff.AddListener(SetupStuff);
doTheStuff.AddListener(DoStuff);
Relay onPrep;
Relay onExec;
onPrep.AddListener(SetupStuff);
onExec.AddListener(DoStuff);
...
onPrep.Dispatch();
onExec.Dispatch();
For the record, Relay actually invokes in reverse order - this is to ensure synchronous listener addition and self-removal is safe (see the next section).
Relay explicitly supports synchronous listener self-removal and arbitrary addition. I just made that phrase up and I don't know if there's already another name for it.
Meaning: a method called by a Relay may freely remove itself from that Relay and add any other methods to it. However, it should not remove other listeners. It's not supported, and it's bad anyway.
Relay myRelay;
myRelay.AddListener(B);
...
void A(){...}
void B(){
// GOOD
myRelay.RemoveListener(B); // Remove this method from myRelay while it's executing
myRelay.AddListener(A); // Add a different method to myRelay while it's executing
// Note: A() will NOT be called until the next Dispatch, by design
// BAD
myRelay.RemoveListener(A); // Remove another listener from myRelay while it's executing
myRelay.RemoveAll(); // Remove all listeners from myRelay while it's executing
}
What happens if you do? Probably weird stuff. Might be obvious, might not, so be strict.
As for why it shouldn't be done even if supported, two main reasons. Firstly, to do so "safely" requires knowledge of invocation order (are you removing the other listener before or after it's been called?), which is already bad as stated above. Secondly, it's the event equivalent of goto. It creates confusing flow immediately... and implies that the rest of your code may already be callback hell anyway.
Relay (like event) doesn't know about MonoBehaviour.enabled. If you need to stop responding to Relay dispatches when disabled, remember to implement that yourself somehow. If unsure, see below for how.
To temporarily "turn off" a listener, should you remove the listener then add again, or check a boolean in the listener itself? Short answer - either is fine. There's no significant GC allocation or performance issue in either case. For Relays dispatched every frame or more, you may find remove/re-add to be marginally more performant, but in most cases this is micro-optimisation. Do what you prefer.
class MyCharacter : MonoBehaviour {
Relay _onJump;
public void SetupJump(Relay onJump){
_onJump = onJump;
_onJump.AddListener(Jump);
}
void Jump(){...}
void OnEnable(){
_onJump.AddListener(Jump); // Checks for uniqueness by default so won't duplicate
}
void OnDisable(){
_onJump.RemoveListener(Jump);
}
}
class MyCharacter : MonoBehaviour {
Relay _onJump;
public void SetupJump(Relay onJump){
_onJump = onJump;
_onJump.AddListener(Jump);
}
void Jump(){
if (enabled){...}
}
void OnDestroy(){
_onJump.RemoveListener(Jump);
}
}
Generally, the only object that should call Relay.Dispatch() should be the owner. To keep encapsulation try to use Relay.link to get a "safe" reference just for adding/removing listeners. This ensures there's only one way for Dispatch to get called, leading to clearer flow.
class MyDispatcher {
Relay _myRelay = new Relay();
IRelayLink myRelay {get {return _myRelay.link;}} // Other code can see this and add/remove, but not dispatch
void Go(){
_myRelay.Dispatch();
}
}
Likewise, try to use IRelayLink for method arguments rather than Relay.
class MyObserver {
IRelayLink _relay;
void MyListener(){...}
public void Setup(IRelayLink relay){
_relay = relay;
_relay.AddListener(MyListener);
}
public void Teardown(){
if (_relay != null){
_relay.RemoveListener(MyListener);
}
}
}