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: Supply host argument in the Socket lookup event callback. #5598

Closed
wants to merge 1 commit into from

Conversation

entertainyou
Copy link
Contributor

Pull Request check-list

Please make sure to review and check all of these items:

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to [CONTRIBUTING.md][0]?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Affected core subsystem(s)

net

Description of change

Supply host argument in Socket lookup event callback

Also removes _host field in Socket.

@mscdex mscdex added the net Issues and PRs related to the net subsystem. label Mar 8, 2016
@entertainyou
Copy link
Contributor Author

Reverted the _host remove part, as _host field is used in lib/_tls_wrap.js.

Remove _host makes test/internet/test-tls-reuse-host-from-socket.js fail, but make test does not run that testcase.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 8, 2016

Just out of curiosity, what is your use case for this?

@entertainyou
Copy link
Contributor Author

@cjihrig I want to record my app dns resolve time metric(the host should be part of it).

@cjihrig
Copy link
Contributor

cjihrig commented Mar 8, 2016

Do you not already have access to that data when you setup the socket connection?

@entertainyou
Copy link
Contributor Author

@cjihrig I did not use Socket directly, I use the request library, following is a sample code:

var request = require('request');

var req = request('http://httpbin.org/redirect-to?url=http%3A%2F%2Fexample.com%2F', (res) => {
    console.log('Got response', res);
}).on('socket', socket => {
    socket.on('lookup', (err, ip, type) => {
    console.log('LOOKUP', err, ip);
    });
});

The url requested may have redirects, and I want to record each dns resolution(NOTE that I can not control the url requested).

@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 8, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Mar 9, 2016

I'd like to hear if anyone else has opinions on this. The code changes LGTM though.

@evanlucas
Copy link
Contributor

with the addition of being able to specify a lookup function in net.createConnection, this would also need to update those docs for the function signature. I'm on the fence here. Is this not something that could be done with using a custom lookup function?

@entertainyou
Copy link
Contributor Author

@evanlucas , I don't quite understand, why need to update lookup function signature? The host variable is which we passed to the lookup function, this patch does not modify lookup related code.

@jasnell
Copy link
Member

jasnell commented Mar 11, 2016

@cjihrig ... this LGTM but I don't think it's a high priority.

@evanlucas
Copy link
Contributor

@entertainyou ah disregard my previous comment. Sorry about that.

LGTM

@entertainyou
Copy link
Contributor Author

ping, @evanlucas , it's this PR good to be merge?

@r-52
Copy link
Contributor

r-52 commented Mar 18, 2016

@evanlucas
Copy link
Contributor

One of the build bots went offline. New CI: https://ci.nodejs.org/job/node-test-pull-request/1955/

I'll land it today if the CI is happy. Thanks!

evanlucas pushed a commit that referenced this pull request Mar 18, 2016
Previously, we emitted ip and addressType. This change includes the host
as the last argument to the lookup event.

PR-URL: #5598
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
@evanlucas
Copy link
Contributor

Landed in 545b8fd. Thanks for the contribution!

@evanlucas evanlucas closed this Mar 18, 2016
evanlucas pushed a commit that referenced this pull request Mar 30, 2016
Previously, we emitted ip and addressType. This change includes the host
as the last argument to the lookup event.

PR-URL: #5598
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
evanlucas pushed a commit that referenced this pull request Mar 31, 2016
Previously, we emitted ip and addressType. This change includes the host
as the last argument to the lookup event.

PR-URL: #5598
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
evanlucas added a commit that referenced this pull request Mar 31, 2016
Notable changes:

* buffer:
  * make byteLength work with ArrayBuffer & DataView (Jackson Tian)
[#5255](#5255)
  * backport --zero-fill-buffers command line option (James M Snell)
[#5744](#5744)
  * backport new buffer constructor APIs (James M Snell)
[#5763](#5763)
  * add swap16() and swap32() methods (James M Snell)
[#5724](#5724)
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
[#5333](#5333)
* net: emit host in lookup event (HUANG Wei)
[#5598](#5598)
* node: --no-browser-globals configure flag (Fedor Indutny)
[#5853](#5853)
* npm: Upgrade to v3.8.3. (Forrest L Norvell)
* repl: support standalone blocks (Prince J Wesley)
[#5581](#5581)
* src: override v8 thread defaults using cli options (Tom Gallacher)
[#4344](#4344)
evanlucas added a commit that referenced this pull request Mar 31, 2016
Notable changes:

* buffer:
  * make byteLength work with ArrayBuffer & DataView (Jackson Tian)
[#5255](#5255)
  * backport --zero-fill-buffers command line option (James M Snell)
[#5744](#5744)
  * backport new buffer constructor APIs (James M Snell)
[#5763](#5763)
  * add swap16() and swap32() methods (James M Snell)
[#5724](#5724)
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
[#5333](#5333)
* net: emit host in lookup event (HUANG Wei)
[#5598](#5598)
* node: --no-browser-globals configure flag (Fedor Indutny)
[#5853](#5853)
* npm: Upgrade to v3.8.3. Fixes a security flaw in the use of
authentication tokens in HTTP requests that would allow an attacker to
set up a server that could collect tokens from users of the command-line
interface. Authentication tokens have previously been sent with every
request made by the CLI for logged-in users, regardless of the destination
of the request. This update fixes this by only including those tokens
for requests made against the registry or registries used for the
current install. (Forrest L Norvell)
[npm#6](npm#6)
* repl: support standalone blocks (Prince J Wesley)
[#5581](#5581)
* src: override v8 thread defaults using cli options (Tom Gallacher)
[#4344](#4344)
evanlucas added a commit that referenced this pull request Apr 1, 2016
Notable changes:

* buffer:
  * make byteLength work with ArrayBuffer & DataView (Jackson Tian)
[#5255](#5255)
  * backport --zero-fill-buffers command line option (James M Snell)
[#5744](#5744)
  * backport new buffer constructor APIs (James M Snell)
[#5763](#5763)
  * add swap16() and swap32() methods (James M Snell)
[#5724](#5724)
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
[#5333](#5333)
* net: emit host in lookup event (HUANG Wei)
[#5598](#5598)
* node: --no-browser-globals configure flag (Fedor Indutny)
[#5853](#5853)
* npm: Upgrade to v3.8.3. Fixes a security flaw in the use of
authentication tokens in HTTP requests that would allow an attacker to
set up a server that could collect tokens from users of the command-line
interface. Authentication tokens have previously been sent with every
request made by the CLI for logged-in users, regardless of the destination
of the request. This update fixes this by only including those tokens
for requests made against the registry or registries used for the
current install. (Forrest L Norvell)
[npm#6](npm#6)
* repl: support standalone blocks (Prince J Wesley)
[#5581](#5581)
* src: override v8 thread defaults using cli options (Tom Gallacher)
[#4344](#4344)
evanlucas added a commit that referenced this pull request Apr 1, 2016
Notable changes:

* buffer:
  * make byteLength work with ArrayBuffer & DataView (Jackson Tian)
[#5255](#5255)
  * backport --zero-fill-buffers command line option (James M Snell)
[#5744](#5744)
  * backport new buffer constructor APIs (James M Snell)
[#5763](#5763)
  * add swap16() and swap32() methods (James M Snell)
[#5724](#5724)
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
[#5333](#5333)
* net: emit host in lookup event (HUANG Wei)
[#5598](#5598)
* node: --no-browser-globals configure flag (Fedor Indutny)
[#5853](#5853)
* npm: Upgrade to v3.8.3. Fixes a security flaw in the use of
authentication tokens in HTTP requests that would allow an attacker to
set up a server that could collect tokens from users of the command-line
interface. Authentication tokens have previously been sent with every
request made by the CLI for logged-in users, regardless of the destination
of the request. This update fixes this by only including those tokens
for requests made against the registry or registries used for the
current install. (Forrest L Norvell)
[npm#6](npm#6)
* repl: support standalone blocks (Prince J Wesley)
[#5581](#5581)
* src: override v8 thread defaults using cli options (Tom Gallacher)
[#4344](#4344)

PR-URL: #5970
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.

6 participants