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: add custom lookup function in sockets #14560

Merged
merged 1 commit into from Aug 3, 2017

Conversation

Projects
None yet
9 participants
@cjihrig
Contributor

cjihrig commented Aug 1, 2017

This commit adds support for custom DNS lookup functions in dgram sockets. This is similar to the existing feature in net sockets.

Refs: #6189

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

dgram

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 1, 2017

Member

semver-minor?

Member

Trott commented Aug 1, 2017

semver-minor?

@cjihrig cjihrig added the semver-minor label Aug 1, 2017

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Aug 1, 2017

Contributor

Yes, added the label. Thanks.

Contributor

cjihrig commented Aug 1, 2017

Yes, added the label. Thanks.

@jasnell

jasnell approved these changes Aug 1, 2017

* `reuseAddr` {boolean} When `true` [`socket.bind()`][] will reuse the
address, even if another process has already bound a socket on it. Optional.
Defaults to `false`.
* `lookup` {Function} Custom lookup function. Defaults to [`dns.lookup()`][].

This comment has been minimized.

@geek

geek Aug 1, 2017

Member

Would be nice to provide the function signature

@geek

geek Aug 1, 2017

Member

Would be nice to provide the function signature

This comment has been minimized.

@cjihrig

cjihrig Aug 1, 2017

Contributor

I copied that directly from the net docs for the same feature.

@cjihrig

cjihrig Aug 1, 2017

Contributor

I copied that directly from the net docs for the same feature.

@geek

geek approved these changes Aug 1, 2017

@cjihrig

This comment has been minimized.

Show comment
Hide comment
Contributor

cjihrig commented Aug 2, 2017

cjihrig added a commit to cjihrig/node-1 that referenced this pull request Aug 3, 2017

dgram: add custom lookup function in sockets
This commit adds support for custom DNS lookup functions in
dgram sockets. This is similar to the existing feature in net
sockets.

Refs: nodejs#6189
PR-URL: nodejs#14560
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
dgram: add custom lookup function in sockets
This commit adds support for custom DNS lookup functions in
dgram sockets. This is similar to the existing feature in net
sockets.

Refs: #6189
PR-URL: #14560
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>

@cjihrig cjihrig merged commit 5a05055 into nodejs:master Aug 3, 2017

@cjihrig cjihrig deleted the cjihrig:lookup branch Aug 3, 2017

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Aug 10, 2017

Member

This doesn’t land cleanly on 8.x; if you can, please follow the guide and raise a backport PR, if you don’t think it’s worth it just add the dont-land-on label.

Member

addaleax commented Aug 10, 2017

This doesn’t land cleanly on 8.x; if you can, please follow the guide and raise a backport PR, if you don’t think it’s worth it just add the dont-land-on label.

maclover7 added a commit to maclover7/node that referenced this pull request Sep 17, 2017

doc: do not begin yaml value with backtick
Will break YAML parsing!

Original Node error:

```
Jonathans-MBP:node jon$ node tools/doc/generate.js --format=json doc/api/dgram.md
Input file = doc/api/dgram.md
{ Error
    at generateError (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:165:10)
    at throwError (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:171:9)
    at readBlockSequence (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:935:7)
    at composeNode (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1331:12)
    at readBlockMapping (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1062:11)
    at composeNode (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1332:12)
    at readDocument (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1492:3)
    at loadDocuments (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1548:5)
    at load (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1569:19)
    at Object.safeLoad (/Users/jon/code/nodejs/node/tools/eslint/node_modules/js-yaml/lib/js-yaml/loader.js:1591:10)
  name: 'YAMLException',
  reason: 'bad indentation of a sequence entry',
  mark:
   Mark {
     name: null,
     buffer: '\nadded: v0.11.13\nchanges:\n  - version: REPLACEME\n    pr-url: nodejs#14560    description: The `lookup` option is supported.\n  - version: REPLACEME\n    pr-url: nodejs#13623    description: `recvBufferSize` and `sendBufferSize` options are supported now.\n\u0000',
     position: 248,
     line: 8,
     column: 17 },
  message: 'bad indentation of a sequence entry at line 9, column 18:\n        description: `recvBufferSize` and `sendBuffer ... \n                     ^' }
```

Then I extracted out the problematic YAML, and tried running through Ruby as a
separate `.yml` file:

```
Psych::SyntaxError: (<unknown>): found character that cannot start any token while scanning for the next token at line 8 column 18
	from /Users/jon/.rbenv/versions/2.4.0/lib/ruby/2.4.0/psych.rb:377:in `parse'
	from /Users/jon/.rbenv/versions/2.4.0/lib/ruby/2.4.0/psych.rb:377:in `parse_stream'
	from /Users/jon/.rbenv/versions/2.4.0/lib/ruby/2.4.0/psych.rb:325:in `parse'
	from /Users/jon/.rbenv/versions/2.4.0/lib/ruby/2.4.0/psych.rb:252:in `load'
	from (irb):7
	from /Users/jon/.rbenv/versions/2.4.0/bin/irb:11:in `<main>'
```

@jasnell jasnell referenced this pull request Sep 20, 2017

Closed

Dgram buffers #13623

4 of 4 tasks complete
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Sep 20, 2017

Member

Ping @cjihrig ... Initial basic attempts to backport this for v8.x are failing due to some issue with dns. Will investigate further shortly.

Member

jasnell commented Sep 20, 2017

Ping @cjihrig ... Initial basic attempts to backport this for v8.x are failing due to some issue with dns. Will investigate further shortly.

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Sep 22, 2017

Contributor

@jasnell not sure. The commit in this PR lands cleanly for me on v8.x-staging. I ran the test suite locally and only saw one unrelated failure (test/sequential/test-benchmark-http.js).

Contributor

cjihrig commented Sep 22, 2017

@jasnell not sure. The commit in this PR lands cleanly for me on v8.x-staging. I ran the test suite locally and only saw one unrelated failure (test/sequential/test-benchmark-http.js).

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Sep 22, 2017

Member

It's likely that the commits I landed today unblocked it. I'll try again tomorrow

Member

jasnell commented Sep 22, 2017

It's likely that the commits I landed today unblocked it. I'll try again tomorrow

jasnell added a commit that referenced this pull request Sep 25, 2017

dgram: add custom lookup function in sockets
This commit adds support for custom DNS lookup functions in
dgram sockets. This is similar to the existing feature in net
sockets.

Refs: #6189
PR-URL: #14560
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>

jasnell added a commit that referenced this pull request Sep 26, 2017

2017-09-26, Node.js Version 8.6.0 (Current)
* **crypto**
  * Support for multiple ECDH curves. [#15206](#15206)
* **dgram**
  * Added `setMulticastInterface()` API. [#7855](#7855)
  * Custom lookup functions are now supported. [#14560](#14560)
* **n-api**
  * The command-line flag is no longer required to use N-API. [#14902](#14902)
* **tls**
  * Docs-only deprecation of `parseCertString()`. [#14245](#14245)
* **New Contributors**
  * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)

jasnell added a commit that referenced this pull request Sep 26, 2017

2017-09-26, Node.js Version 8.6.0 (Current)
* **crypto**
  * Support for multiple ECDH curves. [#15206](#15206)
* **dgram**
  * Added `setMulticastInterface()` API. [#7855](#7855)
  * Custom lookup functions are now supported. [#14560](#14560)
* **n-api**
  * The command-line flag is no longer required to use N-API. [#14902](#14902)
* **tls**
  * Docs-only deprecation of `parseCertString()`. [#14245](#14245)
* **New Contributors**
  * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)

jasnell added a commit that referenced this pull request Sep 26, 2017

2017-09-26, Node.js Version 8.6.0 (Current)
* **crypto**
  * Support for multiple ECDH curves. [#15206](#15206)
* **dgram**
  * Added `setMulticastInterface()` API. [#7855](#7855)
  * Custom lookup functions are now supported. [#14560](#14560)
* **n-api**
  * The command-line flag is no longer required to use N-API. [#14902](#14902)
* **tls**
  * Docs-only deprecation of `parseCertString()`. [#14245](#14245)
* **New Contributors**
  * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)

jasnell added a commit that referenced this pull request Sep 26, 2017

2017-09-26, Node.js Version 8.6.0 (Current)
* **crypto**
  * Support for multiple ECDH curves. [#15206](#15206)
* **dgram**
  * Added `setMulticastInterface()` API. [#7855](#7855)
  * Custom lookup functions are now supported. [#14560](#14560)
* **n-api**
  * The command-line flag is no longer required to use N-API. [#14902](#14902)
* **tls**
  * Docs-only deprecation of `parseCertString()`. [#14245](#14245)
* **New Contributors**
  * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)

jasnell added a commit that referenced this pull request Sep 26, 2017

2017-09-26, Node.js Version 8.6.0 (Current)
* **crypto**
  * Support for multiple ECDH curves. [#15206](#15206)
* **dgram**
  * Added `setMulticastInterface()` API. [#7855](#7855)
  * Custom lookup functions are now supported. [#14560](#14560)
* **n-api**
  * The command-line flag is no longer required to use N-API. [#14902](#14902)
* **tls**
  * Docs-only deprecation of `parseCertString()`. [#14245](#14245)
* **New Contributors**
  * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)

MylesBorins added a commit that referenced this pull request Sep 29, 2017

2017-09-26, Node.js Version 8.6.0 (Current)
* **crypto**
  * Support for multiple ECDH curves. [#15206](#15206)
* **dgram**
  * Added `setMulticastInterface()` API. [#7855](#7855)
  * Custom lookup functions are now supported. [#14560](#14560)
* **n-api**
  * The command-line flag is no longer required to use N-API. [#14902](#14902)
* **tls**
  * Docs-only deprecation of `parseCertString()`. [#14245](#14245)
* **New Contributors**
  * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](#15354)

addaleax added a commit to addaleax/ayo that referenced this pull request Sep 30, 2017

2017-09-26, Node.js Version 8.6.0 (Current)
* **crypto**
  * Support for multiple ECDH curves. [#15206](nodejs/node#15206)
* **dgram**
  * Added `setMulticastInterface()` API. [#7855](nodejs/node#7855)
  * Custom lookup functions are now supported. [#14560](nodejs/node#14560)
* **n-api**
  * The command-line flag is no longer required to use N-API. [#14902](nodejs/node#14902)
* **tls**
  * Docs-only deprecation of `parseCertString()`. [#14245](nodejs/node#14245)
* **New Contributors**
  * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](nodejs/node#15354)
@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Jan 15, 2018

Member

Release team were -1 on landing on v6.x, if you disagree let us know.

Member

gibfahn commented Jan 15, 2018

Release team were -1 on landing on v6.x, if you disagree let us know.

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 15, 2018

2017-09-26, Node.js Version 8.6.0 (Current)
* **crypto**
  * Support for multiple ECDH curves. [#15206](nodejs#15206)
* **dgram**
  * Added `setMulticastInterface()` API. [#7855](nodejs#7855)
  * Custom lookup functions are now supported. [#14560](nodejs#14560)
* **n-api**
  * The command-line flag is no longer required to use N-API. [#14902](nodejs#14902)
* **tls**
  * Docs-only deprecation of `parseCertString()`. [#14245](nodejs#14245)
* **New Contributors**
  * Welcome Sebastiaan Deckers (@sebdeckers) as a new Collaborator! [#15354](nodejs#15354)

This applies parts of 05e4c1d thar are
relevant to N-API.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment