Skip to content
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

dns: Use object without protoype for map #5843

Conversation

benjamingr
Copy link
Member

Pull Request check-list

Affected core subsystem(s)

dns

Description of change

Please provide a description of the change here.

Currently we use {} for the lookup function to find the relevant
resolver to the dns.resolve function. It is preferable to use an
object without a Object.prototype, currently for example you can do
something like:

dns.resolve("google.com", "toString", console.log);

And get [Object undefined] logged and the callback would never be
called. This is unexpected and strange behavior in my opinion.
In addition, if someone adds a property to Object.prototype might
also create unexpected results.

I recalled a client actually ran into this issue in production a while ago so now that I'm touching a bunch of dns I figured I might as well fix it :)

This pull request fixes it, with it an appropriate error is thrown.

I tagged this semver-major to be on the safe side since an error is thrown where strange behavior currently happens. This is more likely a bug fix but I figured I'd error on the safe side - this is more like a bug fix.

Please advise.

@benjamingr benjamingr added dns Issues and PRs related to the dns subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Mar 22, 2016
Currently we use `{}` for the `lookup` function to find the relevant
resolver to the dns.resolve function. It is preferable to use an
object without a Object.prototype, currently for example you can do
something like:

```js
dns.resolve("google.com", "toString", console.log);
```

And get `[Object undefined]` logged and the callback would never be
called. This is unexpected and strange behavior in my opinion.
In addition, if someone adds a property to `Object.prototype` might
also create unexpected results.

This pull request fixes it, with it an appropriate error is thrown.
@Fishrock123
Copy link
Contributor

Please try to remove the qs readme rename from your git history. :S

@Fishrock123
Copy link
Contributor

Seems fine to me, since we only ever make it one we don't need to worry about the perf hit.

@benjamingr benjamingr force-pushed the dns-refactor-object-map-to-createnull branch from ef50aeb to 6f584af Compare March 22, 2016 14:15
@benjamingr
Copy link
Member Author

@Fishrock123 do you have any idea how that could happen (the qs readme rename)? (Fixed.)

Also, why would there be a perf hit in these cases - is Object.create(null) generally slower than {} for property lookup? I assumed it wouldn't be the case. I agree it's not significant anyway in this case, but I'm wondering for future reference in case I ever want to make this change in hot paths.

What do you think about the semver?

@Fishrock123
Copy link
Contributor

@Fishrock123 do you have any idea how that could happen (the qs readme rename)? (Fixed.)

git with case-sensitive filesystems. At one point there was a move and rename as the same time with the same name and different casing. Turns out, git usually doesn't like that and you end up with problems.

Also, why would there be a perf hit in these cases - is Object.create(null) generally slower than {}

It is something like 70 times slower to create. So, possibly an issue if you do it a lot.

probably semver-patch

@benjamingr benjamingr removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 22, 2016
@benjamingr
Copy link
Member Author

git with case-sensitive filesystems. At one point there was a move and rename as the same time with the same name and different casing. Turns out, git usually doesn't like that and you end up with problems.

Oh! That makes sense, thanks!

It is something like 70 times slower to create. So, possibly an issue if you do it a lot.

Ok, good to know.

probably semver-patch

Thanks, and thanks for the review.

@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

Seems reasonable. May want to include a test case.

@benjamingr
Copy link
Member Author

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

Added test, good idea.

@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

LGTM if CI is green

@targos
Copy link
Member

targos commented Mar 22, 2016

LGTM

1 similar comment
@cjihrig
Copy link
Contributor

cjihrig commented Mar 22, 2016

LGTM

jasnell pushed a commit that referenced this pull request Mar 22, 2016
Currently we use `{}` for the `lookup` function to find the relevant
resolver to the dns.resolve function. It is preferable to use an
object without a Object.prototype, currently for example you can do
something like:

```js
dns.resolve("google.com", "toString", console.log);
```

And get `[Object undefined]` logged and the callback would never be
called. This is unexpected and strange behavior in my opinion.
In addition, if someone adds a property to `Object.prototype` might
also create unexpected results.

This pull request fixes it, with it an appropriate error is thrown.

PR-URL: #5843
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

Landed in c9c387f

@jasnell jasnell closed this Mar 22, 2016
@benjamingr
Copy link
Member Author

Thanks. Aren't we supposed to wait 48h on these?

(Newbie question, I've only been added as a collaborator a week ago)

@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

Generally, yes, this one is quite simple, had green CI, and got plenty of LGTM's pretty quickly, seemed worthwhile.

@benjamingr
Copy link
Member Author

Great, thanks.

evanlucas pushed a commit that referenced this pull request Mar 30, 2016
Currently we use `{}` for the `lookup` function to find the relevant
resolver to the dns.resolve function. It is preferable to use an
object without a Object.prototype, currently for example you can do
something like:

```js
dns.resolve("google.com", "toString", console.log);
```

And get `[Object undefined]` logged and the callback would never be
called. This is unexpected and strange behavior in my opinion.
In addition, if someone adds a property to `Object.prototype` might
also create unexpected results.

This pull request fixes it, with it an appropriate error is thrown.

PR-URL: #5843
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
evanlucas pushed a commit that referenced this pull request Mar 31, 2016
Currently we use `{}` for the `lookup` function to find the relevant
resolver to the dns.resolve function. It is preferable to use an
object without a Object.prototype, currently for example you can do
something like:

```js
dns.resolve("google.com", "toString", console.log);
```

And get `[Object undefined]` logged and the callback would never be
called. This is unexpected and strange behavior in my opinion.
In addition, if someone adds a property to `Object.prototype` might
also create unexpected results.

This pull request fixes it, with it an appropriate error is thrown.

PR-URL: #5843
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 8, 2016
Currently we use `{}` for the `lookup` function to find the relevant
resolver to the dns.resolve function. It is preferable to use an
object without a Object.prototype, currently for example you can do
something like:

```js
dns.resolve("google.com", "toString", console.log);
```

And get `[Object undefined]` logged and the callback would never be
called. This is unexpected and strange behavior in my opinion.
In addition, if someone adds a property to `Object.prototype` might
also create unexpected results.

This pull request fixes it, with it an appropriate error is thrown.

PR-URL: #5843
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dns Issues and PRs related to the dns subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants