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: fix empty elem in result array #3696

Closed
wants to merge 1 commit into from
Closed

Conversation

john-yan
Copy link

@john-yan john-yan commented Nov 6, 2015

When getaddrinfo linked-list results contain entries other than AF_INET
and AF_INET6, the resulting v8::Array will contain undefined value.
That's because initialization of v8::Array pre-allocate entries for all
linked-list nodes, but we may actually some of them after the 2 while
loops.

When getaddrinfo linked-list results contain entries other than AF_INET
and AF_INET6, the resulting v8::Array will contain undefined value.
That's because initialization of v8::Array pre-allocate entries for all
linked-list nodes, but we may actually some of them after the 2 while
loops.
@Fishrock123 Fishrock123 added the dns Issues and PRs related to the dns subsystem. label Nov 6, 2015
@john-yan
Copy link
Author

john-yan commented Nov 6, 2015

This fixes intermittent failure of test/internet/test-dns-ipv6.js caused by assertion failure on line 168.

     ips.forEach(function(ip) {
       assert.ok(isIPv6(ip.address),                           // failing assertion
                 'Invalid IPv6: ' + ip.address.toString());
       assert.strictEqual(ip.family, 6);
     });

I observed that the ip.address is undefined.

then I try to print out the whole ips, and found this output:

[ { address: '2607:f8b0:4004:806::1010', family: 6 },
  { address: undefined, family: 6 } ]

then I try to print out the parameter "addresses" in onlookupall in lib/dns.js:

[ '2607:f8b0:4004:806::1010',  ]

@cjihrig
Copy link
Contributor

cjihrig commented Nov 6, 2015

LGTM, but I'd like the input of someone more familiar with that part of the code base. Are we going to take a hit somewhere by not passing n to Array::New?

@john-yan
Copy link
Author

john-yan commented Nov 6, 2015

@cjihrig btw, the default length passing to Array::New is 0.

@mhdawson
Copy link
Member

mhdawson commented Nov 6, 2015

If there is some concern with not passing a larger default length, can we remove the entries that have an undefined address after the fact ?

@cjihrig
Copy link
Contributor

cjihrig commented Nov 6, 2015

Seems like a question @trevnorris might know the answer to. Is it likely faster to create a larger array up front and then remove undefined values, or build the array as needed? It may also depend on how common undefined values are.

@john-yan
Copy link
Author

john-yan commented Nov 6, 2015

@cjihrig undefined value is a special double representation (8 bytes).

@john-yan
Copy link
Author

john-yan commented Nov 6, 2015

@mhdawson allocating a larger array and then removing entries would increase internal fragmentation. It may not benefit performance.

@trevnorris
Copy link
Contributor

Without testing, I would say each are equally fast or slow. Wouldn't worry about that amount of possible performance gain here.

@cjihrig
Copy link
Contributor

cjihrig commented Nov 6, 2015

In that case, I'd say this is fine as long as the CI is happy.

@john-yan
Copy link
Author

john-yan commented Nov 6, 2015

@trevnorris absolutely agree.

@bnoordhuis
Copy link
Member

LGTM, this is not performance critical. At least, not so critical that specifying the length upfront makes a difference.

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

@john-yan
Copy link
Author

Hello, can someone merge this in? It has been 4 days now. :)

@jasnell
Copy link
Member

jasnell commented Nov 10, 2015

@john-yan .. there were some failures in the CI on freebsd. They look unrelated but can you please take a look and confirm.

@john-yan
Copy link
Author

@jasnell Hello, the failures show some native modules not found. They should be unrelated.

@cjihrig
Copy link
Contributor

cjihrig commented Nov 10, 2015

Yes, this looks like it should be fine. Landing.

cjihrig pushed a commit that referenced this pull request Nov 10, 2015
When getaddrinfo linked-list results contain entries other than AF_INET
and AF_INET6, the resulting v8::Array will contain undefined values.
That's because initialization of v8::Array pre-allocates entries for all
linked-list nodes, but not all of them will be in the final results.
This commit ensures that the array only contains valid results.

PR-URL: #3696
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@cjihrig
Copy link
Contributor

cjihrig commented Nov 10, 2015

Thanks! Landed in 19bf72d

@cjihrig cjihrig closed this Nov 10, 2015
Fishrock123 pushed a commit that referenced this pull request Nov 11, 2015
When getaddrinfo linked-list results contain entries other than AF_INET
and AF_INET6, the resulting v8::Array will contain undefined values.
That's because initialization of v8::Array pre-allocates entries for all
linked-list nodes, but not all of them will be in the final results.
This commit ensures that the array only contains valid results.

PR-URL: #3696
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@Fishrock123 Fishrock123 mentioned this pull request Nov 11, 2015
john-yan pushed a commit to ibmruntimes/node that referenced this pull request Nov 12, 2015
When getaddrinfo linked-list results contain entries other than AF_INET
and AF_INET6, the resulting v8::Array will contain undefined values.
That's because initialization of v8::Array pre-allocates entries for all
linked-list nodes, but not all of them will be in the final results.
This commit ensures that the array only contains valid results.

PR-URL: nodejs/node#3696
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 17, 2015
When getaddrinfo linked-list results contain entries other than AF_INET
and AF_INET6, the resulting v8::Array will contain undefined values.
That's because initialization of v8::Array pre-allocates entries for all
linked-list nodes, but not all of them will be in the final results.
This commit ensures that the array only contains valid results.

PR-URL: #3696
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@MylesBorins
Copy link
Contributor

landed in v4.x-staging in a632db5

rvagg pushed a commit that referenced this pull request Dec 4, 2015
When getaddrinfo linked-list results contain entries other than AF_INET
and AF_INET6, the resulting v8::Array will contain undefined values.
That's because initialization of v8::Array pre-allocates entries for all
linked-list nodes, but not all of them will be in the final results.
This commit ensures that the array only contains valid results.

PR-URL: #3696
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@jasnell jasnell mentioned this pull request Dec 17, 2015
jasnell pushed a commit that referenced this pull request Dec 17, 2015
When getaddrinfo linked-list results contain entries other than AF_INET
and AF_INET6, the resulting v8::Array will contain undefined values.
That's because initialization of v8::Array pre-allocates entries for all
linked-list nodes, but not all of them will be in the final results.
This commit ensures that the array only contains valid results.

PR-URL: #3696
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
jasnell pushed a commit that referenced this pull request Dec 23, 2015
When getaddrinfo linked-list results contain entries other than AF_INET
and AF_INET6, the resulting v8::Array will contain undefined values.
That's because initialization of v8::Array pre-allocates entries for all
linked-list nodes, but not all of them will be in the final results.
This commit ensures that the array only contains valid results.

PR-URL: #3696
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
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.

8 participants