Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handled rejected promises are claimed as unhandled in some cases #238

Closed
scf2k opened this issue Mar 20, 2013 · 20 comments
Closed

Handled rejected promises are claimed as unhandled in some cases #238

scf2k opened this issue Mar 20, 2013 · 20 comments
Milestone

Comments

@scf2k
Copy link

scf2k commented Mar 20, 2013

Handled rejected promises are claimed as unhandled in some cases. Can be reproduced with example like this:

var QFS = require('q-io/fs');

QFS.lastModified('/nonexistent')
.fail(function(err) {
    console.log('error caught!');
});

This code will generate a warning on application exist. Seems to be related directly to Q, not Q-IO.

@arikon
Copy link

arikon commented Mar 27, 2013

@domenic Have a look at this, please. It blocks us from moving to q 0.9.x.

@domenic
Copy link
Collaborator

domenic commented Mar 27, 2013

Is this Node 0.10-only?

I don't have much time this week, but maybe next...

@scf2k
Copy link
Author

scf2k commented Mar 27, 2013

@domenic no, it's not node dependant.

@domenic
Copy link
Collaborator

domenic commented Apr 7, 2013

This seems to be q-io related, since the following does not cause the problem:

var Q = require('q');

Q.reject("boo!")
.fail(function(err) {
    console.log('error caught!');
});

investigating...

@domenic
Copy link
Collaborator

domenic commented Apr 7, 2013

Nope, it's Q's problem. This triggers it:

var Q = require("q");

Q.reject("foo").get("bar").fail(function (err) {
    console.log("error caught!", err);
});

domenic added a commit that referenced this issue Apr 7, 2013
Introduces `Q.resetUnhandledRejections`, which is necessary for testing. Setting `Q.unhandledReasons.length = 0` like I was doing is dangerous, since it makes the `unhandledReasons` and `unhandledRejections` get out of sync.

Re: #238.
@domenic
Copy link
Collaborator

domenic commented Apr 8, 2013

@kriskowal, I'm at my wits end on how to fix this one. I added a failing test in a branch, as above, but am not sure how to approach it myself.

@mikerobe
Copy link

mikerobe commented Apr 8, 2013

This also causes spurious unhandled rejects:

q = require 'q'
defer = q.defer()
promise = defer.promise
promise.fail (err) ->
  console.log err

# removing this fin makes it work properly
promise.fin ->
  console.log 'fin'
defer.reject(new Error())

It may be a good idea to roll back the feature on the main releases until it's been thought out a little better (to avoid wrong and confusing errors)

@domenic
Copy link
Collaborator

domenic commented Apr 8, 2013

@mikerobe, just as a point of etiquette, it is very rude to post bug reports in CoffeeScript to a JavaScript repo.

@mikerobe
Copy link

mikerobe commented Apr 8, 2013

@domenic :) that's a good one

@tsgautier
Copy link

This is easily reproducible:

put this in a file called test.js:

var q = require('q');

var deferred = q.defer();
deferred.promise.then(function(e) { console.log("then", e); });
deferred.promise.fail(function(e) { console.log(e); });
deferred.reject('rejected');

run it:

$ node test

rejected
Unhandled rejected promise (no stack): rejected

@debug-ito
Copy link

This issue seems reproducible in Web browsers since the test at
https://rawgithub.com/kriskowal/q/master/spec/q-spec.html
emits the following error message to the console.

[Q] Unhandled rejection reasons (should be empty): []

I tried Firefox 20.0 and Chromium 25.0.1364.160 (both on Ubuntu 12.04), and both of them emit the message.

@satazor
Copy link

satazor commented Apr 21, 2013

I've also encountered this bug, but it doesn't happen every time when running the same code multiple times.
Is there any way to disable this feature? I would prefer to disable it until this bug got fixed.

@Myztiq
Copy link

Myztiq commented Apr 23, 2013

It would be nice to be able to disable this tracking via a configuration, or set a max size/count for the array to prevent memory being consumed on a production machine. Especially if that production machine does not "crash" or get restarted on a regular basis.

@domenic
Copy link
Collaborator

domenic commented Apr 23, 2013

An API for disabling this is planned; see #265.

@Myztiq
Copy link

Myztiq commented Apr 23, 2013

Thanks @domenic, I missed that issue.

@arikon
Copy link

arikon commented Apr 30, 2013

@domenic @kriskowal Any progress on fixing this issue? It still blocks us from migration to q 0.9.x.

@kriskowal
Copy link
Owner

We might have to amputate.

SLaks added a commit to SLaks/q that referenced this issue May 9, 2013
The test was also broken; it needs to check for emptiness after fail()
runs.

Fixes kriskowal#238
domenic added a commit that referenced this issue May 10, 2013
Introduces `Q.resetUnhandledRejections`, which is necessary for testing. Setting `Q.unhandledReasons.length = 0` like I was doing is dangerous, since it makes the `unhandledReasons` and `unhandledRejections` get out of sync.

Re: #238.
@TrevorBurnham
Copy link

I'm wondering why this was closed... surely it's a bug that attaching additional success handlers to a promise causes additional entries to be added to unhandledReasons/unhandledExceptions? Here's a test case:

deferred = Q.defer();
deferred.reject(new Error('foo'));
deferred.promise.fail(function() {
  console.log(Q.getUnhandledReasons().length);  // 0
});
deferred.promise.then(function() {  /* ... */ });
deferred.promise.fail(function() {
  console.log(Q.getUnhandledReasons().length);  // 1
});

It's perfectly valid to attach success handlers to a promise separately from failure handlers, and should not complicate debugging. N'est pas?

@domenic
Copy link
Collaborator

domenic commented May 30, 2014

Remember that .then and .fail create new promises, which are unhandled if the original one is unhandled.

On May 30, 2014, at 17:45, "Trevor Burnham" <notifications@github.commailto:notifications@github.com> wrote:

I'm wondering why this was closed... surely it's a bug that attaching additional success handlers to a promise causes additional entries to be added to unhandledReasons/unhandledExceptions? Here's a test case:

deferred = Q.defer();
deferred.reject(new Error('foo'));
deferred.promise.fail(function() {
console.log(Q.getUnhandledReasons().length); // 0
});
deferred.promise.then(function() { /* ... */ });
deferred.promise.fail(function() {
console.log(Q.getUnhandledReasons().length); // 1
});

It's perfectly valid to attach more success handlers than failure handlers to a promise, and should not complicate debugging. N'est pas?

Reply to this email directly or view it on GitHubhttps://github.com//issues/238#issuecomment-44703133.

@TrevorBurnham
Copy link

Yes, I was just realizing that. Not a bug, but a conceptual choice. Thanks for taking the time to clarify.

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

Successfully merging a pull request may close this issue.

10 participants