http: should support userland Agent #11567

fengmk2 · 2017-02-26T17:15:42Z

#10053 assert agent must be instance of http.Agent, it make userland Agents are broken. (e.g.: https://github.com/request/tunnel-agent/blob/master/index.js#L47)

I think we should keep userland Agents still can work on the future is very important.

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)

http

fengmk2 · 2017-02-26T17:22:21Z

It's hard to inherits from http.Agent to implement a sub class Agent in the real world.

So the userland Agent implement the methods of http.Agent and make itself look like Agent class.

joyeecheung · 2017-02-26T17:26:06Z

On a side note: seeing the download stats of tunnel agent I think it should be added to citgm, but it doesn't have a test...¯\(ツ)/¯

EDIT: looks like it's tested in the tests of request, don't know why it is not failing citgm..

jasnell · 2017-02-27T01:05:19Z

@nodejs/http @bnoordhuis @indutny ... thoughts?

mscdex · 2017-02-27T01:09:23Z

Why can't such models just inherit from http.Agent? I would've thought they should've already been doing that? Especially since http.Agent keeps internal state of its own, so you'd need to call its constructor (implicitly or explicitly), which is important because various parts of http (currently _http_client.js) look for certain properties set by the http.Agent constructor.

bnoordhuis

We don't explicitly say in the docs that you should subclass http.Agent. We suggest it in one place but not very explicitly.

On the other hand, if you don't subclass, you must have reverse-engineered its implementation from node's sources, so you probably knew what you were getting yourself into.

I'm undecided if we should support that although I suppose it's Javascript-y to do so.

bnoordhuis · 2017-02-27T05:28:39Z

lib/_http_client.js

@@ -68,7 +68,8 @@ function ClientRequest(options, cb) {
    }
    // Explicitly pass through this statement as agent will not be used
    // when createConnection is provided.
-  } else if (!(agent instanceof Agent.Agent)) {
+  } else if (!(agent instanceof Agent.Agent) &&
+            typeof agent.addRequest !== 'function') {


If you're going to do this, you might as well remove the instanceof check.

Also needs a change to the error message two lines below, which will make this semver-major.

fengmk2 · 2017-02-27T06:57:03Z

@mscdex Because http.Agent constructor will execute a lot logic like handle free socket https://github.com/nodejs/node/blob/master/lib/_http_agent.js#L43 . And every userland Agent need to remove that listener then use a new listener instead of it.

Maybe we need to refactor the http.Agent constructor and make subclass more easy inherit from it.

fengmk2 · 2017-02-27T08:08:19Z

@mscdex See the other pull request to refactor http.Agent constructor #11577

And the new TunnelingAgent implement #11577 (comment)

fengmk2 · 2017-03-04T17:09:48Z

We should support userland Agent before node 8 release.

bnoordhuis

LGTM. I regret I can't start the CI right now, it's timing out.

gibfahn · 2017-03-05T19:06:02Z

CI: https://ci.nodejs.org/job/node-test-commit/8281/

fengmk2 · 2017-03-17T05:24:19Z

ping @nodejs/http

fhinkel · 2017-03-26T09:55:28Z

CI once more: https://ci.nodejs.org/job/node-test-pull-request/7043/

jasnell · 2017-03-26T19:20:42Z

CI failures appear unrelated

PR-URL: #11567 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

jasnell · 2017-03-26T20:20:21Z

Landed in 90403dd as a semver-major

* **Async Hooks** * The `async_hooks` module has landed in core [[`4a7233c178`](nodejs@4a7233c178)] [nodejs#12892](nodejs#12892). * **Buffer** * Using the `--pending-deprecation` flag will cause Node.js to emit a deprecation warning when using `new Buffer(num)` or `Buffer(num)`. [[`d2d32ea5a2`](nodejs@d2d32ea5a2)] [nodejs#11968](nodejs#11968). * `new Buffer(num)` and `Buffer(num)` will zero-fill new `Buffer` instances [[`7eb1b4658e`](nodejs@7eb1b4658e)] [nodejs#12141](nodejs#12141). * Many `Buffer` methods now accept `Uint8Array` as input [[`beca3244e2`](nodejs@beca3244e2)] [nodejs#10236](nodejs#10236). * **Child Process** * Argument and kill signal validations have been improved [[`97a77288ce`](nodejs@97a77288ce)] [nodejs#12348](nodejs#12348), [[`d75fdd96aa`](nodejs@d75fdd96aa)] [nodejs#10423](nodejs#10423). * Child Process methods accept `Uint8Array` as input [[`627ecee9ed`](nodejs@627ecee9ed)] [nodejs#10653](nodejs#10653). * **Console** * Error events emitted when using `console` methods are now supressed. [[`f18e08d820`](nodejs@f18e08d820)] [nodejs#9744](nodejs#9744). * **Dependencies** * The npm client has been updated to 5.0.0 [[`3c3b36af0f`](nodejs@3c3b36af0f)] [nodejs#12936](nodejs#12936). * V8 has been updated to 5.8 with forward ABI stability to 6.0 [[`60d1aac8d2`](nodejs@60d1aac8d2)] [nodejs#12784](nodejs#12784). * **Domains** * Native `Promise` instances are now `Domain` aware [[`84dabe8373`](nodejs@84dabe8373)] [nodejs#12489](nodejs#12489). * **Errors** * We have started assigning static error codes to errors generated by Node.js. This has been done through multiple commits and is still a work in progress. * **File System** * The utility class `fs.SyncWriteStream` has been deprecated [[`7a55e34ef4`](nodejs@7a55e34ef4)] [nodejs#10467](nodejs#10467). * The deprecated `fs.read()` string interface has been removed [[`3c2a9361ff`](nodejs@3c2a9361ff)] [nodejs#9683](nodejs#9683). * **HTTP** * Improved support for userland implemented Agents [[`90403dd1d0`](nodejs@90403dd1d0)] [nodejs#11567](nodejs#11567). * Outgoing Cookie headers are concatenated into a single string [[`d3480776c7`](nodejs@d3480776c7)] [nodejs#11259](nodejs#11259). * The `httpResponse.writeHeader()` method has been deprecated [[`fb71ba4921`](nodejs@fb71ba4921)] [nodejs#11355](nodejs#11355). * New methods for accessing HTTP headers have been added to `OutgoingMessage` [[`3e6f1032a4`](nodejs@3e6f1032a4)] [nodejs#10805](nodejs#10805). * **Lib** * All deprecation messages have been assigned static identifiers [[`5de3cf099c`](nodejs@5de3cf099c)] [nodejs#10116](nodejs#10116). * The legacy `linkedlist` module has been removed [[`84a23391f6`](nodejs@84a23391f6)] [nodejs#12113](nodejs#12113). * **N-API** * Experimental support for the new N-API API has been added [[`56e881d0b0`](nodejs@56e881d0b0)] [nodejs#11975](nodejs#11975). * **Process** * Process warning output can be redirected to a file using the `--redirect-warnings` command-line argument [[`03e89b3ff2`](nodejs@03e89b3ff2)] [nodejs#10116](nodejs#10116). * Process warnings may now include additional detail [[`dd20e68b0f`](nodejs@dd20e68b0f)] [nodejs#12725](nodejs#12725). * **REPL** * REPL magic mode has been deprecated [[`3f27f02da0`](nodejs@3f27f02da0)] [nodejs#11599](nodejs#11599). * **Src** * `NODE_MODULE_VERSION` has been updated to 57 (nodejs@ec7cbaf266)] [nodejs#12995](nodejs#12995). * Add `--pending-deprecation` command-line argument and `NODE_PENDING_DEPRECATION` environment variable [[`a16b570f8c`](nodejs@a16b570f8c)] [nodejs#11968](nodejs#11968). * The `--debug` command-line argument has been deprecated. Note that using `--debug` will enable the *new* Inspector-based debug protocol as the legacy Debugger protocol previously used by Node.js has been removed. [[`010f864426`](nodejs@010f864426)] [nodejs#12949](nodejs#12949). * Throw when the `-c` and `-e` command-line arguments are used at the same time [[`a5f91ab230`](nodejs@a5f91ab230)] [nodejs#11689](nodejs#11689). * Throw when the `--use-bundled-ca` and `--use-openssl-ca` command-line arguments are used at the same time. [[`8a7db9d4b5`](nodejs@8a7db9d4b5)] [nodejs#12087](nodejs#12087). * **Stream** * `Stream` now supports `destroy()` and `_destroy()` APIs [[`b6e1d22fa6`](nodejs@b6e1d22fa6)] [nodejs#12925](nodejs#12925). * `Stream` now supports the `_final()` API [[`07c7f198db`](nodejs@07c7f198db)] [nodejs#12828](nodejs#12828). * **TLS** * The `rejectUnauthorized` option now defaults to `true` [[`348cc80a3c`](nodejs@348cc80a3c)] [nodejs#5923](nodejs#5923). * The `tls.createSecurePair()` API now emits a runtime deprecation [[`a2ae08999b`](nodejs@a2ae08999b)] [nodejs#11349](nodejs#11349). * A runtime deprecation will now be emitted when `dhparam` is less than 2048 bits [[`d523eb9c40`](nodejs@d523eb9c40)] [nodejs#11447](nodejs#11447). * **URL** * The WHATWG URL implementation is now a fully-supported Node.js API [[`d080ead0f9`](nodejs@d080ead0f9)] [nodejs#12710](nodejs#12710). * **Util** * `Symbol` keys are now displayed by default when using `util.inspect()` [[`5bfd13b81e`](nodejs@5bfd13b81e)] [nodejs#9726](nodejs#9726). * `toJSON` errors will be thrown when formatting `%j` [[`455e6f1dd8`](nodejs@455e6f1dd8)] [nodejs#11708](nodejs#11708). * Convert `inspect.styles` and `inspect.colors` to prototype-less objects [[`aab0d202f8`](nodejs@aab0d202f8)] [nodejs#11624](nodejs#11624). * The new `util.promisify()` API has been added [[`99da8e8e02`](nodejs@99da8e8e02)] [nodejs#12442](nodejs#12442). * **Zlib** * Support `Uint8Array` in Zlib convenience methods [[`91383e47fd`](nodejs@91383e47fd)] [nodejs#12001](nodejs#12001). * Zlib errors now use `RangeError` and `TypeError` consistently [[`b514bd231e`](nodejs@b514bd231e)] [nodejs#11391](nodejs#11391).

* **Async Hooks** * The `async_hooks` module has landed in core [[`4a7233c178`](4a7233c178)] [#12892](#12892). * **Buffer** * Using the `--pending-deprecation` flag will cause Node.js to emit a deprecation warning when using `new Buffer(num)` or `Buffer(num)`. [[`d2d32ea5a2`](d2d32ea5a2)] [#11968](#11968). * `new Buffer(num)` and `Buffer(num)` will zero-fill new `Buffer` instances [[`7eb1b4658e`](7eb1b4658e)] [#12141](#12141). * Many `Buffer` methods now accept `Uint8Array` as input [[`beca3244e2`](beca3244e2)] [#10236](#10236). * **Child Process** * Argument and kill signal validations have been improved [[`97a77288ce`](97a77288ce)] [#12348](#12348), [[`d75fdd96aa`](d75fdd96aa)] [#10423](#10423). * Child Process methods accept `Uint8Array` as input [[`627ecee9ed`](627ecee9ed)] [#10653](#10653). * **Console** * Error events emitted when using `console` methods are now supressed. [[`f18e08d820`](f18e08d820)] [#9744](#9744). * **Dependencies** * The npm client has been updated to 5.0.0 [[`3c3b36af0f`](3c3b36af0f)] [#12936](#12936). * V8 has been updated to 5.8 with forward ABI stability to 6.0 [[`60d1aac8d2`](60d1aac8d2)] [#12784](#12784). * **Domains** * Native `Promise` instances are now `Domain` aware [[`84dabe8373`](84dabe8373)] [#12489](#12489). * **Errors** * We have started assigning static error codes to errors generated by Node.js. This has been done through multiple commits and is still a work in progress. * **File System** * The utility class `fs.SyncWriteStream` has been deprecated [[`7a55e34ef4`](7a55e34ef4)] [#10467](#10467). * The deprecated `fs.read()` string interface has been removed [[`3c2a9361ff`](3c2a9361ff)] [#9683](#9683). * **HTTP** * Improved support for userland implemented Agents [[`90403dd1d0`](90403dd1d0)] [#11567](#11567). * Outgoing Cookie headers are concatenated into a single string [[`d3480776c7`](d3480776c7)] [#11259](#11259). * The `httpResponse.writeHeader()` method has been deprecated [[`fb71ba4921`](fb71ba4921)] [#11355](#11355). * New methods for accessing HTTP headers have been added to `OutgoingMessage` [[`3e6f1032a4`](3e6f1032a4)] [#10805](#10805). * **Lib** * All deprecation messages have been assigned static identifiers [[`5de3cf099c`](5de3cf099c)] [#10116](#10116). * The legacy `linkedlist` module has been removed [[`84a23391f6`](84a23391f6)] [#12113](#12113). * **N-API** * Experimental support for the new N-API API has been added [[`56e881d0b0`](56e881d0b0)] [#11975](#11975). * **Process** * Process warning output can be redirected to a file using the `--redirect-warnings` command-line argument [[`03e89b3ff2`](03e89b3ff2)] [#10116](#10116). * Process warnings may now include additional detail [[`dd20e68b0f`](dd20e68b0f)] [#12725](#12725). * **REPL** * REPL magic mode has been deprecated [[`3f27f02da0`](3f27f02da0)] [#11599](#11599). * **Src** * `NODE_MODULE_VERSION` has been updated to 57 (ec7cbaf266)] [#12995](#12995). * Add `--pending-deprecation` command-line argument and `NODE_PENDING_DEPRECATION` environment variable [[`a16b570f8c`](a16b570f8c)] [#11968](#11968). * The `--debug` command-line argument has been deprecated. Note that using `--debug` will enable the *new* Inspector-based debug protocol as the legacy Debugger protocol previously used by Node.js has been removed. [[`010f864426`](010f864426)] [#12949](#12949). * Throw when the `-c` and `-e` command-line arguments are used at the same time [[`a5f91ab230`](a5f91ab230)] [#11689](#11689). * Throw when the `--use-bundled-ca` and `--use-openssl-ca` command-line arguments are used at the same time. [[`8a7db9d4b5`](8a7db9d4b5)] [#12087](#12087). * **Stream** * `Stream` now supports `destroy()` and `_destroy()` APIs [[`b6e1d22fa6`](b6e1d22fa6)] [#12925](#12925). * `Stream` now supports the `_final()` API [[`07c7f198db`](07c7f198db)] [#12828](#12828). * **TLS** * The `rejectUnauthorized` option now defaults to `true` [[`348cc80a3c`](348cc80a3c)] [#5923](#5923). * The `tls.createSecurePair()` API now emits a runtime deprecation [[`a2ae08999b`](a2ae08999b)] [#11349](#11349). * A runtime deprecation will now be emitted when `dhparam` is less than 2048 bits [[`d523eb9c40`](d523eb9c40)] [#11447](#11447). * **URL** * The WHATWG URL implementation is now a fully-supported Node.js API [[`d080ead0f9`](d080ead0f9)] [#12710](#12710). * **Util** * `Symbol` keys are now displayed by default when using `util.inspect()` [[`5bfd13b81e`](5bfd13b81e)] [#9726](#9726). * `toJSON` errors will be thrown when formatting `%j` [[`455e6f1dd8`](455e6f1dd8)] [#11708](#11708). * Convert `inspect.styles` and `inspect.colors` to prototype-less objects [[`aab0d202f8`](aab0d202f8)] [#11624](#11624). * The new `util.promisify()` API has been added [[`99da8e8e02`](99da8e8e02)] [#12442](#12442). * **Zlib** * Support `Uint8Array` in Zlib convenience methods [[`91383e47fd`](91383e47fd)] [#12001](#12001). * Zlib errors now use `RangeError` and `TypeError` consistently [[`b514bd231e`](b514bd231e)] [#11391](#11391).

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Feb 26, 2017

bnoordhuis reviewed Feb 27, 2017

View reviewed changes

fengmk2 force-pushed the support-userland-agent branch 2 times, most recently from c409b18 to b2043e6 Compare February 27, 2017 07:08

http: should support userland Agent

f17d533

fengmk2 force-pushed the support-userland-agent branch from b2043e6 to f17d533 Compare February 27, 2017 07:21

jasnell approved these changes Mar 4, 2017

View reviewed changes

bnoordhuis approved these changes Mar 5, 2017

View reviewed changes

cjihrig approved these changes Mar 6, 2017

View reviewed changes

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 26, 2017

jasnell pushed a commit that referenced this pull request Mar 26, 2017

http: should support userland Agent

90403dd

PR-URL: #11567 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

jasnell closed this Mar 26, 2017

fengmk2 deleted the support-userland-agent branch March 29, 2017 15:40

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

zkat mentioned this pull request May 4, 2017

npm5: Agent option must be an instance of http.Agent, undefined or false. npm/npm#16530

Closed

1 task

lpinca mentioned this pull request May 27, 2017

http: subclass of Agent can override the initialization #11577

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: should support userland Agent #11567

http: should support userland Agent #11567

fengmk2 commented Feb 26, 2017 •

edited

Loading

fengmk2 commented Feb 26, 2017

joyeecheung commented Feb 26, 2017 •

edited

Loading

jasnell commented Feb 27, 2017

mscdex commented Feb 27, 2017 •

edited

Loading

bnoordhuis left a comment

bnoordhuis Feb 27, 2017

Trott Feb 27, 2017

fengmk2 commented Feb 27, 2017

fengmk2 commented Feb 27, 2017 •

edited

Loading

fengmk2 commented Mar 4, 2017

bnoordhuis left a comment

gibfahn commented Mar 5, 2017

fengmk2 commented Mar 17, 2017

fhinkel commented Mar 26, 2017

jasnell commented Mar 26, 2017

jasnell commented Mar 26, 2017

http: should support userland Agent #11567

http: should support userland Agent #11567

Conversation

fengmk2 commented Feb 26, 2017 • edited Loading

Checklist

Affected core subsystem(s)

fengmk2 commented Feb 26, 2017

joyeecheung commented Feb 26, 2017 • edited Loading

jasnell commented Feb 27, 2017

mscdex commented Feb 27, 2017 • edited Loading

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Feb 27, 2017

Choose a reason for hiding this comment

Trott Feb 27, 2017

Choose a reason for hiding this comment

fengmk2 commented Feb 27, 2017

fengmk2 commented Feb 27, 2017 • edited Loading

fengmk2 commented Mar 4, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

gibfahn commented Mar 5, 2017

fengmk2 commented Mar 17, 2017

fhinkel commented Mar 26, 2017

jasnell commented Mar 26, 2017

jasnell commented Mar 26, 2017

fengmk2 commented Feb 26, 2017 •

edited

Loading

joyeecheung commented Feb 26, 2017 •

edited

Loading

mscdex commented Feb 27, 2017 •

edited

Loading

fengmk2 commented Feb 27, 2017 •

edited

Loading