dgram: prefer strict equality by claudiorodriguez · Pull Request #8011 · nodejs/node

claudiorodriguez · 2016-08-08T09:41:30Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

dgram

Description of change

Enforces strict comparisons in dgram.
bindState should always be strictly equal to one of the defined constant states, and newHandle type is a string.

thefourtheye · 2016-08-08T10:26:02Z

Strictly speaking, this would be semver-major...

console.log(['udp4'] == 'udp4');

Now this will not work...

claudiorodriguez · 2016-08-08T10:33:11Z

@thefourtheye indeed, good point! All the tests passed on my machine, so I'm guessing there's no tests for that use case? In the docs here https://nodejs.org/api/dgram.html#dgram_dgram_createsocket_type_callback the type is "string". Are there any examples of type being an array in the wild? I definitely agree though, I'm just trying to understand the impact of this.

thefourtheye · 2016-08-08T10:40:31Z

@claudiorodriguez I don't have solid evidence that people use something like that. Perhaps the impact might not be that big and may probably be characterized as a bug-fix. I am not very sure though.

@nodejs/collaborators thoughts?

cjihrig · 2016-08-08T13:52:03Z

LGTM. I'm fine with semver patch.

claudiorodriguez · 2016-08-08T14:15:35Z

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

jasnell · 2016-08-08T15:09:18Z

This would need to be a semver-major if it landed as is even if passing ['whatever'] is an edge case. An additional check to see if the argument is an array, and if it is, pull out the first element, could be added to make this backwards compatible. e.g.

if (Array.isArray(type))
  type = type[0];

cjihrig · 2016-08-08T15:18:14Z

-1 to adding an additional array check. We're already catering to undocumented/unsupported behavior here. I'd rather see it become a semver major change than to add code for a ridiculous edge case.

jasnell · 2016-08-08T15:19:10Z

Works for me as a semver-major

On Monday, August 8, 2016, Colin Ihrig notifications@github.com wrote:

-1 to adding an additional array check. We're already catering to
undocumented/unsupported behavior here. I'd rather see it become a semver
major change then to add code for a ridiculous edge case.

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#8011 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2eTrKP7A3pJ2AP0fTzqW5HdjAMNCDks5qd0i9gaJpZM4Je3sg
.

claudiorodriguez · 2016-08-08T15:33:52Z

I just realized that there's also new String('udp4'), which would break anyway if you were doing it for some strange reason. And if we did something like

if (Array.isArray(type)) {
  type = type[0];
} else if (typeof type !== 'string') {
  type = String(type);
}

but that would defeat the entire purpose of the strict equality. So I'm also +1 on major.

jasnell · 2016-08-08T15:35:37Z

Thinking about it further, we could always force a coercion... e.g. type = String(type) ... that would cover both the Array and new String() cases...

yorkie · 2016-08-08T17:09:57Z

BTW, should we add cases to cover these strict equations?

claudiorodriguez · 2016-08-08T17:14:02Z

@yorkie it would make sense for type, one way or the other. I'll see about adding some tests tomorrow.

mcollina · 2016-08-16T09:07:24Z

LGTM

JacksonTian · 2016-08-16T13:31:53Z

LGTM with semver-major.

claudiorodriguez · 2016-08-17T08:27:36Z

While writing the test I noticed that currently { type: 'udp4' } is a valid (and undocumented) type argument for createSocket. This, unlike ['udp4'] seemed very explicit, so I accomodated for it and added that possibility to the list of valid values in the test.
PTAL

claudiorodriguez · 2016-08-17T08:31:46Z

lib/dgram.js

It actually is if the proper error message is to be thrown. Otherwise it fails with null with a "cannot access property type of null" message (typeof null is 'object').

Yea, +1 on your comment :-)

Can you explicitly check for null in this check.

yorkie · 2016-08-17T08:40:19Z

LGTM

claudiorodriguez · 2016-08-17T08:41:24Z

New CI run: https://ci.nodejs.org/job/node-test-pull-request/3704/

claudiorodriguez · 2016-08-18T17:55:10Z

CI failure seems unrelated.
Retrying just in case: https://ci.nodejs.org/job/node-test-pull-request/3732/

claudiorodriguez · 2016-08-23T10:46:59Z

@mcollina @JacksonTian @cjihrig are you okay with the latest changes?

mcollina · 2016-08-23T11:13:22Z

Can you please squash the commits?

claudiorodriguez · 2016-08-23T11:20:59Z

Done

mcollina · 2016-08-23T11:52:11Z

Ok for me.

@claudiorodriguez are you a collaborator? Otherwise I'll merge it.

jasnell · 2016-08-23T16:55:58Z

@mcollina ... do you know if there are any significant npm modules that depend on dgram that we can test with CITGM for this change? I'd be more comfortable signing off if we could get at least one good citgm run but I'm not sure any of our existing modules there actually use dgram.

mcollina · 2016-08-23T17:06:29Z

https://github.com/mafintosh/k-rpc-socket (powers the torrent modules)

I'll also cc @mafintosh, a couple of his p2p modules can definitely be in there.

I own coap, but it has a couple of flaky tests.

cjihrig · 2016-08-24T00:42:44Z

LGTM with a comment.

- Enforces strict comparisons in dgram - bindState should always be strictly equal to one of the defined constant states, and newHandle type is a string. - Check that the argument `type` in createSocket is not null when it is of type 'object', before using its `type` property. - Adds a test to check dgram.createSocket is properly validating its `type` argument. PR-URL: nodejs#8011

claudiorodriguez · 2016-08-24T08:03:04Z

@cjihrig thanks, changed

mcollina · 2016-08-24T10:53:31Z

@jasnell I will also include https://www.npmjs.com/package/multicast-dns.

addaleax · 2016-08-28T13:24:28Z

New CI: https://ci.nodejs.org/job/node-test-commit/4807/

Is there anything keeping this from landing?

mcollina · 2016-08-28T13:52:18Z

@jasnell said #8011 (comment).

@addaleax IMHO we should land this in v7.

addaleax · 2016-08-28T13:59:39Z

lib/dgram.js

  EventEmitter.call(this);

-  if (typeof type === 'object') {
+  if (type !== null && typeof type === 'object') {


I think V8 likes it better to have typeof checks come first.

@addaleax do you have any docs on this? Not saying it's not the case - I don't really know myself, but a quick google search throws nothing, and in the lib codebase both cases seem to be equally prevalent:

node/lib/https.js

Line 53 in be68b68

function createConnection(port, host, options) {

node/lib/dns.js

Line 111 in 013d76c

if (hostname && typeof hostname !== 'string') {

vs

node/lib/querystring.js

Line 172 in 6979632

if (typeof v === 'number' && isFinite(v))

node/lib/events.js

Line 63 in b7a8a69

if (typeof n !== 'number' || n < 0 || isNaN(n))

if we can confirm one case is better, this could mean future improvement PRs and more consistency over the codebase

@claudiorodriguez I’ll try to look it up/benchmark it, all I could remember right now is emberjs/ember.js#14118 … might be a different case with !== null, but even so I think I’d find the typeof-first variants more readable.

@claudiorodriguez This does indeed seem to be different with !== null. You can feel free to disregard my comment :)
Expressions of the form a && typeof a === 'object' definitely seem to be a reason for deopts, though.

addaleax · 2016-08-28T14:00:18Z

ack, I’m attaching the milestone to it.

jasnell · 2016-09-08T19:47:52Z

@addaleax @mcollina ... if we're going to get this into v7 then it would need to land before the morning of Monday Sept 12th. I'm going to be cutting the v7.x branch that morning to start the beta process.

mcollina · 2016-09-08T20:08:29Z

@jasnell we are waiting for you, you said you wanted solid coverage in CITGM before landing. May I merge, or what else should I do to make this progress?

Everybody is in agreement here.

jasnell · 2016-09-08T20:15:38Z

I think we should be good to go. We can verify the citgm stuff as we go.

addaleax · 2016-09-08T20:29:30Z

Cool – @claudiorodriguez do you want to go ahead and land this?

claudiorodriguez · 2016-09-08T21:22:28Z

Landed in e9b6fbb

- Enforces strict comparisons in dgram - bindState should always be strictly equal to one of the defined constant states, and newHandle type is a string. - Check that the argument `type` in createSocket is not null when it is of type 'object', before using its `type` property. - Adds a test to check dgram.createSocket is properly validating its `type` argument. PR-URL: #8011 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com> Reviewed-By: Jackson Tian <shvyo1987@gmail.com>

claudiorodriguez added the dgram Issues and PRs related to the dgram subsystem / UDP. label Aug 8, 2016

claudiorodriguez added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 8, 2016

claudiorodriguez force-pushed the dgram-prefer-strict-equal branch from a5ba154 to 8360593 Compare August 17, 2016 08:24

claudiorodriguez force-pushed the dgram-prefer-strict-equal branch from 8360593 to a24d87d Compare August 17, 2016 08:30

claudiorodriguez reviewed Aug 17, 2016
View reviewed changes

claudiorodriguez force-pushed the dgram-prefer-strict-equal branch from a24d87d to 9da93c0 Compare August 23, 2016 11:20

claudiorodriguez force-pushed the dgram-prefer-strict-equal branch from 9da93c0 to 3e1e974 Compare August 24, 2016 07:37

claudiorodriguez force-pushed the dgram-prefer-strict-equal branch from 3e1e974 to daf6284 Compare August 24, 2016 07:39

addaleax reviewed Aug 28, 2016
View reviewed changes

addaleax added this to the 7.0.0 milestone Aug 28, 2016

claudiorodriguez closed this Sep 8, 2016

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

Uh oh!

Conversation

claudiorodriguez commented Aug 8, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

thefourtheye commented Aug 8, 2016

Uh oh!

claudiorodriguez commented Aug 8, 2016

Uh oh!

thefourtheye commented Aug 8, 2016

Uh oh!

cjihrig commented Aug 8, 2016

Uh oh!

claudiorodriguez commented Aug 8, 2016

Uh oh!

jasnell commented Aug 8, 2016

Uh oh!

cjihrig commented Aug 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Aug 8, 2016

Uh oh!

claudiorodriguez commented Aug 8, 2016

Uh oh!

jasnell commented Aug 8, 2016

Uh oh!

yorkie commented Aug 8, 2016

Uh oh!

claudiorodriguez commented Aug 8, 2016

Uh oh!

mcollina commented Aug 16, 2016

Uh oh!

JacksonTian commented Aug 16, 2016

Uh oh!

claudiorodriguez commented Aug 17, 2016

Uh oh!

claudiorodriguez Aug 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yorkie Aug 17, 2016

Choose a reason for hiding this comment

Uh oh!

cjihrig Aug 24, 2016

Choose a reason for hiding this comment

Uh oh!

yorkie commented Aug 17, 2016

Uh oh!

claudiorodriguez commented Aug 17, 2016

Uh oh!

claudiorodriguez commented Aug 18, 2016

Uh oh!

claudiorodriguez commented Aug 23, 2016

Uh oh!

mcollina commented Aug 23, 2016

Uh oh!

claudiorodriguez commented Aug 23, 2016

Uh oh!

mcollina commented Aug 23, 2016

Uh oh!

jasnell commented Aug 23, 2016

Uh oh!

mcollina commented Aug 23, 2016

Uh oh!

cjihrig commented Aug 24, 2016

Uh oh!

claudiorodriguez commented Aug 24, 2016

Uh oh!

mcollina commented Aug 24, 2016

Uh oh!

addaleax commented Aug 28, 2016

Uh oh!

mcollina commented Aug 28, 2016

Uh oh!

addaleax Aug 28, 2016

Choose a reason for hiding this comment

Uh oh!

claudiorodriguez Aug 28, 2016

cjihrig commented Aug 8, 2016 •

edited

Loading

claudiorodriguez Aug 17, 2016 •

edited

Loading