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

Add unhandled rejection tracking #643

Merged
merged 7 commits into from
Apr 26, 2015
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 63 additions & 30 deletions q.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ var nextTick =(function () {
var flushing = false;
var requestTick = void 0;
var isNodeJS = false;
var laterQueue = []; // queue for late tasks
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


function flush() {
/* jshint loopfunc: true */
Expand All @@ -105,43 +106,50 @@ var nextTick =(function () {
head.domain = void 0;
domain.enter();
}
runSingle(task);

try {
task();

} catch (e) {
if (isNodeJS) {
// In node, uncaught exceptions are considered fatal errors.
// Re-throw them synchronously to interrupt flushing!

// Ensure continuation if the uncaught exception is suppressed
// listening "uncaughtException" events (as domains does).
// Continue in next event to avoid tick recursion.
if (domain) {
domain.exit();
}
setTimeout(flush, 0);
if (domain) {
domain.enter();
}
}
while(laterQueue.length){
var fn = laterQueue.pop();
runSingle(fn);
}
flushing = false;
}

throw e;
function runSingle(task){ // runs a single function in the async queue
try {
task();

} else {
// In browsers, uncaught exceptions are not fatal.
// Re-throw them asynchronously to avoid slow-downs.
setTimeout(function() {
throw e;
}, 0);
} catch (e) {
if (isNodeJS) {
// In node, uncaught exceptions are considered fatal errors.
// Re-throw them synchronously to interrupt flushing!

// Ensure continuation if the uncaught exception is suppressed
// listening "uncaughtException" events (as domains does).
// Continue in next event to avoid tick recursion.
if (domain) {
domain.exit();
}
setTimeout(flush, 0);
if (domain) {
domain.enter();
}
}

if (domain) {
domain.exit();
throw e;

} else {
// In browsers, uncaught exceptions are not fatal.
// Re-throw them asynchronously to avoid slow-downs.
setTimeout(function() {
throw e;
}, 0);
}
}

flushing = false;
if (domain) {
domain.exit();
}
}

nextTick = function (task) {
Expand Down Expand Up @@ -203,7 +211,13 @@ var nextTick =(function () {
setTimeout(flush, 0);
};
}

nextTick.runAfter = function(task){
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

laterQueue.push(task);
if (!flushing) {
flushing = true;
requestTick();
}
};
return nextTick;
})();

Expand Down Expand Up @@ -997,6 +1011,7 @@ Promise.prototype.isRejected = function () {
// shimmed environments, this would naturally be a `Set`.
var unhandledReasons = [];
var unhandledRejections = [];
var reportedUnhandledRejections = [];
var trackUnhandledRejections = true;

function resetUnhandledRejections() {
Expand All @@ -1012,6 +1027,14 @@ function trackRejection(promise, reason) {
if (!trackUnhandledRejections) {
return;
}
if(typeof process === "object" && typeof process.emit === "function"){
Q.nextTick.runAfter(function(){
if(array_indexOf(unhandledRejections, promise) !== -1){
process.emit("unhandledRejection", reason, promise);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to just have optional Q method hooks for browser support, like Q.onerror, or support a delegate object with method hooks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The process bits are part of the specification for node/io and work this way across promise libraries and soon native promises in io.js

Firing events for browsers here is something we can definitely wait with - I asked @domenic if he wanted it and he said he did. I can leave it just like that if you'd like.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ll stand with @domenic. Progress on this would be good.

reportedUnhandledRejections.push(promise);
}
});
}

unhandledRejections.push(promise);
if (reason && typeof reason.stack !== "undefined") {
Expand All @@ -1028,6 +1051,16 @@ function untrackRejection(promise) {

var at = array_indexOf(unhandledRejections, promise);
if (at !== -1) {
if(typeof process === "object" && typeof process.emit === "function"){
Q.nextTick.runAfter(function(){
var atReport = array_indexOf(reportedUnhandledRejections, promise);
if(atReport !== -1) {
process.emit("rejectionHandled", unhandledReasons[at], promise);
reportedUnhandledRejections.splice(atReport, 1);
}

});
}
unhandledRejections.splice(at, 1);
unhandledReasons.splice(at, 1);
}
Expand Down