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

Wrong legacy API parameter caused by typo in _http_agent.js #5051

Closed
asukakenji opened this issue Feb 3, 2016 · 5 comments
Closed

Wrong legacy API parameter caused by typo in _http_agent.js #5051

asukakenji opened this issue Feb 3, 2016 · 5 comments
Labels
good first issue Issues that are suitable for first-time contributors. http Issues or PRs related to the http subsystem.

Comments

@asukakenji
Copy link

At https://github.com/nodejs/node/blob/master/lib/_http_agent.js#L114:

The line should read:

      localAddress: arguments[3]

instead of:

      path: arguments[3]

The localAddress parameter was added to the addRequest() function in lib/http.js since 1e9bcf2#diff-1c0f1c434b17b7f8795d44a51a14320a.

The function was then refactored into _http_agent.js at 12cd133.

However, the localAddress parameter was mistaken to be path during the refactoring. Below are some evidences showing that the parameter should be localAddress.

Here is the current code in master:

Here is how the options object is used. It clearly expects localAddress:

Here is the code in an old release (v0.10.36, which is the version currently used on AWS):

These clears shows the format host:port:localAddress:

@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Feb 3, 2016
@evanlucas
Copy link
Contributor

This does appear to be the case. Is this breaking something for you?

@asukakenji
Copy link
Author

Yes, of course! If it didn't break anything, why did I spend my time digging into the code, finding the problem, and issuing a bug report?

@cjihrig
Copy link
Contributor

cjihrig commented Feb 3, 2016

@asukakenji would you mind opening a PR, including a regression test?

@asukakenji
Copy link
Author

Of course I would like to do something to help! But it is my first time doing that, would you please point me to some guide lines? Thanks!

@cjihrig
Copy link
Contributor

cjihrig commented Feb 3, 2016

@evanlucas evanlucas added the good first issue Issues that are suitable for first-time contributors. label Feb 9, 2016
dirceu added a commit to dirceu/node that referenced this issue May 10, 2016
Fix `options` usage on `lib/_http_agent.js` for the Legacy API.

Fixes: nodejs#5051
Fishrock123 pushed a commit that referenced this issue May 23, 2016
Fix `options` usage on `lib/_http_agent.js` for the Legacy API.

Fixes: #5051
PR-URL: #5190
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this issue Jun 2, 2016
Fix `options` usage on `lib/_http_agent.js` for the Legacy API.

Fixes: #5051
PR-URL: #5190
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this issue Apr 27, 2017
Removed common.PORT from test-regress-GH-5051 and
test-regress-GH-5727 in order to eliminate the possibility
of port collision.

Refs: #12376
PR-URL: #12639
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
gibfahn pushed a commit that referenced this issue Jun 18, 2017
Removed common.PORT from test-regress-GH-5051 and
test-regress-GH-5727 in order to eliminate the possibility
of port collision.

Refs: #12376
PR-URL: #12639
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
gibfahn pushed a commit that referenced this issue Jun 20, 2017
Removed common.PORT from test-regress-GH-5051 and
test-regress-GH-5727 in order to eliminate the possibility
of port collision.

Refs: #12376
PR-URL: #12639
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
MylesBorins pushed a commit that referenced this issue Jul 11, 2017
Removed common.PORT from test-regress-GH-5051 and
test-regress-GH-5727 in order to eliminate the possibility
of port collision.

Refs: #12376
PR-URL: #12639
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
ryzokuken added a commit to ryzokuken/node that referenced this issue Mar 13, 2018
Rename the tests appropriately alongside mentioning the subsystem
Also, make a few basic changes to make sure the tests conform
to the standard test structure

- Rename test-regress-nodejsGH-9819 to test-crypto-tostring-segfault
- Rename test-regress-nodejsGH-5051 to test-http-addrequest-localaddress
- Rename test-regress-nodejsGH-5727 to test-net-listen-invalid-port
- Rename test-regress-nodejsGH-5927 to test-tty-stdin-pipe
- Rename test-regress-nodejsGH-6235 to test-v8-global-setter

Refs: nodejs#19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
lpinca pushed a commit that referenced this issue Mar 13, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the tests conform to the
standard test structure.

- Rename test-regress-GH-9819 to test-crypto-tostring-segfault
- Rename test-regress-GH-5051 to test-http-addrequest-localaddress
- Rename test-regress-GH-5727 to test-net-listen-invalid-port
- Rename test-regress-GH-5927 to test-tty-stdin-pipe
- Rename test-regress-GH-6235 to test-v8-global-setter

PR-URL: #19275
Refs: #19105
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 20, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the tests conform to the
standard test structure.

- Rename test-regress-GH-9819 to test-crypto-tostring-segfault
- Rename test-regress-GH-5051 to test-http-addrequest-localaddress
- Rename test-regress-GH-5727 to test-net-listen-invalid-port
- Rename test-regress-GH-5927 to test-tty-stdin-pipe
- Rename test-regress-GH-6235 to test-v8-global-setter

PR-URL: #19275
Refs: #19105
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 20, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the tests conform to the
standard test structure.

- Rename test-regress-GH-9819 to test-crypto-tostring-segfault
- Rename test-regress-GH-5051 to test-http-addrequest-localaddress
- Rename test-regress-GH-5727 to test-net-listen-invalid-port
- Rename test-regress-GH-5927 to test-tty-stdin-pipe
- Rename test-regress-GH-6235 to test-v8-global-setter

PR-URL: #19275
Refs: #19105
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the tests conform to the
standard test structure.

- Rename test-regress-nodejsGH-9819 to test-crypto-tostring-segfault
- Rename test-regress-nodejsGH-5051 to test-http-addrequest-localaddress
- Rename test-regress-nodejsGH-5727 to test-net-listen-invalid-port
- Rename test-regress-nodejsGH-5927 to test-tty-stdin-pipe
- Rename test-regress-nodejsGH-6235 to test-v8-global-setter

PR-URL: nodejs#19275
Refs: nodejs#19105
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that are suitable for first-time contributors. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants