Fix bug where EventBus handlers are removed before execution

[justinanastos]

Stop mutating handlers[type] in EventBus#off. Doing so prevents all handlers from being run if off() is called while executing a callback.

Fixes #1773

[dsparacio]

Seems like a great fix. just missed the release ;) Thanks for the commit.

[justinanastos]

@AkamaiDASH

Seems like a great fix. just missed the release ;) Thanks for the commit.

Darn! How often does your team release?

[dsparacio]

6 weeks or so. It will get merged this week into dev and nightly.

[dsparacio]

@justinanastos Looks like we have a regression here from last release that will hold up this release. I can either revert your change or you can try to address it.

[justinanastos]

Thanks for the heads up @AkamaiDASH , I'll look into the regression today.

[justinanastos]

I reviewed #1889 and it doesn't seem like this PR (#1744) caused a regression, but revealed a bug in the code elsewhere.

Previously, when the handler list was mutated, the handler would be removed and not called.

As far as I can understand, this is a mistake. calling off(type, listener, scope) should not be removing anything besides listener. #1889 is caused by expecting off to work incorrectly.

[davemevans]

#1858 is caused by expecting off to work incorrectly.

I think anyone calling off would be surprised to see that listener is still called in certain circumstances. I don't believe it is expecting off to work incorrectly to expect listener not to be called at all after a call to off.

Before this PR, calling off removed listener and it was no longer called, including during the current trigger iteration. After the merge, calling off removes the listener, but it will still be called, if it was registered after the listener ultimately calling off, if removed during the current trigger iteration. This seems like a regression to me?

Another approach might be to set handler[type][idx] = undefined in off, and check handler is defined in
the forEach in trigger, getHandlerIdx etc though that is not ideal as the handler list will grow and need to be cleaned up - see https://gist.github.com/bbcrddave/e314004600eadc6452fc124ab41c6905.

[justinanastos]

Looking again with fresh eyes, you are totally right and I was mistaken. I apologize for my hasty response, I didn't read #1889 correctly.

The bug I was trying to fix can be seen here: https://jsbin.com/haculos/2/edit?js,console

While traversing the array of handlers, if a handler removes the event listener for the current handler, the next handler will not be called.

A apparently didn't understand how Array.prototype.forEach worked well enough.

@bbcrddave I like your gist. I submitted PR #1891 with one variation: I used setTimeout(..., 0) to clean up the null handlers in off so we don't have to make trigger clean it up.

[dsparacio]

All agree we should hold the release until we sort this one out?