#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 10 years ago by
comment:2 Changed 10 years ago by
We have also verified that Mark W's patch fixes the problems on our end.
comment:3 Changed 10 years ago by
Component: | Core → Events |
---|---|
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 10 years ago by
Cc: | cjolif added |
---|
comment:5 Changed 10 years ago by
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
We're also encountering this issue in several projects. What's the chance of getting this patch into 1.7.2?