core icon indicating copy to clipboard operation
core copied to clipboard

Set active properties before emitting events

Open Sakeran opened this issue 6 years ago • 2 comments
trafficstars

This changes Effect#activate and Effect#deactivate such that the Effect's active property is set to the proper value before their corresponding events are emitted. Most importantly, this prevents a case where an infinite loop might be created.

Example (some debugEffect.js):

module.exports = {
  config: {
    duration: 1000
  },
  listeners: state => ({
    effectDeactivated: function() {
      this.target.emit('badNews'); // infinite loop
      Broadcast.sayAt(this.target, "Effect deactivated.");
    }
  })
};

In this case, EffectList#validateEffects and the listener function will be stuck invoking one another until the stack overflows. While less likely to occur practice, we could engineer a similar loop with Effect#activate and its related event.

Setting the active property before any events are called ensures that each method will short-circuit before this becomes an issue.

Sakeran avatar Sep 25 '19 18:09 Sakeran

I think I have experienced this as well. I have had to gate the deactivate listener to only fire once in a few things. But never looked into it.

nelsonsbrian avatar Sep 25 '19 20:09 nelsonsbrian

Really nice catch!

azigler avatar Jun 07 '20 04:06 azigler