GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — net: Supply host argument in the Socket lookup event callback. by entertainyou · Pull Request #5598 · nodejs/node

Conversation

@entertainyou

@entertainyou entertainyou commented Mar 8, 2016

Copy link
Copy Markdown
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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mscdex mscdex added the net Issues and PRs related to the net subsystem. label Mar 8, 2016
@entertainyou

entertainyou commented Mar 8, 2016

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig

cjihrig commented Mar 8, 2016

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@entertainyou

entertainyou commented Mar 8, 2016

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig

cjihrig commented Mar 8, 2016

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@entertainyou

entertainyou commented Mar 8, 2016

Copy link
Copy Markdown
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).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

cjihrig commented Mar 9, 2016

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@evanlucas

evanlucas commented Mar 9, 2016

Copy link
Copy Markdown
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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@entertainyou

entertainyou commented Mar 9, 2016

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Mar 11, 2016

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@evanlucas

evanlucas commented Mar 11, 2016

Copy link
Copy Markdown
Contributor

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

LGTM
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@entertainyou
net: Supply host argument in the Socket lookup event callback.
a26753e
@entertainyou
entertainyou force-pushed the socket_lookup branch from 08f2d8b to a26753e Compare March 16, 2016 03:00
@entertainyou

entertainyou commented Mar 18, 2016

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@r-52

r-52 commented Mar 18, 2016

Copy link
Copy Markdown
Contributor

CI https://ci.nodejs.org/job/node-test-pull-request/1954/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@evanlucas

evanlucas commented Mar 18, 2016

Copy link
Copy Markdown
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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
evanlucas pushed a commit that referenced this pull request Mar 18, 2016
@entertainyou @evanlucas
net: emit host in lookup event
545b8fd 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
@evanlucas

evanlucas commented Mar 18, 2016

Copy link
Copy Markdown
Contributor

Landed in 545b8fd. Thanks for the contribution!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@evanlucas evanlucas closed this Mar 18, 2016
evanlucas pushed a commit that referenced this pull request Mar 30, 2016
@entertainyou @evanlucas
net: emit host in lookup event
d1483f0 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
@evanlucas evanlucas mentioned this pull request Mar 31, 2016
v5.10.0 Release Proposal #5970
Closed
evanlucas pushed a commit that referenced this pull request Mar 31, 2016
@entertainyou @evanlucas
net: emit host in lookup event
b07bc5d 
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
evanlucas added a commit that referenced this pull request Mar 31, 2016
@evanlucas
2016-03-31 Version 5.10.0 (Stable) Release
9cd6e98 
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
@evanlucas
2016-03-31 Version 5.10.0 (Stable) Release
1bb2c4b 
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
@evanlucas
2016-03-31 Version 5.10.0 (Stable) Release
79ea8c3 
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
@evanlucas
2016-03-31 Version 5.10.0 (Stable) Release
64c87e2 
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

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@entertainyou @cjihrig @evanlucas @jasnell @r-52 @mscdex
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.