Support http2. #1399

sogaani · 2018-07-31T15:07:10Z

Hi.

This PR enable http/2, if EXPOSE_HTTP2 env is true.

This PR still not support ALPN(as @kornelski mentioned in #980).
I'd prefer to add ALPN functionality after merging this PR and express http/2 PR.

kornelski · 2018-07-31T17:02:15Z

Thank you! That's a very useful functionality.

kornelski · 2018-07-31T17:04:42Z

Does EXPOSE_HTTP2 affect ability to make HTTP/1 requests? Is Node going to fall back automatically? I wonder if we need to give more runtime control over this.

kornelski · 2018-07-31T17:07:32Z

lib/node/http2wrapper.js

+  this._headers = {};
+
+  const session = http2.connect(`${protocol}://${host}:${port}`, sessionOptions);
+  this.setHeader('host', `${host}:${port}`)


Is it necessary here? In normal implementation we allow options.host to be different than host header, because it's useful to do request.get('http://[ip address]').host('example.com') to force request to connect to a particular IP address.

I think current implementation supports request.get('http://[ip address]').host('example.com'). I will check later 👍 .

Still not send header here and we can change Host header by set method. I add test for this.

kornelski · 2018-07-31T17:08:28Z

lib/node/http2wrapper.js

+    case 'http':
+      return net.connect(options.socketPath);
+    case 'https':
+      options.ALPNProtocols = ['h2'];


AFAIK browsers also send http/1 here. Do we support a fallback?

Current implement does not support fall back. I can implement fallback, I plan to take fallback after this PR. Or Should I integrate with this PR?

I see. If there's no fallback, then that's OK.

kornelski · 2018-07-31T17:10:02Z

lib/node/http2wrapper.js

+    switch (key) {
+      case HTTP2_HEADER_HOST:
+        key = HTTP2_HEADER_AUTHORITY;
+        value = value.startsWith('http') ? parse(value).host : value;


Why is such conditional parsing needed?

This is add for compatibility with Http/1. HTTP2_HEADER_AUTHORITY(alternative Host in http/2) only support domain string, otherwise it raise error. However, I met Host header with schema(like Host: http://example.com) in some express tests.

So it would be better to improve the check, otherwise host like httparchive.org would be misparsed :)

/^https?:\/\//.test(value)

kornelski · 2018-07-31T17:12:01Z

test/node/pipe.js

@@ -12,13 +17,19 @@ app.get("/", (req, res) => {
 });

 app.post("/", (req, res) => {
-  res.send(req.body);
+  if (process.env.EXPOSE_HTTP2){
+    res.set('content-type', 'application/json');


Why is this needed?

http/2 module have not full compatibility with http/1 module. http/2 imcommingHttpRequest does not have body property.

Above was my misunderstanding.
Http2 request does not append content-length header if use writableStream. And body-perser does not parse body without content-length. res.send method exported by express automatically add content-type: **json header with object input. However I cannot got body object by body-parser. Therefor I added content-type manually.

kornelski · 2018-07-31T17:13:01Z

test/node/unix-sockets.js

@@ -94,6 +105,7 @@ describe("[unix-sockets] https", () => {
        .get(`${base}/request/path`)
        .ca(cert)
        .end((err, res) => {
+          console.log(err);


My misstake! I will fix 👍

kornelski · 2018-07-31T17:13:32Z

test/node/user-agent.js

@@ -6,7 +6,7 @@ const base = setup.uri;

 describe("req.get()", () => {
  it("should set a default user-agent", () =>
-    request.get(`${base}/ua`).then(res => {
+    request.put(`${base}/ua`).then(res => {


Can you explain this change?

http/2 client module cannot send body with head or get Method. But, it seems to not need here. I check this again later.

I test locally and noticed that this change is not needed. I will revert this change.

kornelski · 2018-07-31T17:14:27Z

test/support/server.js

+  if(process.env.EXPOSE_HTTP2){
+    Object.keys(headers).forEach(function(name) {
+      if(isPseudoHeader(name)){
+        delete headers[name];


Maybe this should translate the header to HTTP/1 header, or allow both in the test using it?

HTTP/2 use some headers to control protocol(e.g. :method, :authority, :path ...) and it can not set by user. I just delete those headers here.

sogaani · 2018-07-31T17:24:15Z

Does EXPOSE_HTTP2 affect ability to make HTTP/1 requests? Is Node going to fall back automatically? I wonder if we need to give more runtime control over this.

Yes. We cannot use HTTP/1 requests, if EXPOSE_HTTP2 is true. Node runtime does not support fall back. Control by env is good for that existing http/1 tests divert to http/2 tests like this PR.
Other runtime control like following might be useful.

request.get('/').http2().end()

kornelski · 2018-07-31T17:34:28Z

@focusaurus do you have a preference how HTTP/1 vs 2 API should look like?

focusaurus · 2018-07-31T18:15:54Z

Hmm. Am I interpreting the discussion correctly? So if a user sets EXPOSE_HTTP2=true they can only connect to http2 servers from that entire node process and connections to http/1 servers will fail? That sounds untenable. I think I'd prefer leaving HTTP/1 as the default and exposing an API to use http/2 on a per-request basis. For that I think the .http2() method name is a good API.

sogaani · 2018-07-31T23:34:53Z

@focusaurus Thank you for feedback!.

I think I'd prefer leaving HTTP/1 as the default and exposing an API to use http/2 on a per-request basis. For that I think the .http2() method name is a good API.

It is reasonable. I change this soon. 👍

sogaani · 2018-08-01T11:34:18Z

I addressed review and change the API to exposing an HTTP/2 API.

Superagent can be enabled http2 by http2 field:

    const request = require('superagent');
    request.http2 =true;
    request
      .get('http://example.com/search')
      .then(res => {

      });

A request can be enabled http2 by http2 method:

    const request = require('superagent');
    request
      .get('http://example.com/search')
      .http2()
      .then(res => {

      });

sogaani · 2018-08-03T02:43:48Z

ping @kornelski @focusaurus

kornelski · 2018-08-03T15:01:31Z

Thanks

focusaurus · 2018-08-03T15:09:23Z

Awesome contribution @sogaani !

Kazuhiro Sogawa and others added 5 commits July 29, 2018 05:41

support http2

2b088d5

Fix http2 tests

32056e5

Delete lockfile

e27e70a

Lint

175671a

Add travis tests

be22f74

kornelski reviewed Jul 31, 2018

View reviewed changes

sogaani added 2 commits August 1, 2018 17:01

address review

d4867a8

Add mechanism to enable http2

0304d41

sogaani mentioned this pull request Aug 2, 2018

Support http2. ladjs/supertest#494

Closed

kornelski merged commit 70bc5ed into ladjs:master Aug 3, 2018

sogaani deleted the http2 branch August 6, 2018 12:10

ejose19 mentioned this pull request Aug 7, 2020

[superagent] add http2 method DefinitelyTyped/DefinitelyTyped#46622

Merged

10 tasks

bj7 mentioned this pull request Feb 5, 2021

Does POST function support http2? #1618

Open

Louis-Tian mentioned this pull request Nov 30, 2022

Does superagent support ALPN for http2 ? #1754

Open

pgayvallet mentioned this pull request May 15, 2024

Add http2 support for Kibana server elastic/kibana#183465

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support http2. #1399

Support http2. #1399

sogaani commented Jul 31, 2018

kornelski commented Jul 31, 2018

kornelski commented Jul 31, 2018

kornelski Jul 31, 2018

sogaani Jul 31, 2018

sogaani Aug 1, 2018

kornelski Jul 31, 2018

sogaani Jul 31, 2018

kornelski Aug 1, 2018

kornelski Jul 31, 2018

sogaani Jul 31, 2018

kornelski Jul 31, 2018

kornelski Jul 31, 2018

sogaani Jul 31, 2018

sogaani Aug 1, 2018

kornelski Jul 31, 2018

sogaani Jul 31, 2018

kornelski Jul 31, 2018

sogaani Jul 31, 2018

sogaani Aug 1, 2018

kornelski Jul 31, 2018

sogaani Jul 31, 2018

sogaani commented Jul 31, 2018 •

edited

kornelski commented Jul 31, 2018 •

edited

focusaurus commented Jul 31, 2018

sogaani commented Jul 31, 2018

sogaani commented Aug 1, 2018

sogaani commented Aug 3, 2018

kornelski commented Aug 3, 2018

focusaurus commented Aug 3, 2018

Support http2. #1399

Support http2. #1399

Conversation

sogaani commented Jul 31, 2018

kornelski commented Jul 31, 2018

kornelski commented Jul 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sogaani commented Jul 31, 2018 • edited

kornelski commented Jul 31, 2018 • edited

focusaurus commented Jul 31, 2018

sogaani commented Jul 31, 2018

sogaani commented Aug 1, 2018

sogaani commented Aug 3, 2018

kornelski commented Aug 3, 2018

focusaurus commented Aug 3, 2018

sogaani commented Jul 31, 2018 •

edited

kornelski commented Jul 31, 2018 •

edited