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

process: nextTick should not be handled lazily #3860

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@yorkie
Member

yorkie commented Nov 16, 2015

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

@@ -20,6 +20,15 @@ process.nextTick(function() {
order.push('C');
});
try {

This comment has been minimized.

@cjihrig

cjihrig Nov 16, 2015

Contributor

You should use assert.throws() here.

src/node.js Outdated
@@ -489,6 +489,8 @@
// on the way out, don't bother. it won't get fired anyway.
if (process._exiting)
return;
if (typeof callback !== 'function')

This comment has been minimized.

@cjihrig

cjihrig Nov 16, 2015

Contributor

I would move this if above the previous if for a more consistent throwing experience.

src/node.js Outdated
@@ -489,6 +489,8 @@
// on the way out, don't bother. it won't get fired anyway.
if (process._exiting)
return;
if (typeof callback !== 'function')
throw new Error('callback is not a function');

This comment has been minimized.

@cjihrig

cjihrig Nov 16, 2015

Contributor

Can you make this a TypeError

@mscdex mscdex added the process label Nov 16, 2015

@mscdex

This comment has been minimized.

Contributor

mscdex commented Nov 16, 2015

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

@jasnell

This comment has been minimized.

Member

jasnell commented Nov 16, 2015

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

@trevnorris

This comment has been minimized.

Contributor

trevnorris commented Nov 16, 2015

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.

@jasnell

This comment has been minimized.

Member

jasnell commented Nov 16, 2015

@ronkorving

This comment has been minimized.

Contributor

ronkorving commented Nov 17, 2015

Nice improvement 👍

@yorkie

This comment has been minimized.

Member

yorkie commented Nov 17, 2015

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

@@ -20,6 +20,28 @@ process.nextTick(function() {
order.push('C');
});
assert.throws(

This comment has been minimized.

@mscdex

mscdex Nov 17, 2015

Contributor

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.

This comment has been minimized.

@mscdex

mscdex Nov 17, 2015

Contributor

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

@yorkie

This comment has been minimized.

Member

yorkie commented Nov 17, 2015

Fixed, just forgot to remove that lines, sorry.

@targos

This comment has been minimized.

Member

targos commented Nov 17, 2015

LGTM

@mscdex

This comment has been minimized.

Contributor

mscdex commented Nov 17, 2015

@yorkie

This comment has been minimized.

Member

yorkie commented Nov 17, 2015

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

@jasnell

This comment has been minimized.

Member

jasnell commented Nov 17, 2015

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

@yorkie

This comment has been minimized.

Member

yorkie commented Nov 17, 2015

Sure, I see.

@jasnell

This comment has been minimized.

Member

jasnell commented Nov 18, 2015

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

@mscdex

This comment has been minimized.

Contributor

mscdex commented Nov 18, 2015

LGTM

cjihrig added a commit that referenced this pull request Nov 18, 2015

process: throw on non-function to nextTick()
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>
@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Nov 18, 2015

Thanks! Landed in 72e3dd9

@cjihrig cjihrig closed this Nov 18, 2015

@yorkie yorkie deleted the yorkie:fix/nextTick branch Nov 18, 2015

@yorkie

This comment has been minimized.

Member

yorkie commented Jan 26, 2016

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?

@jasnell

This comment has been minimized.

Member

jasnell commented Jan 26, 2016

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Jan 26, 2016

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

@yorkie

This comment has been minimized.

Member

yorkie commented Jan 26, 2016

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

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Jan 26, 2016

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

@trevnorris

This comment has been minimized.

Contributor

trevnorris commented Jan 27, 2016

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

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Jan 27, 2016

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

@yorkie

This comment has been minimized.

Member

yorkie commented Jan 27, 2016

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

@jasnell jasnell referenced this pull request Mar 17, 2016

Closed

Planning for v6 #5766

@jasnell jasnell referenced this pull request Apr 19, 2016

Closed

What is new in v6? #6264

jasnell added a commit that referenced this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release
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).

jasnell added a commit that referenced this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release
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).

jasnell added a commit that referenced this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release
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).

jasnell added a commit that referenced this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release
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).

jasnell added a commit that referenced this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release
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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment