Class events fail on first failure #2226

Closed
ibolmo opened this Issue Jan 19, 2012 · 11 comments

Comments

Projects
None yet
7 participants
@ibolmo
Owner

ibolmo commented Jan 19, 2012

(from Lighthouse)

OK - so title isn't very well understood but here's the main issue-
Whenever a broken function is fired using fireEvent, it will break the event loop.

This is not standard DOM events behavior, and it creates a blind dependency between different objects that might be listening to the same shared object.

I've set up a demo page here:
http://jsfiddle.net/6L2qf/

the first anchor shows how a Class behaves. The second how a DOM element behaves, and the third shows a fixed version of Events.

The fix itself is quite simple:

simply making each function fire with a setTimeout of 0 would solve this. This line (and the following line):
https://github.com/mootools/mootools-core/blob/master/Source/Class/...
simply would be replaced with

fn.delay(delay || 0, this, args);

I'll mention that this isn't a theoretical problem. On an app I'm working on, I have a Dispatcher Class that is used by many other objects. During dev, whenever there's a bug with one of these listeners, it hurts all others who listen to the same events. As I understand it, events are supposed to be a very useful means of decoupling between objects. This makes the events API unsafe for shared environments (a bit of exaggeration but you get my point).

Currently I simply use the fix above, but IMO this should be a part of the main implementation

@arian

This comment has been minimized.

Show comment Hide comment
@arian

arian Jan 19, 2012

Owner

Also see this blogpost about this problem: http://dean.edwards.name/weblog/2009/03/callbacks-vs-events/

Owner

arian commented Jan 19, 2012

Also see this blogpost about this problem: http://dean.edwards.name/weblog/2009/03/callbacks-vs-events/

@assertchris

This comment has been minimized.

Show comment Hide comment
@assertchris

assertchris Aug 8, 2012

What about try/catching exceptions from within Events? Bad for traditional debugging but ok for keeping the events going...

What about try/catching exceptions from within Events? Bad for traditional debugging but ok for keeping the events going...

@ibolmo ibolmo modified the milestones: 1.5.1, 1.6 Mar 3, 2014

@SergioCrisostomo SergioCrisostomo modified the milestones: 1.5.2, 1.5.1 Jul 3, 2014

@ibolmo

This comment has been minimized.

Show comment Hide comment
@ibolmo

ibolmo Nov 11, 2014

Owner

@arian is this something with should bother with? or let the devs deal with the exception handling. I'm leaning on the latter.

Owner

ibolmo commented Nov 11, 2014

@arian is this something with should bother with? or let the devs deal with the exception handling. I'm leaning on the latter.

@GCheung55

This comment has been minimized.

Show comment Hide comment
@GCheung55

GCheung55 Nov 11, 2014

Contributor

The reason is because they are synchronous, right? Should we consider making them asynchronous?

On Nov 10, 2014, at 9:39 PM, Olmo Maldonado notifications@github.com wrote:

@arian is this something with should bother with? or let the devs deal with the exception handling. I'm leaning on the latter.


Reply to this email directly or view it on GitHub.

Contributor

GCheung55 commented Nov 11, 2014

The reason is because they are synchronous, right? Should we consider making them asynchronous?

On Nov 10, 2014, at 9:39 PM, Olmo Maldonado notifications@github.com wrote:

@arian is this something with should bother with? or let the devs deal with the exception handling. I'm leaning on the latter.


Reply to this email directly or view it on GitHub.

@kentaromiura

This comment has been minimized.

Show comment Hide comment
@kentaromiura

kentaromiura Nov 11, 2014

Member

No! if there's something people really rely on is the sync'ity of fireEvent, making it async will break tons of code, we can implement fireEventAsync though (with a better name)

Member

kentaromiura commented Nov 11, 2014

No! if there's something people really rely on is the sync'ity of fireEvent, making it async will break tons of code, we can implement fireEventAsync though (with a better name)

@arian

This comment has been minimized.

Show comment Hide comment
@arian

arian Nov 11, 2014

Owner

As @kentaromiura making events async by default will break things. Adding an extra method might work, but that complicates the API, and as nobody has commented on this issue in two years, I doubt about the actual necessity.

Owner

arian commented Nov 11, 2014

As @kentaromiura making events async by default will break things. Adding an extra method might work, but that complicates the API, and as nobody has commented on this issue in two years, I doubt about the actual necessity.

@timwienk

This comment has been minimized.

Show comment Hide comment
@timwienk

timwienk Nov 11, 2014

Owner

I agree about its perceived necessity, however: untrusted user code should be executed with a clean stack to avoid these kinds of problems, plus the fact that acting upon events is quite obviously an asynchronous process. This is a design oversight on our part that's been there for a long time. I'm unsure what the best course of action is, since @kentaromiura's comment regarding breakage is probably true as well. I see the following options:

  • A try-catch around every execution to keep things synchronous. (Maybe manually re-throw the caught errors after all events were executed? (with a clean stack?)) (1.6?),
  • Asynchronous execution of each handler (only way to execute with a clean stack, as far as I know) (1.6?),
  • Keep current behaviour (and optionally add the new method @kentaromiura suggested, though I feel that's quite weird, since Class X would basically have to be aware of which way Class Y uses to fire its events) (could optionally be 1.5.x).
Owner

timwienk commented Nov 11, 2014

I agree about its perceived necessity, however: untrusted user code should be executed with a clean stack to avoid these kinds of problems, plus the fact that acting upon events is quite obviously an asynchronous process. This is a design oversight on our part that's been there for a long time. I'm unsure what the best course of action is, since @kentaromiura's comment regarding breakage is probably true as well. I see the following options:

  • A try-catch around every execution to keep things synchronous. (Maybe manually re-throw the caught errors after all events were executed? (with a clean stack?)) (1.6?),
  • Asynchronous execution of each handler (only way to execute with a clean stack, as far as I know) (1.6?),
  • Keep current behaviour (and optionally add the new method @kentaromiura suggested, though I feel that's quite weird, since Class X would basically have to be aware of which way Class Y uses to fire its events) (could optionally be 1.5.x).
@ibolmo

This comment has been minimized.

Show comment Hide comment
@ibolmo

ibolmo Nov 11, 2014

Owner

I agree with Tim's options. Here are my comments though on each (reference in order):

  • this will not be performant. Especially on mousemove. We can be specific to different type of events but we wouldn't be changing the paradigm.
  • We can accomplish a non-bc by wrapping new set time out (async) with compat block. Therefore this would be a 1.6.0 pr
  • I'd be OK with this as well. I've personally dealt with such cases in a per case basis. We could document the practice in the docs for 1.5.2

If docs on the best practice is enough let's do that for 1.5.2. If this is a huge burden on developers let's add async with compat block. I propose the former and keep an eye out for more requests.

Owner

ibolmo commented Nov 11, 2014

I agree with Tim's options. Here are my comments though on each (reference in order):

  • this will not be performant. Especially on mousemove. We can be specific to different type of events but we wouldn't be changing the paradigm.
  • We can accomplish a non-bc by wrapping new set time out (async) with compat block. Therefore this would be a 1.6.0 pr
  • I'd be OK with this as well. I've personally dealt with such cases in a per case basis. We could document the practice in the docs for 1.5.2

If docs on the best practice is enough let's do that for 1.5.2. If this is a huge burden on developers let's add async with compat block. I propose the former and keep an eye out for more requests.

@timwienk

This comment has been minimized.

Show comment Hide comment
@timwienk

timwienk Nov 11, 2014

Owner

Regarding your comment on the first point: this is about the Events Class mixin, not Element.Events. A possible performance issue regarding mousemove doesn't affect the Events class mixin as far as I'm aware. (Also, Element.Events uses native events, it should already be asynchronous.)

Example regarding wrapping in a try-catch:
http://jsfiddle.net/358jjnsf/

fireEvent: function(type, args, delay){
    type = removeOn(type);
    var events = this.$events[type];
    if (!events) return this;
    args = Array.from(args);
    events.each(function(fn){
        if (delay){
            fn.delay(delay, this, args);
        } else {
            try {
                fn.apply(this, args);
            } catch (error){
                (function(){ throw error; }).delay(0);
            }
        }
    }, this);
    return this;
});

Obvious problem with the above is, if people actually depend on fireEvent throwing an exception if a handler messes up, it won't anymore. The Error object should still have the original call stack stored, but it would be thrown asynchronously (to not disrupt normal event handling).

Owner

timwienk commented Nov 11, 2014

Regarding your comment on the first point: this is about the Events Class mixin, not Element.Events. A possible performance issue regarding mousemove doesn't affect the Events class mixin as far as I'm aware. (Also, Element.Events uses native events, it should already be asynchronous.)

Example regarding wrapping in a try-catch:
http://jsfiddle.net/358jjnsf/

fireEvent: function(type, args, delay){
    type = removeOn(type);
    var events = this.$events[type];
    if (!events) return this;
    args = Array.from(args);
    events.each(function(fn){
        if (delay){
            fn.delay(delay, this, args);
        } else {
            try {
                fn.apply(this, args);
            } catch (error){
                (function(){ throw error; }).delay(0);
            }
        }
    }, this);
    return this;
});

Obvious problem with the above is, if people actually depend on fireEvent throwing an exception if a handler messes up, it won't anymore. The Error object should still have the original call stack stored, but it would be thrown asynchronously (to not disrupt normal event handling).

@kentaromiura

This comment has been minimized.

Show comment Hide comment
@kentaromiura

kentaromiura Nov 11, 2014

Member

If we want to break b/c I'd rather implement proper CustomEvents instead,
see @WebReflection https://github.com/WebReflection/dom4/blob/master/src/dom4.js#L230-L282 as an example of what I mean.

Besides, in the link @arian provided Dean Edwards show how to make that work in oldIE as well, so if we have to keep IE8 compatibility we can do a workaround like that onpropertychange (with all the restriction property change has in ie8)

to keep B/C we can't really use try catch as any code using try catch to get (for example) any possible window.external exception will not work if we defer the re-throw of the exception.

Member

kentaromiura commented Nov 11, 2014

If we want to break b/c I'd rather implement proper CustomEvents instead,
see @WebReflection https://github.com/WebReflection/dom4/blob/master/src/dom4.js#L230-L282 as an example of what I mean.

Besides, in the link @arian provided Dean Edwards show how to make that work in oldIE as well, so if we have to keep IE8 compatibility we can do a workaround like that onpropertychange (with all the restriction property change has in ie8)

to keep B/C we can't really use try catch as any code using try catch to get (for example) any possible window.external exception will not work if we defer the re-throw of the exception.

@timwienk

This comment has been minimized.

Show comment Hide comment
@timwienk

timwienk Nov 11, 2014

Owner

CustomEvent relies on a DOM implementation, doesn't it? And thus makes the Events Class mixin depend on a browser or a non-native DOM implementation.

Owner

timwienk commented Nov 11, 2014

CustomEvent relies on a DOM implementation, doesn't it? And thus makes the Events Class mixin depend on a browser or a non-native DOM implementation.

@ibolmo ibolmo added wontfix and removed bug labels Jan 14, 2015

@ibolmo ibolmo closed this Jan 14, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment