Does this cover all test cases?

[domenic]

In nodejs/node#758 we discovered a lot of edge cases introduced by the most naive logic. The current spec is more naive than not.

The test cases in https://github.com/nodejs/io.js/blob/master/test/parallel/test-promises-unhandled-rejections.js are comprehensive and (IMO) contain reasonable expectations. I think we should try to pass those, and if that requires tweaking the spec, do so.

[domenic]

So Blink has an implementation that passes all the tests. Now we want to ensure that the spec matches that implementation with regard to the fiddly stuff like queuing a task and timing during the event loop and similar.

• https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/bindings/core/v8/RejectedPromises.h
• https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp&sq=package:chromium&rcl=1434627063&l=207
• https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/web/WebKit.cpp&sq=package:chromium&rcl=1434627063&l=82

In particular it looks like instead of queuing a task to notify about rejected promises, there's a new step that occurs after the microtask checkpoint. That makes a lot of sense given the tests' requirements and how io.js implemented things.

[jeisinger]

yeah, that indeed introduces a new phase. not sure what to do about this

@annevk - opinions?

[annevk]

I think asking @bzbarsky and @Hixie is probably better. I don't feel strongly how this is organized I think. (Although I'll note that browsers seem very slow to converge on event loop behavior.)

[domenic]

The nice thing in Blink is we were already doing this. I heard Firefox was working on Chrome-esque unhandled rejection tracking, so I wonder if they also naturally ended up here...

[jeisinger]

@bzbarsky mind commenting on this one as well? Is it ok to introduce another face after microtask processing completes?

[bzbarsky]

Sorry for the lag here; the lack of cross-browser interop, or in fact clear specification between ES and webapps here makes thinking about this complicated.

First the simple thing: Gecko's current unhandled rejection tracking (not web-exposed yet) is fully async off a task. I believe this was what we'd agreed on back when we were discussing this setup in the abstract, because that gives people who might be calling then() or catch() on the promise async a chance to do that before we report it as an uncaught rejection.

Second, I'd like to understand the exact motivation for adding another phase to microtask checkpoints and the ensuing event loop complications. Specifically:

1. What is the actual proposed behavior? Is there a clear definition somewhere? For example, can the new phase trigger another microtask checkpoint, or is the whole thing guarded by the "performing a microtask checkpoint" flag, and in both cases what are the exact details of how things work?
2. Why is this behavior better than just doing things off a task, which doesn't have those complications? A brief glance at Implement unhandled rejection tracking nodejs/node#758 suggests the latter was at least suggested there, but I haven't been able to dig out the problems people had with it.

[jeisinger]

The proposed behavior is that at at after performing a checkpoint, but before processing any other task these events are fired. any microtasks scheduled during the events would get processed in the next checkpoint (i,e, firing the events itself doesn't create a new checkpoint)

I guess instead of firing a task right away, we could also schedule a task that runs the event handlers, however, deciding which events to fire has to happen at the end of the microtask checkpoint.

specifically, if a promise is rejected, but a handler is then attached during the microtask checkpoint, no events are triggered.

also, if in the unhandledrejection event a handler is attached, no rejectionhandled event is fired.

[domenic]

Why is this behavior better than just doing things off a task, which doesn't have those complications?

I was kind of hoping @jeisinger would try this and report the problems back, but it turns out we already were using this post-microtask-checkpoint place for the unhandled rejection reporting to the console.

But, we tried similar things in io.js, which ran into a few problems. I don't want to claim these will carry over to browsers since the event loop over there is a bit different. Here is a sampling though:

var p = Promise.reject()
setImmediate(function() {
  p.catch(function(){});
});

// vs.

setImmediate(function() {
  p.catch(function(){});
});
var p = Promise.reject()

// should give the same result

// Similarly:

var p = Promise.reject();
setImmediate(function() {
  Promise.resolve().then(function () {
    p.catch(function(){});
  });
});

// vs.

setImmediate(function() {
  Promise.resolve().then(function () {
    p.catch(function(){});
  });
});
var p = Promise.reject();

Worst of all, we encountered a case where

window.on("unhandledrejection", () => console.log("unhandledrejection"));

const a = Promise.reject(); // (1)

queueTask(() => {
  const b = Promise.reject();
  queueMicrotask(() => b.catch(() => {}));
});

// This logged "unhandledrejection" twice.
// If you commented out (1), it logged nothing at all (!?).
// The desired behavior is to log once! (i.e. log for a, and not for b).

[bzbarsky]

firing the events itself doesn't create a new checkpoint

Yes, but what about things the event handlers can do that would perform a microtask checkpoint normally? That sort of invasive change is what I'm not so keen on here.

however, deciding which events to fire has to happen at the end of the microtask checkpoint

Can you explain why?

specifically, if a promise is rejected, but a handler is then attached during the microtask checkpoint,
no events are triggered.

That's a basic assumption here, yes. But why do we need an event even if a handler is attached after the microtask checkpoint from a task?

also, if in the unhandledrejection event a handler is attached, no rejectionhandled event is fired.

That seems pretty weird to me when multiple event handlers exist; some of them will see an unhandledrejection event but no corresponding rejectionhandled event.

@domenic, thank you for the concrete examples. Let's consider the various possibilities and see how they affect your examples:

1. End of microtask checkpoint fires unhandled rejection events, whatever the details.
2. End of microtask checkpoint checks for promises that are rejected but unhandled, collects that list, posts a task to fire rejection events on those, when the task runs the events are just fired unconditionally, even if the promise rejection is handled at that point.
3. End of microtask checkpoint checks for promises that are rejected but unhandled, collects that list, posts a task to fire rejection events on those, when the task runs the events are fired only on those promises which are still unhandled.
4. End of microtask checkpoint checks for promises that are rejected but unhandled. If the list is not empty it posts a task to fire rejection evens on whatever promises are still rejected but unhandled when the task runs.
5. As 4, but only posts a task if there is no such task pending already.
6. When a promise is rejected it posts a task to fire an unhandled rejection event on itself if it's still unhandled when the task runs.
7. As 5, but do the task-posting when a promise is rejected, not at end of microtask checkpoint. So basically 6 with task coalescing.

Any other options to consider?

For your first example, I believe options 6 and 7 would fire the unhandledrejection event in the first case but not the second one, right? So we can stop considering those below, I guess, but please do tell me if I'm getting this wrong. Options 1 and 2 would fire the event in both cases. 3, 4, and 5 would not fire the event in either case.

For your second example, options 1 and 2 would fire the event in both cases. 3, 4, and 5 would not fire an event in either case.

For your third example, all of options 1-5 would fire the event for "a" but not for "b", as far as I can tell.

I personally somewhat prefer the behavior of options 3-5 on your first two examples. Of those, I think option 5 gives the generally sanest behavior...

[domenic]

So just to make sure I'm on the right page, all including 1-5 involve a new "end of microtask checkpoint" concept, right?

I'll dig in to the differences between them on Monday, hopefully writing tests that can distinguish between them all (if we don't have such tests already). Without having spent too much energy on it, 5 does sound pretty reasonable.

The current test cases and Blink implementation might be aligned more with 1 or 2... in particular they trigger unhandledrejection on the equivalent my second example above:

unhandled-rejections-browser-spec/tests/resources/promise-rejection-events.js

Lines 509 to 522 in 463179c

	async_test(function(t) {
	var e = new Error();
	var p;
	onUnhandledSucceed(t, e, function() { return p; });
	p = Promise.reject(e);
	postMessageTask(function() {
	Promise.resolve().then(function() {
	p.catch(function() {});
	});
	});
	}, 'delayed handling: postMessageTask after promise creation/rejection, plus promise microtasks, is too late to ' +
	'attach a rejection handler');

I am in general in favor of relaxing the tracking such that it fires for less potential "false positives", so if 3-5 accomplish that without doing anything else weird (e.g. asymmetry or something broken with the third test case), then that's definitely an improvement.

[bzbarsky]

involve a new "end of microtask checkpoint" concept, right?

Well, or just a new step in the microtask checkpoint algorithm, yes?

[domenic]

Yes, I guess that works too, and makes more sense. Cool.

[domenic]

I've updated the current spec to the simplest version I could think of, which is simply 1. Will work up a PR for something a bit more complicated next.

[domenic]

Hmm, got stuck. Working through @bzbarsky's proposals in a bit more detail:

// Distinguishing between 1 and 2:

Promise.reject();
window.onunhandledrejection = function () {
  console.log("onunhandledrejection");
}
postMessageTask(function () {
  console.log("postMessageTask");
});

// 1 logs onunhandledrejection, postMessageTask
// 2 logs postMessageTask, onunhandledrejection

// Distinguishing between 2 and 3:

const p = Promise.reject();
window.onunhandledrejection = function () {
  console.log("onunhandledrejection");
}
postMessageTask(function () {
  p.catch(function () {});
  console.log("postMessageTask");
});

// 2 logs postMessageTask, onunhandledrejection
// 3 logs postMessageTask

Distinguishing between 3 and 4 seems impossible, IIUC. 3 seems to enqueue useless tasks saying "notify about this empty list", but since those tasks do nothing, I cannot figure out how to observe them.

I'm similarly having trouble writing anything that would distinguish between 3/4 and 5, although it seems a bit more believable to me that you could find a difference...

If the difference between 3, 4, and 5 is not meant to be observable but just a spec- and implementation-level "conservation of posted tasks," that's fine too. Just want to make sure I'm not missing something that I could write a test for.

[bzbarsky]

the simplest version I could think of, which is simply 1

Note that my version of 1 did not have sufficient detail to be able to reason about it at all. I think in practice 1 is very hard to define in a sane way...

The difference between 3 and 4 is visible in this testcase, I believe:

var p1 = Promise.reject();
var p2;
setImmediate(function() { 
  p2 = Promise.reject(); })
  setImmediate(function f() { /* do something */ });
}

In 3, there will be a task posted to fire the event on p1 and a separate task for firing the event on p2. The function f will run between those event firings. In 4, there will be a task that fires events on both p1 and p2, then function f will run. Note that if f happens to p2.catch() then option 4 won't fire an event for p2 at all, while option 3 will.

I think the difference between 4 and 5 is probably not observable, but I'd have to think pretty carefully about whether one vs many tasks is an observable thing in the HTML event loop.

[domenic]

Note that my version of 1 did not have sufficient detail to be able to reason about it at all. I think in practice 1 is very hard to define in a sane way...

Can you let me know what you think of what's in the current spec? I think it's also what's in Blink, essentially.

[bzbarsky]

Can you let me know what you think of what's in the current spec?

Assuming it also patches step 2 of the microtask checkpoint algorithm to jump to the newly inserted step, right?

The question I have is what happens if one of those events does something that involves microtasks (e.g. mutates the DOM, or uses Promises). As the spec is written right now, this is happening while the "performing a microtask checkpoint" flag is set, so the microtasks won't run upon returning from the event handler, afaict.... and in fact won't run until the next microtask checkpoint. If I understand this stuff correctly (which is not guaranteed in this case!) if you do Promise.resolve().then(f) inside that event handler, f may not get called until after a task from the event loop has run and triggered another microtask checkpoint. Is that behavior intentional?

[domenic]

In 3, there will be a task posted to fire the event on p1 and a separate task for firing the event on p2. The function f will run between those event firings. In 4, there will be a task that fires events on both p1 and p2, then function f will run. Note that if f happens to p2.catch() then option 4 won't fire an event for p2 at all, while option 3 will.

I worked through this in detail and you are right. However, I am unsure that 4/5's behavior is that good now. Let me outline exactly what happens with 5:

addEventListener('unhandledrejection', function h(ev) {
  console.log(ev.promise);
});

var p1 = Promise.reject(); // (1)
var p2;
postMessageTask(function f() {
  p2 = Promise.reject();
  postMessageTask(function g() {
    console.log('nested task');
  });
});

// Will log p1, p2, 'nested task' under 5.

The sequence is:

1. p1 is created and unhandled.
2. Schedules a task to do f. Task queue is [f].
3. Microtask checkpoint happens. (No-op in this example.)
4. End of microtask checkpoint happens: it queues a task to notify about unhandled rejections. Task queue is [f, notifyAboutUnhandledRejections].
5. Event loop goes back to the top. We perform the f task (removing it from the queue):
   1. p2 is created and unhandled.
   2. Schedules a task to do g. Task queue is [notifyAboutUnhandledRejections, g].
6. Microtask checkpoint happens. (No-op.)
7. End of microtask checkpoint happens: since the "will notify about rejected promises flag" is set, nothing is done.
8. Event loop goes back to the top. We perform the notifyAboutUnhandledRejectionsTask (removing it from the queue):
   1. Both p1 and p2 are unhandled, so we fire both events now.
9. Microtask checkpoint happens. (No-op.)
10. End of microtask checkpoint happens. (No-op.)
11. Event loop goes back to the top. We perform the g task.

Here is the problem: if I comment out (1), the log becomes

'nested task', p2

whereas if I leave (1) on, the log order is

p1, p2, 'nested task'

This is the kind of "nonlocal affects" that I alluded to earlier, and it seems bad.

From this perspective I think 3 is better. Although, it introduces more book-keeping: you can no longer just post a task to notify about unhandled rejections, and instead end up having to post a task to notify about specific elements of the set, or maybe introduce a temporary set, or similar. Which is annoying. Which is why I would fall back to 1 as the simplest, despite it potentially generating more "false positives".

---

Assuming it also patches step 2 of the microtask checkpoint algorithm to jump to the newly inserted step, right?

Eeek yes, will update.

The question I have is what happens if one of those events does something that involves microtasks (e.g. mutates the DOM, or uses Promises). As the spec is written right now, this is happening while the "performing a microtask checkpoint" flag is set, so the microtasks won't run upon returning from the event handler, afaict.... and in fact won't run until the next microtask checkpoint. If I understand this stuff correctly (which is not guaranteed in this case!) if you do Promise.resolve().then(f) inside that event handler, f may not get called until after a task from the event loop has run and triggered another microtask checkpoint. Is that behavior intentional?

Hmm, that seems correct. And the behavior doesn't seem great. (Although it's not the end of the world.)

An easy fix would be to put the resetting of the "performing a microtask checkpoint" flag before this extra notify-about-unhandled-rejections step, which is more in line with what I understand Blink to be doing. Do you see a problem with that?

[bzbarsky]

Do you see a problem with that?

Yes, I do. The call to the event handler will perform another microtask checkpoint, causing precisely the sorts of weirdness the flag is supposed to prevent in terms of things possibly getting mis-ordered, no? In particular, say the unhandled rejection queue has two things in it. When we fire the event for the first one, if that itself causes an unhandled rejection that will then be notified before the second element of the original queue.

If we want to avoid non-local effects, then I agree that 4/5 are no good, and I think we should do 3.

[domenic]

I see. OK, I'll try to spec 3. Might involve some slight of hand regarding posting a task with parameters, but that is probably fine in HTML.

[bzbarsky]

Sure. Tasks in HTML basically close over whatever the heck they want to close over. As in, if you have a variable foo in an algorithm you can post a task that, when it runs, will operate on foo.

[domenic]

I have another question. The current spec synchronously fires rejectionhandled instead of queuing a task. This seems a bit unusual, although we are not in another thread, so I think it is allowed. But maybe it should queue a task anyway?

[bzbarsky]

It synchronously fires it from a task (if we do option 3), right? That seems OK to me.

[domenic]

Actuwasy i was concerned with rejectionhandled, which we haven't discussed yet. Probably should have opened a new issue.

[bzbarsky]

Oh, rejectionhandled... that should perhaps be fired off a task, yes.

[domenic]

#13 implemented our conclusions here \o/