Add unhandled rejection tracking

[benjamingr]

This pull request adds unhandled rejection tracking to Q via process and window events.

Already done:

• Fire process events.
• Ensure it runs after any handlers attached via Q microtasks like arbitrary then nesting.

Need to do:

• Fire window events

[kriskowal]

In general, this approach to creating another priority tier is barely enough to work. I’d much favor the approach of creating a task queue primitive that can be constructed in terms of the queue it uses to request flush. Such event queues could be stacked trivially. That would be a logical next step for asap, a make-asap(requestFlush).

[benjamingr]

In general I agree that two tiers are something that isn't enough - however in this particular case only the library calls runAfter.

While I agree a heap implementing a priority queue or some kind of chain of responsibility pattern is preferred I didn't want to bloat Q with too much code and wanted to add a single extra tier in the simplest way possible. I used an array instead of a linked list since it is much faster and less code to write.

[kriskowal]

You are right that minimum viable is appropriate for this. My intuition is that layering queues would compose better and be more maintainable in the long run. I am content to make v2 the long run branch though, if there’s a pragmatic solution here.

[benjamingr]

Yeah, this should definitely look differently in v2 :)

I'll try to amend this PR Friday to fire the browser hooks - let me know if
there is anything else you'd like me to change. If you feel like
implementing this yourself differently then you're of course also welcome.

On Wed, Feb 11, 2015 at 7:20 AM, Kris Kowal notifications@github.com
wrote:

In q.js #643 (comment):

@@ -91,6 +91,7 @@ var nextTick =(function () {
var flushing = false;
var requestTick = void 0;
var isNodeJS = false;

• var laterQueue = []; // queue for late tasks

You are right that minimum viable is appropriate for this. My intuition is
that layering queues would compose better and be more maintainable in the
long run. I am content to make v2 the long run branch though, if there’s a
pragmatic solution here.

—
Reply to this email directly or view it on GitHub
https://github.com/kriskowal/q/pull/643/files#r24477631.

[kriskowal]

Yeah, I don’t have much discretionary time anymore, and some of the hubris is wearing off too. Glad to accept your help on this one. Maybe I’ll get a chance to merge/test/release this on the weekend.

[benjamingr]

@kriskowal

I think we should skip DOM events for now - implementing it took at least 100 LoC to get right* and it will increase the library size by 5% for existing users for a use case they didn't know existed until now.

I've fixed formatting issues and tested in the following browsers:

• Safari 8 Mac
• Chrome 40 Mac
• Firefox 4 Mac
• Firefox latest Mac

I've also asked friends to test, @bananu7 reported:

• Chrome Windows (40)
• IE11 gets the same results.
• Old IE (Visual Studio IE) gets the same results.

Note that IE11 fails the following Q tests already:

unhandled rejection reporting reports a stack trace. Expected [ '(no stack) [object Error]' ] to equal [ undefined

Old IE reports the following:

computing sum of integers using promises should compute correct result without blowing stack. TypeError: Object doesn't support property or method 'reduce'

Neither are regressions. NodeJS works in version 0.8 0.10 and 0.12, io.js works in 1.2.

I think we're good to go. @kriskowal ?

• See bluebird's implementation here: https://github.com/petkaantonov/bluebird/blob/master/src/captured_trace.js#L436-L500 as well as in several other places referenced in the library.

[kriskowal]

This is sometimes called an idle event, and it’s fine as long as it does not effect a resume. I’ll take it as-is.

[kriskowal]

I have completed a review locally and am good to merge this. Would be great to get 👍 from @domenic. Would be even better if you would follow up with a regression test to ensure that future work does not break this new feature.

[benjamingr]

@kriskowal the current Q specification is all for browsers - I can port over Petka's tests for this feature in io.js native promises if you'd like but it would mean introducing a second way to run tests - your call.

[domenic]

I think it really should get tests. I'm not sure where the impression that the Q tests are for browsers came from? See e.g. the domain tests: https://github.com/kriskowal/q/blob/v1/spec/q-spec.js#L2319-L2413

[benjamingr]

Oh, I see - in that case I'll gladly write tests in.

[benjamingr]

I've written some tests but I have an issue. I'm trying to run the tests in the spec runner and for some reason:

        it("should report unhandled rejections", function() {
            var d = Q.defer();
            process.on("unhandledRejection", function(p, r) {
                d.resolve();
            });
            d.promise.then(function(){
                console.log("RESOLVED"); // This logs
            });

            Q.reject(new Error());
            return d.promise;
        });

This fails with:

1. unhandled rejection tracking should report unhandled rejections
   Message:
   Error: Timed out after 500 ms

However doing a return Q(); succeeds. I think it's something with process since if I resolve in a setTimeout it works.

Any ideas what this might be related to? If I copy Petka's spec from io.js and run it it runs.

[benjamingr]

@kriskowal

[benjamingr]

:(

[kriskowal]

Sorry for the delay. I have merged and published this as in v1.3.0. If you decide to follow-up with browser support for this feature with window events, or if you follow up with pending and settled events, take a moment to examine the prevailing style for white space in control flow blocks.

[benjamingr]

Awesome news!