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

dgram: prefer strict equality #8011

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
8 participants
@claudiorodriguez
Member

claudiorodriguez commented Aug 8, 2016

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

This comment has been minimized.

Contributor

thefourtheye commented Aug 8, 2016

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

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

Now this will not work...

@claudiorodriguez

This comment has been minimized.

Member

claudiorodriguez commented Aug 8, 2016

@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

This comment has been minimized.

Contributor

thefourtheye commented Aug 8, 2016

@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

This comment has been minimized.

Contributor

cjihrig commented Aug 8, 2016

LGTM. I'm fine with semver patch.

@claudiorodriguez

This comment has been minimized.

@jasnell

This comment has been minimized.

Member

jasnell commented Aug 8, 2016

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

This comment has been minimized.

Contributor

cjihrig commented Aug 8, 2016

-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

This comment has been minimized.

Member

jasnell commented Aug 8, 2016

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

This comment has been minimized.

Member

claudiorodriguez commented Aug 8, 2016

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

This comment has been minimized.

Member

jasnell commented Aug 8, 2016

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

This comment has been minimized.

Member

yorkie commented Aug 8, 2016

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

@claudiorodriguez

This comment has been minimized.

Member

claudiorodriguez commented Aug 8, 2016

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

@mcollina

This comment has been minimized.

Member

mcollina commented Aug 16, 2016

LGTM

@JacksonTian

This comment has been minimized.

Contributor

JacksonTian commented Aug 16, 2016

LGTM with semver-major.

@claudiorodriguez claudiorodriguez force-pushed the claudiorodriguez:dgram-prefer-strict-equal branch Aug 17, 2016

@claudiorodriguez

This comment has been minimized.

Member

claudiorodriguez commented Aug 17, 2016

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 claudiorodriguez force-pushed the claudiorodriguez:dgram-prefer-strict-equal branch Aug 17, 2016

@claudiorodriguez

View changes

lib/dgram.js Outdated
@@ -78,7 +78,7 @@ exports._createSocketHandle = function(address, port, addressType, fd, flags) {
function Socket(type, listener) {
EventEmitter.call(this);
if (typeof type === 'object') {
if (type && typeof type === 'object') {

This comment has been minimized.

@claudiorodriguez

claudiorodriguez Aug 17, 2016

Member

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').

This comment has been minimized.

@yorkie

yorkie Aug 17, 2016

Member

Yea, +1 on your comment :-)

This comment has been minimized.

@cjihrig

cjihrig Aug 24, 2016

Contributor

Can you explicitly check for null in this check.

@yorkie

This comment has been minimized.

Member

yorkie commented Aug 17, 2016

LGTM

@claudiorodriguez

This comment has been minimized.

Member

claudiorodriguez commented Aug 17, 2016

@claudiorodriguez

This comment has been minimized.

Member

claudiorodriguez commented Aug 18, 2016

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

@claudiorodriguez

This comment has been minimized.

Member

claudiorodriguez commented Aug 23, 2016

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

@mcollina

This comment has been minimized.

Member

mcollina commented Aug 23, 2016

Can you please squash the commits?

@claudiorodriguez claudiorodriguez force-pushed the claudiorodriguez:dgram-prefer-strict-equal branch Aug 23, 2016

@claudiorodriguez

This comment has been minimized.

Member

claudiorodriguez commented Aug 23, 2016

Done

@mcollina

This comment has been minimized.

Member

mcollina commented Aug 23, 2016

Ok for me.

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

@jasnell

This comment has been minimized.

Member

jasnell commented Aug 23, 2016

@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

This comment has been minimized.

Member

mcollina commented Aug 23, 2016

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

This comment has been minimized.

Contributor

cjihrig commented Aug 24, 2016

LGTM with a comment.

@claudiorodriguez claudiorodriguez force-pushed the claudiorodriguez:dgram-prefer-strict-equal branch Aug 24, 2016

dgram: prefer strict equality, type validation
- 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

@claudiorodriguez claudiorodriguez force-pushed the claudiorodriguez:dgram-prefer-strict-equal branch to daf6284 Aug 24, 2016

@claudiorodriguez

This comment has been minimized.

Member

claudiorodriguez commented Aug 24, 2016

@cjihrig thanks, changed

@mcollina

This comment has been minimized.

Member

mcollina commented Aug 24, 2016

@addaleax

This comment has been minimized.

Member

addaleax commented Aug 28, 2016

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

Is there anything keeping this from landing?

@mcollina

This comment has been minimized.

Member

mcollina commented Aug 28, 2016

@jasnell said #8011 (comment).

@addaleax IMHO we should land this in v7.

@@ -78,7 +78,7 @@ exports._createSocketHandle = function(address, port, addressType, fd, flags) {
function Socket(type, listener) {
EventEmitter.call(this);
if (typeof type === 'object') {
if (type !== null && typeof type === 'object') {

This comment has been minimized.

@addaleax

addaleax Aug 28, 2016

Member

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

This comment has been minimized.

@claudiorodriguez

claudiorodriguez Aug 28, 2016

Member

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

function createConnection(port, host, options) {

node/lib/dns.js

Line 111 in 013d76c

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

vs

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

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

This comment has been minimized.

@addaleax

addaleax Aug 28, 2016

Member

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

This comment has been minimized.

@addaleax

addaleax Aug 29, 2016

Member

@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

This comment has been minimized.

Member

addaleax commented Aug 28, 2016

ack, I’m attaching the milestone to it.

@addaleax addaleax added this to the 7.0.0 milestone Aug 28, 2016

@jasnell

This comment has been minimized.

Member

jasnell commented Sep 8, 2016

@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

This comment has been minimized.

Member

mcollina commented Sep 8, 2016

@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

This comment has been minimized.

Member

jasnell commented Sep 8, 2016

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

@addaleax

This comment has been minimized.

Member

addaleax commented Sep 8, 2016

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

@claudiorodriguez

This comment has been minimized.

Member

claudiorodriguez commented Sep 8, 2016

Landed in e9b6fbb

claudiorodriguez added a commit that referenced this pull request Sep 8, 2016

dgram: prefer strict equality, type validation
- 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>

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment