kriskowal / q

A promise library for JavaScript
MIT License
14.94k stars 1.21k forks source link

Misleading warning about unhandled failure #474

Closed cowwoc closed 10 years ago

cowwoc commented 10 years ago

q.js is always outputting [Q] Unhandled rejection reasons (should be empty) regardless of whether a failure handler is provided.

Here is my code:

    "use strict";
    return Q.try(function()
    {
        return AjaxWithRetry.ajax(
            {
                url: this.participants,
                type: "POST",
                data: JSON.stringify(
                    {
                        "user": user
                    }),
                contentType: "application/vnd.com.foo.user+json; version=1; charset=utf-8"
            }).
            then(function(result)
            {
                var settings = result.settings;
                var xhr = result.xhr;
                throw new ConflictingResourceException(settings.type, settings.url, xhr.responseText,
                    xhr.getResponseHeader("location"));
            },
                function(value)
                {
                    log.debug("0: " + value);
                    throw value;
                }).catch(function(value)
        {
            log.debug("0.1: " + value);
            throw value;
        });
    }.bind(this));

When I run this code, I see the following events:

[Q] Unhandled rejection reasons (should be empty): 
["(no stack) ConflictingResourceException: POST http…rticipant: http://localhost:8080/participants/85/"]
0.1: ConflictingResourceException: POST http://localhost:8080/calls/1/participants/ returned: Conflict with existing participant: http://localhost:8080/participants/85/ log4javascript.js:155

Meaning, Q complains about an unhandled failure, even though the failure handler is invoked a second later. In the above example AjaxWithRetry invokes JQuery's ajax() method, and returns a Promise.

petkaantonov commented 10 years ago

There is nothing to catch the error you throw after logging 0.1?

cowwoc commented 10 years ago

The code sniplet I pasted was incomplete. Something does catch the error after logging 0.1 but more to the point, note that Q logs the warning before the handler for 0.1 is invoked. That by itself indicates a bug.

duncanbrown commented 10 years ago

I think I'm also seeing this issue. Running the following results in a warning in the console saying 'Error: foo' is an unhandled rejection reason:

var d = Q.defer();
setTimeout(function () {
    d.resolve(1);
}, 1);
d.promise
    .then(function () {
        throw new Error('foo');
    })
    .catch(function (e) {
        return 1;
    })
    .done();

As I understand it, I have handled the Error by attaching the catch handler - am I mistaken?

mrwillis21 commented 10 years ago

I am seeing this issue as well, with a very simple test case:

Q.reject(new Error('error message')).done(
    function(result) {
        console.log(result);
    },
    function(error) {
        console.error(error);
    });

As you can see, I have an error handler in place. However, I still see the Unhandled warning before I get the error output in the console. I've tested this in Chrome 32 and Firefox 26.

kriskowal commented 10 years ago

I am tempted to remove the feature entirely, which I intended to do once there was a viable promise debugger. All of these problems amount to misunderstanding the log entry or consoles failing to update. As for the viable promise debugger, work is ongoing both in the Ember inspector and the Montage inspector.

cowwoc commented 10 years ago

@kriskowal

I assume there is some existing discussion about what "consoles failing to update" means? If so, please send us a link to read.

The feature itself is great, so if you can make it work then please do. As for Ember/Montage, it sounds like they require a lot more heavy lifting to integrate into our projects than q.js. I'm not looking for a framework, just a library.

kriskowal commented 10 years ago

@cowwoc My hope is that you will not have to buy into Ember or Montage to take advantage of their promise debuggers. This will require some coordination about what protocol Q will use communicate with them (over the window message port).

Regarding consoles that don’t update https://github.com/kriskowal/q/issues/151

The console message is sent once and immediately when a rejection is created, regardless of whether it is subsequently handled. If it is subsequently handled, the relevant error should be removed from the array and the console should update the live list. It very well might not, but if the browser fails to update, there is nothing we can do from within Q to improve the situation.

cowwoc commented 10 years ago

@kriskowal Just a thought, but couldn't you do the following instead?

  1. Q registers a default failure handler which prints out this warning.
  2. Error handlers registered by the users get inserted before Q's handler.

This way, you don't need to update consoles. The warning message is only ever printed out if there is a problem.

kriskowal commented 10 years ago

@cowwoc handlers can be added in future events. That an error is not handled before the event queue flushes is not a perfect indication that the event will never be handled. Though it is a decent heuristic in practice, if the console does not update, there will be false negatives.

duncanbrown commented 10 years ago

Thanks for the explanation @kriskowal.

In case it's helpful to anyone else: I have just noticed that, in Chrome, if you close and reopen Dev Tools it updates to show an empty array, as expected.

cowwoc commented 10 years ago

Are there any known open bug reports against Chrome and other browsers for these issues?

PS: I agree with the author of http://www.bennadel.com/blog/2408-Chrome-Dev-Tools-Live-Update-In-The-JavaScript-Console-Is-Confusing.htm ... It's not clear that "Live Update" is all that great for console output. It sounds to me like we need some event to be fired before an object is garbage collected or something so we can check its state then and append to the log if needed. The whole idea of "updating" a log seems wrong to me as well.

cowwoc commented 10 years ago

And according to http://stackoverflow.com/a/8249333/14731 this functionality was removed.

Kris, are you referring to this same Chrome feature or are we talking about something else?

kriskowal commented 10 years ago

@cowwoc Astute observations all around. We are indeed looking forward to post-mortem garbage collection hooks, perhaps in the ES7 timeline, though these have been discussed by TC39 for at least five years http://wiki.ecmascript.org/doku.php?id=strawman:weak_references.

I agree that the console should capture a snapshot.

This is my throw-away prototype for a Chrome Extension that adds a Promise panel https://github.com/montagejs/continuum

This is the issue pertaining to the development of the Promise tab in the Ember inspector https://github.com/tildeio/ember-extension/pull/76

kriskowal commented 10 years ago

@cowwoc Thanks for the tip on the WebKit behavior. If it snapshots when opened, it’s useless for this purpose. We need to remove the feature outright and hope for postmortem GC or a promise inspector. At this point, the log is completely misleading.

kriskowal commented 10 years ago

@domenic I am tempted to remove all unhandled rejection tracking entirely. Do you want to keep the interface in the v1 train and remove it entirely in v2?

domenic commented 10 years ago

@kriskowal I feel there is still a decent bit of use to it. Perhaps a minimal interface like:

Q.onPossiblyUnhandledRejection(function (reason) { ... })
Q.onRejectionHandled(function (reason) { ... })

We could rip it out of v2 for now though and consider adding those back later.

kriskowal commented 10 years ago

@domenic I’m proposing that we remove the log in v1, retain the current interface, and perhaps add the interface you’re proposing as a temporary substitute. In the end, we need to implement an inspector protocol that provides those hooks.

cowwoc commented 10 years ago

I would suggest disabling the feature by default (in v1 and v2) while you experiment with making it more reliable. Users who are interested could enable the relevant flag.

petkaantonov commented 10 years ago

@kriskowal Note that the heuristic should be 'no handlers attached by the start of second turn' (so after queue has been completely flushed), not before as you say. So this way the only way to fool it is to use 3rd party asynchronous boundary in-between.

kriskowal commented 10 years ago

@petkaantonov That is a better but not a perfect heuristic.

var Queue = require("q/queue");
var queue = new Queue();
queue.put(Q.reject());
// … later
queue.get().catch(function () {
});
petkaantonov commented 10 years ago

I don't get it, you are attaching the handler synchronously right away.

kriskowal commented 10 years ago

The producer and consumer are running in separate events. Ergo, // … later.