lib: consistent callback checking across project #3539

MylesBorins · 2015-10-27T00:52:12Z

Throughout the codebase there are two patterns for checking if a
callback has been provided as an argument. This PR replaces all
instances to do explicit type checking.

Closes #3536

MylesBorins · 2015-10-27T00:57:30Z

lib/_http_outgoing.js

@@ -90,7 +90,7 @@ exports.OutgoingMessage = OutgoingMessage;

 OutgoingMessage.prototype.setTimeout = function(msecs, callback) {

-  if (callback) {
+  if (typeof callback === 'function') {


I had originally done this with a regex... and I am realizing now why you might want to check for callback... what if it doesn't exist :s

although if (typeof callback === 'function') will still evaluate false whether or not callback is defined.

Seem like the extra if statement here isn't neccessary

Trott · 2015-10-27T00:57:44Z

Likely semver-major, yeah?

cjihrig · 2015-10-27T01:00:00Z

LGTM. Just out of curiosity, what can break from this change that wouldn't already be a crash before?

MylesBorins · 2015-10-27T01:04:18Z

lib/_http_outgoing.js

@@ -482,7 +484,7 @@ OutgoingMessage.prototype.write = function(chunk, encoding, callback) {

 function writeAfterEndNT(self, err, callback) {
  self.emit('error', err);
-  if (callback) callback(err);
+  if (typeof callback === 'function') callback(err);


This is an interesting one. Prior to the change a callback that isn't a function would throw. In this instance it would fail silently

Probably needs an else if (callback) throw … or something like that

Trott · 2015-10-27T01:04:49Z

@cjihrig Can't tell without checking more closely, but it seems extremely likely that things will fail differently. So, someone may have code wrapped in a try/catch that expects a TypeError but now gets something else entirely.

Which actually brings up a point: Should these be changed so that there's an else that says that if callback is truthy but not a function, throw a TypeError? And if so, why not just leave them the way they are?

EDIT: I guess one advantage is you could be more stringent and say that callback needs to be null, undefined, or a function and anything else is a TypeError whereas the current situation will let falsy values like 0 slip by.

cjihrig · 2015-10-27T01:09:41Z

That is REALLY pushing the line between a fix and a breaking change, especially since most of these are asynchronous.

I wouldn't be opposed to throwing when a truthy, non-function value is passed, but then we're definitely into breaking change territory.

MylesBorins · 2015-10-27T01:10:07Z

@Trott just brought up the exact thing I was just thinking. With this change it is possible for things to fail silently.

That being said, is it the duty of node.js to do type checking on function calls?

Trott · 2015-10-27T01:21:05Z

@cjihrig I'm OK with removing the semver-major tag if that's what consensus is. One litmus test perhaps: Would we want to port this change to LTS? I'd argue "no" and would therefore favor semver-major in this case.

cjihrig · 2015-10-27T01:23:38Z

I'm still not really convinced that this is semver-major, but I'm fine with making it semver-major just for the sake of extra caution.

MylesBorins · 2015-10-27T01:25:46Z

Question for ya'll... how do we avoid the two patterns from diverging in the future.

Would it be unreasonable to make a linting rule for this type of thing?

Trott · 2015-10-27T01:31:03Z

CI, in case there are any surprises: https://ci.nodejs.org/job/node-test-pull-request/623/

thefourtheye · 2015-10-27T02:48:48Z

lib/_http_outgoing.js

    this.on('timeout', callback);
  }
+  else {
+    throw new TypeError('callback must be a function');


This is not necessary. The on function takes care of it already. I would say you can simply remove the if guard around on, once, and removeListener, as they check if the parameter passed to them are functions.

didn't mean to bury this but realized that there was a bug here.

Want to continue the conversation at --> https://github.com/nodejs/node/pull/3539/files#r43200588

jasnell · 2015-10-27T17:19:27Z

I think semver-major is the safest bet on this one. This is exactly the kind of change that leads to "oh crap we broke the world" moments immediately after cutting a new release. Throwing on a truthy non-function is a very good idea, I think.

A linting rule on callback type checking would be an interesting approach. We really ought to be more consistent about it.

Throughout the codebase there are two pattern for checking if a callback has been provided as an argument. This PR replaces all instances to do explicit type checking. Closes #3536

MylesBorins · 2015-10-27T23:07:50Z

lib/_http_outgoing.js

    this.on('timeout', callback);
  }
+  else if (callback) {
+    throw new TypeError('callback must be a function');


continuing https://github.com/nodejs/node/pull/3539/files#r43079350

@thefourtheye so you are suggesting that this Type check shouldn't be done.

@Trott suggested otherwise in --> #3539 (comment)

@jasnell was for it as well --> #3539 (comment)

I'm for it if there is the typeof check. I think @thefourtheye is saying that you can discard the typeof check and the else block and just pass blindly to .on() because .on() will throw a TypeError if the callback is not a function. At least, that's my understanding. So I'm for either approach, really. Don't make no neither nor to me.

In other words, I think @thefourtheye is saying this would be fine:

if (callback) { this.on('timeout', callback); }

Which throws a wrench in the whole "standardize on typeof checks" approach.

We need that if guard if and only if callback might be optional and therefore undefined. I didn't look at the code to see if that is actually possible.

Right... At least the event handler code doesn't need this if block as it is done anyway in on, once, {add,remove}listener functions

MylesBorins · 2015-10-28T23:25:25Z

Any thoughts on if this is a change we want to adopt?

rvagg · 2015-10-28T23:27:53Z

seems like maybe we need a util._checkIsCallbackArgument() or something that does the typeof and also will throw a TypeError on a truthy non-function

I'm down with this either way, the current truthy is a smell and is now inconsistent across lib/

MylesBorins · 2015-10-28T23:29:43Z

I like the idea of a generic util used across the lib for consistency. Would it make sense for this to land first as an intermediary step to make sure too many things don't blow up, and then add the refactoring with the util afterwards?

rvagg · 2015-10-28T23:32:44Z

Well, landing this first means that we go from callback is not a function errors to silently failing, adding in a truthy-non-function check would put us to a TypeError which is on par with the runtime callback is not a function error, so landing this first puts us further away in terms of compatibility.

I think it would be best to add a truthy-non-function check with TypeError before landing this, whether it's done with a shared util function or done inline. Anyone else want to weigh in on this?

jasnell · 2015-10-28T23:42:15Z

I definitely agree with @rvagg
On Oct 28, 2015 4:32 PM, "Rod Vagg" [email protected] wrote:

Well, landing this first means that we go from callback is not a function
errors to silently failing, adding in a truthy-non-function check would put
us to a TypeError which is on par with the runtime callback is not a
function error, so landing this first puts us further away in terms of
compatibility.

I think it would be best to add a truthy-non-function check with
TypeError before landing this, whether it's done with a shared util
function or done inline. Anyone else want to weigh in on this?

—
Reply to this email directly or view it on GitHub
#3539 (comment).

MylesBorins · 2015-10-29T00:19:37Z

Ok I did a first pass at implementing the util function and came across some weirdness while running the test suite. Turns out the expected behavior of some functions that take callbacks is to fail silently if a bad argument is given :(

socket.close is such an example

https://github.com/TheAlphaNerd/node/blob/null-check-drain-listener/test/parallel/test-dgram-close-is-not-callback.js#L13-L14

What do people think of being stricter in these instances... obviously semver-major, but is it desirable to change this much underlying logic?

MylesBorins · 2015-10-29T00:25:32Z

So digging further into dgram Socket.close it would appear that the callback argument is optional. It is not documented that an incorrect argument will be ignored. I have not gone through all instances of an optional callback, but at the very least we can deduce that the narrative is inconsistent. This will definitely be breaking, but I personally see value in consistency.

MylesBorins · 2015-11-03T18:08:46Z

anyone have thoughts on the above information?

r-52 · 2015-11-08T18:20:11Z

Looks like all other dns functions perform a callback check except dns.lookupService (https://github.com/nodejs/node/blob/master/lib/dns.js#L185-L195). Makes it sense to include it in this PR?

MylesBorins · 2015-12-14T17:56:57Z

I am closing this for now as it is not entirely obvious that this will be easy to land (with various ways to handle callbacks)

MylesBorins reviewed Oct 27, 2015
View reviewed changes

Trott added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 27, 2015

MylesBorins reviewed Oct 27, 2015
View reviewed changes

thefourtheye reviewed Oct 27, 2015
View reviewed changes

lib: consistent callback checking across project

ce7e901

Throughout the codebase there are two pattern for checking if a callback has been provided as an argument. This PR replaces all instances to do explicit type checking. Closes #3536

MylesBorins reviewed Oct 27, 2015
View reviewed changes

MylesBorins mentioned this pull request Nov 1, 2015

http: check callback type of imcoming timeout #3618

Closed

MylesBorins mentioned this pull request Nov 3, 2015

zlib: only apply drain listener if given callback #3534

Closed

MylesBorins closed this Dec 14, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: consistent callback checking across project #3539

lib: consistent callback checking across project #3539

MylesBorins commented Oct 27, 2015

MylesBorins Oct 27, 2015

MylesBorins Oct 27, 2015

Trott commented Oct 27, 2015

cjihrig commented Oct 27, 2015

MylesBorins Oct 27, 2015

ChALkeR Nov 8, 2015

Trott commented Oct 27, 2015

cjihrig commented Oct 27, 2015

MylesBorins commented Oct 27, 2015

Trott commented Oct 27, 2015

cjihrig commented Oct 27, 2015

MylesBorins commented Oct 27, 2015

Trott commented Oct 27, 2015

thefourtheye Oct 27, 2015

MylesBorins Oct 27, 2015

jasnell commented Oct 27, 2015

MylesBorins Oct 27, 2015

MylesBorins Oct 27, 2015

MylesBorins Oct 27, 2015

Trott Oct 27, 2015

Trott Oct 27, 2015

Trott Oct 27, 2015

thefourtheye Oct 29, 2015

MylesBorins commented Oct 28, 2015

rvagg commented Oct 28, 2015

MylesBorins commented Oct 28, 2015

rvagg commented Oct 28, 2015

jasnell commented Oct 28, 2015

MylesBorins commented Oct 29, 2015

MylesBorins commented Oct 29, 2015

MylesBorins commented Nov 3, 2015

r-52 commented Nov 8, 2015

MylesBorins commented Dec 14, 2015

lib: consistent callback checking across project #3539

lib: consistent callback checking across project #3539

Conversation

MylesBorins commented Oct 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Oct 27, 2015

cjihrig commented Oct 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Oct 27, 2015

cjihrig commented Oct 27, 2015

MylesBorins commented Oct 27, 2015

Trott commented Oct 27, 2015

cjihrig commented Oct 27, 2015

MylesBorins commented Oct 27, 2015

Trott commented Oct 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Oct 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MylesBorins commented Oct 28, 2015

rvagg commented Oct 28, 2015

MylesBorins commented Oct 28, 2015

rvagg commented Oct 28, 2015

jasnell commented Oct 28, 2015

MylesBorins commented Oct 29, 2015

MylesBorins commented Oct 29, 2015

MylesBorins commented Nov 3, 2015

r-52 commented Nov 8, 2015

MylesBorins commented Dec 14, 2015