http: introduce new http Client #34082

ronag · 2020-06-27T07:33:45Z

I'm opening this PR mostly as a forum for discussion and expect it to probably end up being closed. Not sure if this is the right approach so feel free to point me in a better direction if possible.

During the OpenJS conference session yesterday we discussed the future of HTTP. Mostly focused on the server side but also the client side was briefly mentioned. In particular @jasnell pointed out that in HTTP2/3 the difference between server and client side is much smaller. Though, we all agreed that any plans in this regard is probably a few years away. We basically ended up with two issues (#1, #2) in the web-server-frameworks working group. Again this mostly focuses on the server side so I'm a little unsure where client side discussions should go. Given the mentioned decrease in difference, maybe it should land in the web-server-frameworks WG at some point?

That being said, I have been looking into implementing a simpler and faster http client and ended up working on @mcollina's undici library. It might or might not make sense to include in node core or possibly some kind of WG package. The idea is that this client should provide a building block for higher level client implementations.

I think most of us are familiar with the issues with the current http client in Node core and that it can be a challenge to maintain. How do we get past this?

In this PR I have included undici to discuss whether or not it might make sense to include in core, and if so what things are missing, should be changed or needs further consideration?

As a quick summary:

more than 2x faster than the current http client.
Uses llhttp for parsing (through Node).
Simpler implementation.
Testing suite with near 99.99% coverage.
HTTP pipelining support.
5 different API's request, pipeline, stream, upgrade and connect.
Hopefully less edge cases and semi compliant behavior (e.g. in relation to streams).
Async Hook support.
https support
Works on all LTS versions of Node.
keep-alive timeout hint support

For further details it's easiest to check the README.

Some challenges:

What should the API be? Could we fit http2/3 into it?
Should this be in core, WG package or just leave it separate?
Does not sanity check input (i.e. method headers etc...). Should it?
Do we end up with 2 different http client API's in core that we need to maintain indefinitely?
Does not cache TLS sessions. Does it need to?
~~- Missing 1xx support (in particular 101).~~
~~- Missing trailers support.~~
Missing 100 continue support (won't fix is current opinion)
Pool vs Agent? What pool strategies?
Other possible enhancements?

Some TODOs:

Port of tests to Node.
Port of docs to Node.
Port of errors to Node.

Benchmarks:

http - keepalive - pipe x 5,768 ops/sec ±4.17% (71 runs sampled)
undici - pipeline - pipe x 7,151 ops/sec ±2.59% (80 runs sampled)
undici - request - pipe x 11,618 ops/sec ±4.43% (72 runs sampled)
undici - stream - pipe x 12,592 ops/sec ±1.03% (81 runs sampled)

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

ronag · 2020-06-27T07:35:16Z

@nodejs/web-server-frameworks @nodejs/http @nodejs/http2

ghermeto · 2020-06-27T08:44:50Z

cc/ @szmarczak @sindresorhus

mcollina · 2020-06-27T10:14:30Z

I started working on undici to verify what was the overhead of the node core http/1.1 client. Turns out it's quite a lot and undici in its simplest API can do almost 2x throughput of the node core http client. The intent behind that initiative was for undicito get back into Node core one day. Thanks to @ronag for bringing undici forward.

This is intertwined with a lot of other questions, mainly - how many HTTP/1.1 client should we have?

mcollina · 2020-06-27T10:14:44Z

cc @nodejs/tsc

jasnell · 2020-06-27T11:48:35Z

There's a lot here. @ronag, would you be willing to jump on a call next week to discuss in depth?

ronag · 2020-06-30T20:38:50Z

There's a lot here. @ronag, would you be willing to jump on a call next week to discuss in depth?

@jasnell Sure!

ronag · 2020-07-02T08:59:45Z

If reviewing the code in detail please look at https://github.com/mcollina/undici instead of this PR. I'm not keeping the code in this PR up to date at the moment. Waiting for feedback and a plan before initiating that.

ronag · 2020-07-02T09:01:00Z

Some updated benchmark results based on recent updates:

Machine: 2.8GHz AMD EPYC 7402P
Configuration: Node v14.4, HTTP/1.1 without TLS, 100 connections, Linux 5.4.12-1-lts

http - keepalive - pipe x 5,768 ops/sec ±4.17% (71 runs sampled)
undici - pipeline - pipe x 7,151 ops/sec ±2.59% (80 runs sampled)
undici - request - pipe x 11,618 ops/sec ±4.43% (72 runs sampled)
undici - stream - pipe x 12,592 ops/sec ±1.03% (81 runs sampled)

Ethan-Arrowood · 2020-07-02T15:48:25Z

Not sure how the call gets scheduled but I'm interested in participating when it does happen 😄 (or if it has happened already would like to watch a recording)

jasnell · 2020-07-02T23:31:00Z

I ran out of time this week to get it scheduled. I'll get something into the calendar next week and will post a comment here on the details!

masx200 · 2020-07-22T11:36:26Z

nodejs/undici#265

Why is nodejs http client slower than undici?

ronag · 2020-08-08T12:39:38Z

undici now has pretty much feature parity with node core. Only thing missing (as far as I'm aware) and which is currently a won't fix is 100 expect support.

ronag · 2020-08-08T16:02:04Z

Slightly improved perf and also a new and faster low level API (dispatch) which all the others are implemented on top of:

http - keepalive x 5,634 ops/sec ±2.53% (274 runs sampled)
undici - pipeline x 8,642 ops/sec ±3.08% (276 runs sampled)
undici - request x 12,681 ops/sec ±0.51% (279 runs sampled)
undici - stream x 14,006 ops/sec ±0.53% (280 runs sampled)
undici - dispatch x 15,002 ops/sec ±0.39% (278 runs sampled)

Up to 2.67x faster at the moment.

EDIT: Updated 18/8

ronag · 2020-09-06T10:37:47Z

With 2.0 + updated benchmarks which make use if pipelining + unix sockets; we are now almost up to 5x faster:

Node 14

http - keepalive x 6,770 ops/sec ±7.70% (75 runs sampled)
undici - pipeline x 10,627 ops/sec ±5.88% (79 runs sampled)
undici - request x 10,902 ops/sec ±1.28% (85 runs sampled)
undici - stream x 20,144 ops/sec ±0.84% (86 runs sampled)
undici - dispatch x 20,295 ops/sec ±1.00% (83 runs sampled)

Node 15

http - keepalive x 10,337 ops/sec ±6.17% (71 runs sampled)
undici - pipeline x 30,387 ops/sec ±1.37% (80 runs sampled)
undici - request x 40,117 ops/sec ±3.25% (77 runs sampled)
undici - stream x 40,543 ops/sec ±1.30% (80 runs sampled)
undici - dispatch x 50,434 ops/sec ±2.08% (77 runs sampled)

Not sure what we did in Node 15 but it's significantly faster. I would guess it has to do with streams + somewhat synthetic benchmarks.

devsnek · 2020-09-06T12:14:28Z

does this support all http versions of just 1.1?

ronag · 2020-09-06T12:32:11Z

does this support all http versions of just 1.1?

Only 1.1

devsnek · 2020-09-06T13:56:13Z

I think we should get this working with h2 (and maybe h2c) before merging.

ronag · 2020-09-06T14:05:55Z

At the moment there are no plans to add h2 support. h3 was recently briefly discussed but that’s still a year or two away and also would need someone to do the implementation.

addaleax · 2020-09-06T14:11:45Z

lib/http/client/symbols.js

+  kRetryTimeout: Symbol('retry timeout'),
+  kClient: Symbol('client'),
+  kEnqueue: Symbol('enqueue'),
+  kMaxAbortedPayload: Symbol('max aborted payload')


Can this follow the standard symbol style please, i.e. identifier and description identical?

addaleax · 2020-09-06T14:11:45Z

lib/http/client/request.js

+} = primordials;
+
+const { AsyncResource } = require('async_hooks');
+const EE = require('events');


As in the other file:

Suggested change

const EE = require('events');

const EventEmitter = require('events');

(and elsewhere below as well, for readability – most Node.js core contributors would know what EE stands for, others might not)

addaleax · 2020-09-06T14:11:45Z

lib/http/client/client-base.js

@@ -0,0 +1,798 @@
+'use strict';
+
+/* eslint-disable no-restricted-syntax */


Just leaving a comment here to make sure this is resolved at some point :)

(Also, the files should probably all go into lib/internal?)

lib/http/client/client-base.js

ronag · 2020-12-26T10:47:24Z

Quite happy to leave undici outside of core. Gives greater flexibility in development and releases. At the moment I don’t see much advantage in merging it into core other than visibility.

We already have it under the NodeJS organization on GH.

Would maybe be nice to mention it in the HTTP docs and in the future doc deprecate the current client.

Maybe publishing it on npm under @nodejs/undici might be interesting.

@mcollina Thoughts? Can I close this PR?

mcollina · 2020-12-26T18:00:53Z

Let's close this for now. We might want to publicize undici a little bit more.

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jun 27, 2020

ronag added the blocked PRs that are blocked by other issues or PRs. label Jun 27, 2020

ronag force-pushed the http-client branch from 7bc08aa to 038f434 Compare Jun 27, 2020

ronag force-pushed the http-client branch 5 times, most recently from 89c03f3 to 4c36192 Compare Jun 27, 2020

http: introduce new http Client

91871d0

ronag force-pushed the http-client branch from 4c36192 to 91871d0 Compare Jun 27, 2020

ronag changed the title ~~http: introduce new http client.~~ http: introduce new http client Jun 27, 2020

ronag changed the title ~~http: introduce new http client~~ http: introduce new http Client Jun 27, 2020

masx200 mentioned this pull request Jul 22, 2020

Why is nodejs http client slower than undici? nodejs/undici#265

Closed

Ethan-Arrowood mentioned this pull request Jul 30, 2020

Node.js Foundation Web Server Team Meeting 2020-07-30 nodejs/web-server-frameworks#59

Closed

ronag requested a review from a team as a code owner Aug 10, 2020

addaleax reviewed Sep 6, 2020

View changes

ronag closed this Dec 26, 2020

http: introduce new http Client #34082

http: introduce new http Client #34082

ronag commented Jun 27, 2020 •

edited

ronag commented Jun 27, 2020

ghermeto commented Jun 27, 2020

mcollina commented Jun 27, 2020

mcollina commented Jun 27, 2020

jasnell commented Jun 27, 2020

ronag commented Jun 30, 2020

ronag commented Jul 2, 2020 •

edited

ronag commented Jul 2, 2020 •

edited

Ethan-Arrowood commented Jul 2, 2020

jasnell commented Jul 2, 2020

masx200 commented Jul 22, 2020

ronag commented Aug 8, 2020

ronag commented Aug 8, 2020 •

edited

ronag commented Sep 6, 2020 •

edited

devsnek commented Sep 6, 2020

ronag commented Sep 6, 2020

devsnek commented Sep 6, 2020

ronag commented Sep 6, 2020 •

edited

addaleax Sep 6, 2020

addaleax Sep 6, 2020

addaleax Sep 6, 2020

ronag commented Dec 26, 2020

mcollina commented Dec 26, 2020

	const EE = require('events');
	const EventEmitter = require('events');

		@@ -0,0 +1,798 @@
		'use strict';

		/* eslint-disable no-restricted-syntax */

http: introduce new http Client #34082

http: introduce new http Client #34082

Conversation

ronag commented Jun 27, 2020 • edited

Checklist

ronag commented Jun 27, 2020

ghermeto commented Jun 27, 2020

mcollina commented Jun 27, 2020

mcollina commented Jun 27, 2020

jasnell commented Jun 27, 2020

ronag commented Jun 30, 2020

ronag commented Jul 2, 2020 • edited

ronag commented Jul 2, 2020 • edited

Ethan-Arrowood commented Jul 2, 2020

jasnell commented Jul 2, 2020

masx200 commented Jul 22, 2020

ronag commented Aug 8, 2020

ronag commented Aug 8, 2020 • edited

ronag commented Sep 6, 2020 • edited

devsnek commented Sep 6, 2020

ronag commented Sep 6, 2020

devsnek commented Sep 6, 2020

ronag commented Sep 6, 2020 • edited

addaleax Sep 6, 2020

Choose a reason for hiding this comment

addaleax Sep 6, 2020

Choose a reason for hiding this comment

addaleax Sep 6, 2020

Choose a reason for hiding this comment

ronag commented Dec 26, 2020

mcollina commented Dec 26, 2020

ronag commented Jun 27, 2020 •

edited

ronag commented Jul 2, 2020 •

edited

ronag commented Jul 2, 2020 •

edited

ronag commented Aug 8, 2020 •

edited

ronag commented Sep 6, 2020 •

edited

ronag commented Sep 6, 2020 •

edited