search

cujojs / when

A solid, fast Promises/A+ and when() implementation, plus other async goodies.
Other
3.44k stars 396 forks source link

Should successful .finally(handler) suppress 'unhandled rejections' message? #357

Closed maciasello closed 10 years ago

maciasello commented 10 years ago

when.reject(5).finally(function() {}); does not produce the unhandled rejections log when.reject(5).finally(function() {throw new Error();}); does produce it

As promise1.finally(function() {}) still rejects when promise1 is rejected the log should be there if nothing attaches and handles the real error (not the cleanup as for finally).

Ideas, comments, WAD?

stefanpenner commented 10 years ago

Interesting, I would expect it not suppress... I believe RSVP.js would also suffer from this.

briancavalier commented 10 years ago

@maciasello yep, sounds like a bug. Looking into it now.

briancavalier commented 10 years ago

@maciasello I just pushed a fix to this branch. Could you give a try to see if it works for you? Thanks!

maciasello commented 10 years ago

@briancavalier I tried a few simple commands in the console and played for a while with my components that use .finally(). For this set it works correctly for the reported issue, but I started observing that when.reject(5).finally(function() {throw new Error();}); adds two unhandled rejections (originally one): the original promise and the one thrown from finally handler. This one for me is far from obvious how it should behave.

briancavalier commented 10 years ago

@maciasello Thanks for trying it, and good catch. That case should also only report 1 unhandled rejection, too. I'll add a unit test for that and refactor a bit more.

briancavalier commented 10 years ago

@maciasello Ok, b3eb15a fixes the extra unhandled rejection (which turned out to be introduced by my last change). I think we might have a winner, but def would appreciate your taking a look. Thanks!

briancavalier commented 10 years ago

... and c20481b improves the tests a bit by verifying the actual rejection reasons, in addition to the count.

maciasello commented 10 years ago

@briancavalier I've tested the latest version from the branch and it works like a charm! Thanks for the fast reaction. What is a predicted schedule of a patch release?

briancavalier commented 10 years ago

@maciasello Great, thanks for verifying. I'm planning to release it today, although it may be later in the evening EST. I'll close this issue, and will merge the changes as a part of #358

briancavalier commented 10 years ago

@maciasello Just released 3.4.5 that includes this fix :) Thanks again for reporting the issue.