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

net: cleanup connect logic and add ability to specify custom lookup function #1505

Closed
wants to merge 2 commits into from

Conversation

evanlucas
Copy link
Contributor

Separates out the lookup logic for net.Socket. In the event the host property is an IP address, the lookup is skipped.

Also allows customization of the lookup function in Socket.prototype.connect.

Ref: nodejs/node-v0.x-archive#8475

I used lookup instead of resolve since we are actually calling lookup currently. I wasn't sure if we wanted to go ahead and change to using resolve or not. If we did, what kind of impact would that have?

Separates out the lookup logic for net.Socket. In the event
the `host` property is an IP address, the lookup is skipped.
@mscdex mscdex added the net Issues and PRs related to the net subsystem. label Apr 22, 2015
debug('connect: find host ' + host);
debug('connect: dns options ' + dnsopts);
self._host = host;
var lookup = options.lookup || dns.lookup;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add type check like

if (typeof lookup !== 'function') {
  throw new TypeError('lookup should be a function.');
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. Added.

assert.equal(err, null);
assert.equal(ip, address);
assert.equal(type, addressType);
ok = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok is unused variable?? need to check the ok is true ??
If ok is unused, remove the variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, check was missing. Added

@evanlucas
Copy link
Contributor Author

@chrisdickinson I'd also like to get your sign-off if you don't mind since you opened the issue

Allows customization of the lookup function used when
Socket.prototype.connect is called using a hostname.
@evanlucas
Copy link
Contributor Author

In this test, cb is always passed, so checking for it is sort of pointless.

@brendanashworth it actually isn't called common.hasIPv6 && check(6); so I switched that back

@evanlucas evanlucas added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 23, 2015
@brendanashworth
Copy link
Contributor

@evanlucas aye! Thanks for catching that, my fault.

@chrisdickinson
Copy link
Contributor

LGTM!

@yosuke-furukawa
Copy link
Member

LGTM2!

evanlucas added a commit that referenced this pull request Apr 24, 2015
Separates out the lookup logic for net.Socket. In the event
the `host` property is an IP address, the lookup is skipped.

PR-URL: #1505
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Yosuke Furukawa <[email protected]>
evanlucas added a commit that referenced this pull request Apr 24, 2015
Allows customization of the lookup function used when
Socket.prototype.connect is called using a hostname.

PR-URL: #1505
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Yosuke Furukawa <[email protected]>
@evanlucas evanlucas closed this Apr 24, 2015
@evanlucas evanlucas deleted the cleanup-connect branch April 24, 2015 12:59
@evanlucas
Copy link
Contributor Author

Thanks, landed in 1bef717 and 4abe2fa

@rvagg rvagg mentioned this pull request Apr 27, 2015
rvagg added a commit that referenced this pull request May 4, 2015
PR-URL: #1532

Notable Changes:

* crypto: significantly reduced memory usage for TLS (Fedor Indutny & Сковорода
  Никита Андреевич) #1529
* net: socket.connect() now accepts a 'lookup' option for a custom DNS
  resolution mechanism, defaults to dns.lookup() (Evan Lucas) #1505
* npm: Upgrade npm to 2.9.0. See the v2.8.4 and v2.9.0 release notes for
  details. Notable items:
  - Add support for default author field to make npm init -y work without
    user-input (@othiym23) npm/npm/d8eee6cf9d
  - Include local modules in npm outdated and npm update (@ArnaudRinquin)
    npm/npm#7426
  - The prefix used before the version number on npm version is now configurable
    via tag-version-prefix (@kkragenbrink) npm/npm#8014
* os: os.tmpdir() is now cross-platform consistent and will no longer returns a
  path with a trailling slash on any platform (Christian Tellnes) #747
* process:
  - process.nextTick() performance has been improved by between 2-42% across the
    benchmark suite, notable because this is heavily used across core (Brian White) #1548
  - New process.geteuid(), process.seteuid(id), process.getegid() and
    process.setegid(id) methods allow you to get and set effective UID and GID
    of the process (Evan Lucas) #1536
* repl:
  - REPL history can be persisted across sessions if the NODE_REPL_HISTORY_FILE
    environment variable is set to a user accessible file,
    NODE_REPL_HISTORY_SIZE can set the maximum history size and defaults to 1000
    (Chris Dickinson) #1513
  - The REPL can be placed in to one of three modes using the NODE_REPL_MODE
    environment variable: sloppy, strict or magic (default); the new magic mode
    will automatically run "strict mode only" statements in strict mode
    (Chris Dickinson) #1513
* smalloc: the 'smalloc' module has been deprecated due to changes coming in V8
  4.4 that will render it unusable
* util: add Promise, Map and Set inspection support (Christopher Monsanto) #1471
* V8: upgrade to 4.2.77.18, see the ChangeLog for full details. Notable items:
  - Classes have moved out of staging; the class keyword is now usable in strict
    mode without flags
  - Object literal enhancements have moved out of staging; shorthand method and
    property syntax is now usable ({ method() { }, property })
  - Rest parameters (function(...args) {}) are implemented in staging behind the
    --harmony-rest-parameters flag
  - Computed property names ({['foo'+'bar']:'bam'}) are implemented in staging
    behind the --harmony-computed-property-names flag
  - Unicode escapes ('\u{xxxx}') are implemented in staging behind the
    --harmony_unicode flag and the --harmony_unicode_regexps flag for use in
    regular expressions
* Windows:
  - Random process termination on Windows fixed (Fedor Indutny) #1512 / #1563
  - The delay-load hook introduced to fix issues with process naming (iojs.exe /
    node.exe) has been made opt-out for native add-ons. Native add-ons should
    include 'win_delay_load_hook': 'false' in their binding.gyp to disable this
    feature if they experience problems . (Bert Belder) #1433
* Governance:
  - Rod Vagg (@rvagg) was added to the Technical Committee (TC)
  - Jeremiah Senkpiel (@Fishrock123) was added to the Technical Committee (TC)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants