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.

Sign up for GitHub

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

Jump to bottom

net: set ADDRCONFIG DNS hint in connections #6281

Merged
merged 1 commit into from
Apr 20, 2016
Merged

net: set ADDRCONFIG DNS hint in connections #6281

merged 1 commit into from
Apr 20, 2016

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Apr 19, 2016

Checklist
  • tests and code linting passes
Affected core subsystem(s)

net

Description of change

b85a50b removed the implicit setting of DNS hints when creating a connection. Stress testing has shown that this caused the pi2 machines to become flaky. This commit restores the implicit dns.ADDRCONFIG hint, but not the more problematic dns.V4MAPPED. The stress test in https://ci.nodejs.org/job/node-stress-single-test/655/ has, so far, stopped being flaky with this change.

I'll update the commit message prior to landing.

Refs: #6133

Likely also
Refs: #6265
Refs: #6134

@mscdex mscdex added dns Issues and PRs related to the dns subsystem. net Issues and PRs related to the net subsystem. labels Apr 19, 2016
@silverwind silverwind added dns Issues and PRs related to the dns subsystem. and removed dns Issues and PRs related to the dns subsystem. labels Apr 19, 2016
@bnoordhuis
Copy link
Member

LGTM. The issue on the pi2's is that they receive IPv6 addresses when they're IPv4-only or vice versa?

@claudiorodriguez
Copy link
Contributor

(Full) CI: https://ci.nodejs.org/job/node-test-pull-request/2324/
LGTM if it's green

@jasnell
Copy link
Member

jasnell commented Apr 19, 2016

LGTM

@Trott
Copy link
Member

Trott commented Apr 20, 2016

LGTM. Wouldn't mind seeing this get fast-tracked so that:

  • Node.js v6.0.0 goes out with this and we don't have to discuss if trying to change this in 6.0.1 is actually semver-major
  • that irksome test-http-agent flakiness goes away on Pi 2 CI (and maybe other irksome Pi 2 flaky tests too)

@Trott
Copy link
Member

Trott commented Apr 20, 2016

CI is green. Multiple stress tests are green too.

@addaleax
Copy link
Member

LGTM

1 similar comment
@santigimeno
Copy link
Member

LGTM

@cjihrig
Copy link
Contributor Author

cjihrig commented Apr 20, 2016

@bnoordhuis to be honest, I'm not sure. I'm not sure how those machines are configured. I was trying to fix the flakiness, and knew it could only be some combination of those two DNS hints. I'm glad it wasn't V4MAPPED, as that one has been a pain. I'm not a huge fan of just adding the hint back, but ADDRCONFIG has seemed pretty harmless.

@cjihrig
Copy link
Contributor Author

cjihrig commented Apr 20, 2016

Oh, and @Trott thanks for running those stress tests.

@saghul
Copy link
Member

saghul commented Apr 20, 2016

LGTM

@jasnell
Copy link
Member

jasnell commented Apr 20, 2016

@cjihrig ... do you wanna land this? (or I can, I'm about to go through a batch of PR's again)

@cjihrig
net: set ADDRCONFIG DNS hint in connections
54dd7c3 
b85a50b removed the implicit
setting of DNS hints when creating a connection. This caused some
of the pi2 machines to become flaky. This commit restores the
implicit dns.ADDRCONFIG hint, but not dns.V4MAPPED.

Fixes: nodejs#6133
PR-URL: nodejs#6281
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
@cjihrig cjihrig merged commit 54dd7c3 into nodejs:master Apr 20, 2016
@cjihrig cjihrig deleted the flag branch April 20, 2016 16:05
This was referenced Apr 21, 2016
Closed
Closed
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
@cjihrig @joelostrowski
net: set ADDRCONFIG DNS hint in connections
1220f3f 
b85a50b removed the implicit
setting of DNS hints when creating a connection. This caused some
of the pi2 machines to become flaky. This commit restores the
implicit dns.ADDRCONFIG hint, but not dns.V4MAPPED.

Fixes: nodejs#6133
PR-URL: nodejs#6281
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
@cjihrig @jasnell
net: set ADDRCONFIG DNS hint in connections
aa395b7 
b85a50b removed the implicit
setting of DNS hints when creating a connection. This caused some
of the pi2 machines to become flaky. This commit restores the
implicit dns.ADDRCONFIG hint, but not dns.V4MAPPED.

Fixes: #6133
PR-URL: #6281
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Claudio Rodriguez <cjrodr@yahoo.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
dns Issues and PRs related to the dns subsystem. net Issues and PRs related to the net subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.