doc: clarify process.nextTick vs. setImmediate (was: Fixes #6718)

[davepacheco]

This documentation change clarifies the potential issue with process.nextTick and steers users towards setImmediate instead for arbitrary callbacks.

[Nodejs-Jenkins]

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

Commit davepacheco/node@acb4bb4 has the following error(s):

• Commit message must indicate the subsystem this commit changes

The following commiters were not found in the CLA:

• Dave Pacheco

You can fix all these things without opening another issue.

Please see CONTRIBUTING.md for more information

[davepacheco]

I updated the commit comment.

[rlidwka]

This is normally once the current event loop turn runs to completion, but it may be much sooner if nextTick handlers are currently being processed. At the end of each tick the process.nextTick handlers are executed until there are none left.

Prize for meaningless.

"All process.nextTick callbacks will be processed in every event loop iteration" means the same thing, but 4 times shorter.

setImmediate is useful in developing APIs where you want to give the user the chance to assign event handlers after an object has been constructed, but before any I/O has occurred.

This one is actually wrong.

[davepacheco]

@rlidwka Thanks for the feedback. I've clarified the wording in the first case and fixed the second one (that was a copy/paste error). Better?

[davepacheco]

"All process.nextTick callbacks will be processed in every event loop iteration": It's not "all" callbacks. It's only the ones that were added since the tick started. (That's obvious if you already understand the interface, but documentation shouldn't assume that.)

Besides that, one significant source of confusion is that people don't seem to realize that by appending a handler from a handler extends the tick, and that that starves the regular event loop. It's a subtle consequence of how nextTick works, and that phrasing does nothing to bring attention to it.

[rlidwka]

That's just a first impression, I don't believe process.md needs to be updated at all, but I won't argue with someone who does.

setImmediate can't really be used instead of process.nextTick in this lovely case:

function iAmAsyncCauseIsaacsSaidSo(arg, cb) {
    var stream = fs.createReadStream('blahblah')
    process.nextTick(function() {
        cb(null, stream)
    })
}

But I'm not sure who in the right mind would do that, so I don't care very much.

[isaacs]

setImmediate can't really be used instead of process.nextTick in this lovely case:

Why not?

I mean, it's slower, sure, but as of node v0.10, no data will be lost since the stream won't flow until you read from it.

[rlidwka]

no data will be lost since the stream won't flow until you read from it

... and then someone adds on('data') handler forcing stream into a compatible mode thinking "well, I'll just duplicate this data here, what might go wrong?", and we'll have something that's very hard to debug...

upd: Well yes, I mostly use old mode for a few reasons. But you can change that line to any EventEmitter depending on IO, and it'll be the same thing. I pretty much learned that all event handlers should be attached in the same tick, and if they don't, something bad happens.

[isaacs]

... and then someone adds on('data') handler forcing stream into a compatible mode thinking "well, I'll just duplicate this data here, what might go wrong?", and we'll have something that's very hard to debug...

Someone who? You have to have given them access to the stream in the first place. Don't read from streams that aren't yours.

[rlidwka]

ok, I admit, you're right there

[jasnell]

Closing due to lack of activity. Can revisit if someone is interested in updating the PR.

[davepacheco]

This is disappointing. People are still confused by this. What work needs to be done to make this able to be integrated?

[jasnell]

Basically just someone willing to do the work. The last update on it was Dec 2013.

[davepacheco]

When I last updated this in 2013, I thought it was done. What's "the work"? Is it that it doesn't merge cleanly now?

[jasnell]

Generally, yes. (1) make sure the changes are still relevant (i.e. make sure that there haven't been other changes landed that make this irrelevant) and (2) make sure they still apply cleanly. Beyond that, I'd recommend also making sure that a similar change doesn't need to be landed on the io.js side of things so that when things merge there's less additional work to do.

[jasnell]

If you're willing to make sure it's updated, I'll reopen.

[davepacheco]

Yes, the changes are still relevant. The issue that this fixes is still open (#6718), and even has recent comments. People are clearly still confused by the existing documentation. And as far as I can tell, the documentation is in exactly the same state as when I submitted the PR: the text that I removed in the PR is still there, the text that I moved is still present in its old place, and nothing has been added to clarify things. (I'm not sure why it doesn't merge cleanly.)

Isn't it inappropriate to close this PR without having answered those questions first?

[jasnell]

@davepacheco as I said, I'm willing to reopen. The point to closing this is exactly to see if anyone is willing to step up to make sure the works gets completed.

[jasnell]

New issue opened in nodejs/docs. Closing this here but still need to revisit.