doc: more use-cases for promise events

[domenic]

This adds an example of a semi-common promise pattern that could
result in false-positive 'unhandledRejection' events, to help motivate
why there is a dual 'rejectionHandled' event. I anticipate it being
helpful to users who encounter 'unhandledRejection' events that do not
stem from obvious typos such as the JSON.pasre example.

[rvagg]

lgtm, I like the suggestion to deal with it explicitly even if it is adding a noop catch

[domenic]

Just pushed another less interesting commit that cleans up the sample a few lines down.

[tellnes]

Why do you want to remove the code highlighting here?

[domenic]

For consistency with the rest of these two sections. The doc is highy inconsistent about how events are written, throughout, but at least the promise sections decided on 'string' not string or 'string'. This is the single exception.

[rvagg]

good point, I read this the wrong way, thinking he'd added it. It shouldn't be removed, this is a consistent style through the docs

[benjamingr]

We're going to have a lot of fun documenting things with ES6 template strings :)

[rvagg]

@domenic you should squash those two commits into one

[benjamingr]

I'll repeat what I said in the other issue - I don't think that showing and thus encouraging this pattern is a good idea. I find people using this pattern incorrectly all the time. I've seen eagerly performing io with that pattern cause issues from race conditions multiple times.

Moreover - in over a year that bluebird (687,511 downloads in the last month) had these events and defaulted to logging - we have not received a single complaint about false positives because of this sort of pattern.

[chrisdickinson]

Not a blocker for this PR, but it'd be nice to standardize / document our language around "turn of the event loop" / "next tick."

[rvagg]

I think "turn" is used here on purpose, it's not a "next tick" in the process.nextTick() sense from what I understand

[chrisdickinson]

Yeah – Promises execute out of the microtask queue, if I'm not mistaken, which are related to the execution of nextTick, which interleaves microtask runs with nextTick callbacks scheduled inside nextTicks... so we get into a foggy area with regards to what a "turn" represents. The thrust of it is clear, but the specifics get hairy; it might be good to document what we mean when we use the general term in terms of the specific actions someplace.

In any case, I might have just talked myself into writing a PR for timers.markdown (with apologies to @trevnorris). :)

edit: and my above summary is still slightly incorrect! It goes "tick", ("nextTick", "microtask" )*, I think.

[benjamingr]

The reason I think "turn" is good terminology here is that most developers don't know what a microtask or a macrotask is but understand "event loop turn". As the person who used this terminology in the original docs PR I think "turn" is the way to go here - that said a clarification page in the documentation on all that would be nice.

[vkurchatkin]

"until next tick" would be more accurate

[benjamingr]

@vkurchatkin more accurate, but much less approachable to users who don't really understand the order the even loop is spun in io.js. Just two months ago I wasn't too aware of it myself and I've been using node for over three years at that point and have used multiple concurrency libraries and promises for at least a year and a half of that.

[vkurchatkin]

This is a valid issue, but not reason to give inaccurate information. in fact "ticks" have nothing do with event loop at all

[benjamingr]

I see your point. Maybe inaccurate but approachable is better than unapproachable?

That said if you think there is better terminology for this that developers will understand I'm all for it.

[vkurchatkin]

I disagree. In this case it's really closer to "wrong" than "inaccurate".

[benjamingr]

I think the solution is to create a document on io's async model that explains what a microtask is, a macrotask is and a queue of tasks is. Otherwise I really can't see how we can expect people to understand this terminology.

[chrisdickinson]

LGTM.

[benjamingr]

Just to clarify - I find this information useful. I just don't think it should go into the process API docs. I discussed a promises.markdown page with @Fishrock123 and opened an issue - a suggestion was raised to expand what the docs cover (perhaps in another location than /api). I think @domenic's example would be excellent there.

[domenic]

Squashed. Kept the removal of backticks since as explained backticks are not used elsewhere in these sections.

[tellnes]

LGTM, except I do not agree on the backticks. But that should probably be addressed in its own issue and in which case be updated everywhere.

[petkaantonov]

I think "avoiding false positives" is misleading, this doesn't eliminate false positives, in both cases of logging periodically/on process exit there can be false positives.

[benjamingr]

@petkaantonov the periodically comment is spot on - although if it's on process exit there can't be a false positive.

[petkaantonov]

@benjamingr why not? Process can exit for many other reasons than there not being anything to be done. For instance when an error is thrown and not caught.

[benjamingr]

It's misleading but it's not a false positive. Since the process has terminated nothing else can handle the rejection so it's just a positive. It's still problematic in that it's not useful in this case.

[petkaantonov]

That's not a useful definition for false positive.

This code path doesn't have any bugs regarding to not handling errors, and it would cause the server logs to be filled with false positives regardless if you use periodic logging or logging-on-exit:

function doIt(request) {
  var a = Promise.reject(new Error());
  setTimeout(function() {
    a.catch(function(e) {
      request.sendError(e.toString());
    });
  }, 10);
}

server.onRequest(doIt);

[petkaantonov]

I mean sure, that specific instance of rejection will technically not be handled by the code since the process was exited but the whole point is to surface the programmer errors of not handling errors in a some code paths. In that, much more useful sense, logging-on-exit will cause false positives.

[benjamingr]

That last comment clarifies it - I didn't understand you were talking about code paths and not specific rejections.

[domenic]

This would avoid the false positive seen in the above resource example.

This isn't a panacea for all false positives. This is an example of one solution for the example given, meant to illustrate the purpose of the 'rejectionHandled' event.

[Fishrock123]

I don't understand how this avoids the "false positive" error. By not crashing when there is an error?

[domenic]

By not logging errors that are not developer errors, but are instead a system being in an errored state but eventually transitioning out of it.

[benjamingr]

The other thing that bothers me in this PR and my own earlier one is a scenario like this:

Alex is using io.js 1.3- or NodeJS 0.12 or even a userland promise library like when or bluebird and has code that looks like this:

function doImportantStuff(){
    analytics.report("I did important stuff");
    return someResultOrPromise;
}

Here, analytics.report returns a promise - but its result is not observed since Alex wants the important operation to continue normally even if reporting to the analytics failed. One can argue Alex's analytics.report API should never reject if it is meant to be used like this and I probably agree but people are writing these sort of APIs often.

Now, Alex is fine at dandy at Company Inc. writing backend services and he goes and reads the new docs being a disciplined developer where he sees the suggestion so Alex copy-pastes the code from the docs:

var unhandledRejections = new Set();
process.on('unhandledRejection', function(reason, p) {
  unhandledRejections.add(p);
});
process.on('rejectionHandled', function(p) {
  unhandledRejections.delete(p);
});

Alex now has a memory leak. The server will crash and it will do so slowly. Should this be addressed or warned against?

[domenic]

@rvagg @chrisdickinson @tellnes I'm not actually a collaborator so I need one of you to do the merge :)

[petkaantonov]

Well, there were 2 pretty important issues brought up after the LGTM's, which are still unaddressed.

[domenic]

I disagree with your assessment on both of them.

[benjamingr]

@domenic well, it would be nice if you explained your disagreement. I think feedback could be helpful here and am genuinely interested in why you think these (false positives and leak) are non-issues.

[tellnes]

This is a topic which have had disagreements earlier, so I think we should at least wait 24 hours before we merge this.

[rvagg]

'Zactly, this isn't mergable while there are unresolved concerns. Particularly as a non-expert on promises I'm not willing to step in while @petkaantonov and anyone else being vocal on here is satisfied. If compromises are necessary then they should be made.

[petkaantonov]

For the false positive case, either the claim should be changed or a strategy other than logging periodically or logging-on-exit should be suggested as these cannot satisfy the claim as shown.

As for the memory leak, an example that doesn't lead to memory leaks should be used or a warning added that the example will lead to memory leaks in certain usage patterns.

I am of course also open to any out of the box solutions that I didn't think of as well.

[chrisdickinson]

@benjamingr Re: the slow leak: a leak like that should show up in the development process (i.e., before it hits servers and crashes.) Further, I'm mostly set on the idea that if a user calls an async API and doesn't handle the callback (or .then the promise, as the case may be) all bets are off. Not handling the rejection in the proposed scenario is equivalent to something like fs.readFile(filename), as opposed to fs.readFile(filename, function nop() { }) – one is a program error, and the other explicitly ignores the returned error.

[benjamingr]

@chrisdickinson you raise a good point. From what I can tell there are two options:

• Not adding a handler to an I/O request is considered a program error - I tend to personally agree with that. However this is exactly what the example in this PR does - it makes an I/O request in the SomeResource constructor but does not add an error callback to it. This is what my initial comment here discusses - I don't think we should encourage this pattern in the docs. It's possible to add a .catch(function(){}) to the promise in the example similar to the nop in your example - keeping the exact same semantics but eliminating the unhandled rejection notice.
• Not adding a handler to an I/O request is not considered a program error - in which case the memory issue stands and the leak might not show up in development (when the analytics server is not under load) but will show up in production.

[Fishrock123]

Maybe this is a candidate for takeover by the new Docs WG? cc @chrisdickinson?

[Qard]

👍 Docs WG seems like a good place for this.

[Qard]

I'm going to say LGTM, the case of not handling a promise seems to me like a program error, but I want to get some additional stamps of approval before considering merging.

cc @chrisdickinson @petkaantonov Anyone care to give another yes/no?

[benjamingr]

The reason it wasn't merged 5 months ago is that Petka, me and a bunch of others feel "no" about it. See the extended discussion above.

[domenic]

And I've already stated that I find those concerns outweighed by the fact that the documentation as-is is misleading, incomplete, and biased. If there are concerns about the wording here, I would suggest this be merged ASAP to correct the problems with the existing text, and then follow-up tweaks PRed by those with concerns.

[Qard]

Yes, I read through the thread before posting that. Near the end of the thread I saw @chrisdickinson comment on not handling a promise being program error, which I agree with, and your response was ambiguous. No one else said anything further, so I wanted to resurrect the discussion and see if any viewpoints had changed, or if there was new ideas to push this forward.

[Fishrock123]

I'm going to close on @Qard's note.

[domenic]

This is emphatically not opinion-based. This is showing how to use a tool that is in the platform. We should either merge this or remove the sections on those events entirely.

[benjamingr]

This is definitely opinion based. It is showing a very opinionated (and IMO wrong) way to use the tool in the platform.

The current sections provide informative and useful information and does not express the technique. The discussion here has been exhausted for months with a consensus (with only you disagreeing) to not merge this at its current form.

Maybe blog about it?

[domenic]

The current sections provide informative and useful information and does not express the technique

This is just false. We must remove the current section if this pull request is considered opinion based. The current section is showing a very opinionated (and IMO wrong) way to use the tool in the platform.

[domenic]

The discussion here has been exhausted for months with a consensus (with only you and @petkaantonov disagreeing) that this adds valuable information.

[benjamingr]

Well, then you can start by addressing the two issues @petkaantonov raised of the memory leak and the false positives. There is also the issue that keeping a list will still fail if there are unhandled rejections when the process ends (where it will log false positives).

The current docs just explain the events - they don't offer any opinion about their usage. If you feel like editing that to something you feel is less opinionated I'm all ears.

[domenic]

Well, then you can start by addressing the two issues @petkaantonov raised of the memory leak and the false positives.

I have already done so numerous times on this thread.

The current docs just explain the events - they don't offer any opinion about their usage.

Why do you keep saying this? You're so blinded by your own perspective that you cannot see that it is biased?

[Fishrock123]

Ok, can we please leave any STRONG feelings about this civil? Otherwise I'm going to just lock it.

Going to review more.

[Fishrock123]

@domenic why is this likely a false positive?

[domenic]

Good question. Perhaps the better phrasing would be "To deal with cases like this, which you likely don't want to track as developer error in the same way as other 'unhandledRejection' events"...

[Fishrock123]

You don't? They all seem the same to me.

What is the other kind? Wouldn't it just be a missing catch somewhere too?

[domenic]

In the example, the loaded promise is initially in the errored state, since no resource has been loaded. This isn't a developer error; it's just part of the system. If the developer never loaded anything into the resource, that would be an error. But the transient errored state is not something that should be logged.

[benjamingr]

@Fishrock123 the biggest point for unhandled rejection hooks is to un-silence developer errors (but not bug you too much about platform errors). So a developer error like:

JSON.prase('"note it's prase and not parse"');

Don't get turned into a silent rejection but are somehow obtainable. On the other hand - program errors (like a network stream not opening) might (this is an opinion) be silenced since if you did not .catch them you might not care about handling the error (like calling a node method without a callback arg, or ignoring err).

[Fishrock123]

the biggest point for unhandled rejection hooks is to un-silence developer errors (but not bug you too much about platform errors).

It is? I'm not so sure about that, I would think everything else should be explicitly .caught().

[benjamingr]

@Fishrock123 Thanks. Don't worry about it, me and Domenic go a while back - there is no beef between us, just a disagreement about something - you don't have to be concerned for me here.

@domenic

Why do you keep saying this?

Because they do not express an opinion about how the events should be used.

The pattern is also not "semi common" - I have gone through every single usage of unhandled rejection hooks on GitHub when I wrote the unhandled rejection tracking proposal and I did not see a single use.

I have already done so numerous times on this thread.

Please, do show me where. Here are the issues again:

• False Positives - If you want to ever access the set of promises to (for example) print them (to actually use the debugging information for anything) you can get false positives (things that were still not handled but will be). So this approach doesn't actually do anything in order to help with false positives.
• Memory Leak - If you add calls to the set regardless (for example analytics/tracing that should not crash the app) you get a memory leak - and this set of rejected promises suddenly crashes your server on non-critical calls failing.

This usage of the hooks is super-opinionated and frankly you're the only one I've ever seen talk about using it.

[Qard]

This is more patterns territory and less explicit reference, which leads me to believe this is probably more suitable as a topic page on debugging promises, or something like that.

I think the approach is absolutely valid, and I'd love to see it documented. I'm just not sure reference is quite the right place for it. I'm a bit uncertain on that though.

@nodejs/documentation Thoughts?

[Fishrock123]

@Fishrock123 Thanks. Don't worry about it, me and Domenic go a while back - there is no beef between us, just a disagreement about something - you don't have to be concerned for me here.

I'm not sure this matters, it's not acceptable either way. We need to make sure others feel comfortable chiming in also and I can guarantee that by now they don't.

This is more patterns territory and less explicit reference, which leads me to believe this is probably more suitable as a topic page on debugging promises, or something like that.

I think the approach is absolutely valid, and I'd love to see it documented. I'm just not sure reference is quite the right place for it. I'm a bit uncertain on that though.

I would tend to agree, but I'll let my questions be answered first.

[domenic]

This is more patterns territory and less explicit reference, which leads me to believe this is probably more suitable as a topic page on debugging promises, or something like that.

I don't think that's really true. It's showing how to use one of the two events; currently only one is documented. And the documentation for that one event is already showing a "pattern". That is why I am saying we should remove all documentation if we think this is not appropriate.

[Fishrock123]

Is the debate actually around whether or not this is a good pattern?

At second look this looks unusual to me, I would expect it to wait until there is something or a timeout, I think?

[domenic]

That's perhaps part of it. But this pattern of using promises to signal state transitions is reasonably common; see http://www.w3.org/2001/tag/doc/promises-guide#state-transitions

[benjamingr]

This is very unusual to me too. I tend to agree, especially since people who subscribe to loaded should be able to get a hook on when it happened.

Better yet - the construction should not pretend to perform IO.

I do think that we can find an example of an operational (not programmer) error happening with promises without it to document something that causes unhandledRejection and requires adding a .catch - for example reporting a non-critical event is pretty common, or sending an email in the background and not caring if it succeeded.

[benjamingr]

If we warn about the two issues in the PR (that the Set can leak, and that you still have false positives) I'm more OK with it (although I definitely think we should wait for @petkaantonov to weigh in here).

[domenic]

Shit, I rebased on master whereas this targets 1.x. Opening a new PR.