process: nextTick should not be handled lazily by yorkie · Pull Request #3860 · nodejs/node · GitHub

I got this log from my production clusters:

node.js:430
        callback();
        ^
TypeError: callback is not a function
    at doNTCallback0 (node.js:430:9)
    at process._tickCallback (node.js:359:13)

The stack trace sucks us in some ways, right? Because we can't start debugging from the log, so after we check the type of the callback at first, the terminal will tell us:

node.js:493
        throw new Error('callback is not a function');
        ^
Error: callback is not a function
    at process.nextTick (node.js:493:15)
    at startup (/Users/yorkieliu/workspace/weflex/api/index.js:3:11)
    at Object.<anonymous> (/Users/yorkieliu/workspace/weflex/api/index.js:6:1)
    at Module._compile (module.js:423:26)
    at Object.Module._extensions..js (module.js:430:10)
    at Module.load (module.js:354:32)
    at Function.Module._load (module.js:311:12)
    at Function.Module.runMain (module.js:455:10)
    at startup (node.js:138:18)
    at node.js:976:3

The location /Users/yorkieliu/workspace/weflex/api/index.js:3:11 is in my source file, so I'm able to debug my program with this stack. Obviously, this is an enhancement on stack trace, isn't it :-)

/cc @trevnorris

You should use assert.throws() here.

It might be worth adding at least one other test that passes in a truthy value that is not a function, such as {}.

This is good, agree with needing additional tests and the use of TypeError. Marked semver-major because this changes error handling around key API.

I wonder about the ordering. Is it alright that nextTick() doesn't throw if the application is exiting, even though the callback won't be called? Hypothetical, but doesn't really matter.

Minus @cjihrig's comments and a green CI, LGTM.

CI Run: https://ci.nodejs.org/job/node-test-pull-request/751/

Nice improvement 👍

Fixed, thanks for reviews @cjihrig, and could someone run CI again if the new diff LGTM :-)

This assertion should be removed because having just process.nextTick (no parens/uninvoked) is never going to throw, it will always return the function itself.

If you want to test the "same" thing as process.nextTick(), you could add testNextTickWith().

Fixed, just forgot to remove that lines, sorry.

LGTM

CI one last time: https://ci.nodejs.org/job/node-test-pull-request/758/

@mscdex does this console look fine? it already cost over 30 minutes, something that I could help?

@yorkie the test looks good but there appears to be something hung up on the jenkins server. I don't believe it's related to this change, however. Given that this is a semver-major change, we should likely hold off a couple of days before landing, just to be on the safe side.

Sure, I see.

@nodejs/ctc ... would appreciate additional eyes on this one also.

LGTM

This commit causes process.nextTick() to throw when its callback argument is not a function. PR-URL: #3860 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Michaël Zasso <mic.besace@gmail.com>

Thanks! Landed in 72e3dd9

Hey Node.js team, I see this commit(72e3dd9) still not get landed in some specific version, is there some special reason make this not get landed yet?

/cc @Fishrock123

@yorkie it is labeled as semver-major. We haven't done a major version bump since this landed.

I see, got a comment(#3860 (comment)) from @jasnell, and do we have a plan about v6.x, the next major?

Tentatively, around April. See https://github.com/nodejs/LTS/blob/master/schedule.png for an approximation of the release schedule.

@cjihrig I'm confused. Don't all patches land in master and it simply isn't back ported if labeled semver-major?

@trevnorris yes. This landed in master already. I think the question is, when will it show up in a release.

Yea, I did ask the question just because the conversation is too long which causes don't see the labels(include semver-major). As @cjihrig did put a timeline of release plan above, I guess it's clear, however I'm still looking forward this patch could be landed ASAP.

BTW, How about the Node.js organization provide a service which let anyone would be able to get docker image or downloads of picked commits, that would be also good for me. lol (just thoughts, ignore me if i'm crazy)

Anyway, thank you guys for my question :-)

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * OS X * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7 [#6402](#6402). * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

process: nextTick should not be handled lazily

29d8578

cjihrig reviewed Nov 16, 2015
View reviewed changes

mscdex added the process Issues and PRs related to the process subsystem. label Nov 16, 2015

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 16, 2015

yorkie added 2 commits November 17, 2015 12:43

fix nits

250e4a1

fix nits

c3961db

mscdex reviewed Nov 17, 2015
View reviewed changes

fix nits

3a4b6af

cjihrig closed this Nov 18, 2015

yorkie deleted the fix/nextTick branch November 18, 2015 16:15

Fishrock123 mentioned this pull request Nov 26, 2015

deps: upgrade npm to 3.5.0 #4032

Closed

Fishrock123 mentioned this pull request Dec 15, 2015

v5.3.0 Release Proposal (Stable) #4281

Closed

jasnell mentioned this pull request Mar 17, 2016

Planning for v6 #5766

Closed

jasnell mentioned this pull request Apr 19, 2016

What is new in v6? #6264

Closed

jasnell mentioned this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release #6383

Closed

process: nextTick should not be handled lazily by yorkie · Pull Request #3860 · nodejs/node · GitHub

Uh oh!

Uh oh!

process: nextTick should not be handled lazily - #3860

process: nextTick should not be handled lazily#3860yorkie wants to merge 4 commits intonodejs:masternodejs/node:masterfrom yorkie:fix/nextTickyorkie/node:fix/nextTickCopy head branch name to clipboard

Conversation

yorkie commented Nov 16, 2015

Uh oh!

cjihrig Nov 16, 2015

Choose a reason for hiding this comment

Uh oh!

mscdex commented Nov 16, 2015

Uh oh!

jasnell commented Nov 16, 2015

Uh oh!

trevnorris commented Nov 16, 2015

Uh oh!

jasnell commented Nov 16, 2015

Uh oh!

ronkorving commented Nov 17, 2015

Uh oh!

yorkie commented Nov 17, 2015

Uh oh!

mscdex Nov 17, 2015

Choose a reason for hiding this comment

Uh oh!

mscdex Nov 17, 2015

Choose a reason for hiding this comment

Uh oh!

yorkie commented Nov 17, 2015

Uh oh!

targos commented Nov 17, 2015

Uh oh!

mscdex commented Nov 17, 2015

Uh oh!

yorkie commented Nov 17, 2015

Uh oh!

jasnell commented Nov 17, 2015

Uh oh!

yorkie commented Nov 17, 2015

Uh oh!

jasnell commented Nov 18, 2015

Uh oh!

mscdex commented Nov 18, 2015

Uh oh!

cjihrig commented Nov 18, 2015

Uh oh!

yorkie commented Jan 26, 2016

Uh oh!

jasnell commented Jan 26, 2016

Uh oh!

cjihrig commented Jan 26, 2016

Uh oh!

yorkie commented Jan 26, 2016

Uh oh!

cjihrig commented Jan 26, 2016

Uh oh!

trevnorris commented Jan 27, 2016

Uh oh!

cjihrig commented Jan 27, 2016

Uh oh!

yorkie commented Jan 27, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

process: nextTick should not be handled lazily#3860
yorkie wants to merge 4 commits into
nodejs:masterfrom
yorkie:fix/nextTick