Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#13743 closed defect (fixed)

[patch] [cla] Subscribing to on.Evented events from event handler

Reported by: Mark Wubben Owned by: Kris Zyp
Priority: high Milestone: 1.8
Component: Events Version: 1.7.0b1
Keywords: Cc: cjolif
Blocked By: Blocking:

Description

Use case:

  • I have an on.Evented instance which emits an event
  • Event handler A removes itself before subscribing a new event handler B

Because the code path for this use case uses after advice, if there is another after advice C to be run after my event handler A, the following occurs:

  • Event handler A removes itself successfully
  • When it subscribes the new event handler B, it's chained from the other after advice C (as a next)
  • After the other after advice C has executed, the new event handler B is run

Unfortunately the other after advice C also removes itself before adding a new event handler D, meaning I'm now caught in an infinite loop, i.e. A -> C -> B -> D -> C' -> D' -> …

The underlying problem is that if you add a new after advice, while after advice is already executing, and at least one other advice is next in line, the new after advice will be run within the same execution. This can lead to infinite loops. Not great for aspects, but that could be considered a design feature. It makes no sense for event handlers though.

This patch changes dojo/aspect to not execute new after advice if after advice is currently being executed: <https://github.com/EqualMedia/dojo/commit/16ecc7796a0988076d588c6a87903a14bc20da80>

Change History (8)

comment:1 Changed 7 years ago by Chris Mitchell

We're also encountering this issue in several projects. What's the chance of getting this patch into 1.7.2?

comment:2 Changed 7 years ago by Chris Mitchell

We have also verified that Mark W's patch fixes the problems on our end.

comment:3 Changed 7 years ago by bill

Component: CoreEvents
Summary: Subscribing to on.Evented events from event handler[patch] [cla] Subscribing to on.Evented events from event handler

It's a little unfortunate to add all that code, although I can appreciate the corner-case markwubben describes.

I wonder if it would be shorter to flag the last advice in the chain at the point that after() started executing it. Or in other words, if you add new advice while after() is running, temporarily mark the new advices with a "new" flag. Either way it sounds like tricky code.

comment:4 Changed 7 years ago by cjolif

Cc: cjolif added

comment:5 Changed 7 years ago by Chris Mitchell

Bill, thanks for taking a look at this.

Another improvement to Mark's patch (from Oliver Rau/IBM CCLA) is

195 +          if(dispatcher.afterAdviseQueue.length){

dispatcher.afterAdviseQueue can be null / undefined here. so changing the if to

195 +          if(dispatcher.afterAdviseQueue && dispatcher.afterAdviseQueue.length){

this corner case is being hit by a number of our teams now as they move to 1.7 eventing

comment:6 Changed 7 years ago by Kris Zyp

Resolution: fixed
Status: newclosed

In [27744]:

Only execute after aspects that were added before the current dispatch execution began, fixes #13743 !strict

comment:7 Changed 7 years ago by Kris Zyp

In [27770]:

Only execute after aspects that were added before the current dispatch execution began, applied to 1.7, refs #13743 !strict

comment:8 Changed 7 years ago by bill

Milestone: tbd1.8

No milestone specified, bulk update to 1.8.

Note: See TracTickets for help on using tickets.