Got: HTTP2 support

[gunta]

Hi!

Can you consider adding HTTP2 support to GOT?
Currently the most used client library is this: https://github.com/molnarg/node-http2

Thank you!

---

IssueHunt Summary

szmarczak has been rewarded.

Backers (Total: $200.00)

• issuehunt ($200.00)

Submitted pull Requests

- #1051 Rewrite Got

Tips

• Checkout the Issuehunt explorer to discover more funded issues.
• Need some help from other developers? Add your repositories on IssueHunt to raise funds.
  

[gunta]

Support for it can be detected with this: https://github.com/stefanjudis/is-http2

[julien-f]

As far as I know, the http2 module is not well maintained, you should use spdy instead.

And I don't believe it is needed to detect HTTP2, simply use spdy to make the request and the protocol will be properly negotiated.

@sindresorhus, @floatdrop you could make this optional by using a try {} catch (e) {} (like I did in this module) and documenting that spdy can be installed to enable HTTP2 support.

[gunta]

That's nice to know :)

[julien-f]

I think I've read somewhere that spdy is the HTTP2 implementation that will be merged into Node in the future.

[gunta]

It should, but it seems that is going to take time since there are too many arguing for it/against it.

The try/catch solution or perhaps adding spdy as an optional dependency should be a good solution meanwhile.

[sindresorhus]

Why not just use the agent option?

got('google.com', {agent: require('spdy').createAgent()});

[julien-f]

That's indeed the easiest approach, but it still requires to specify the agent everywhere while the inclusion in got make it transparent…

[SamVerschueren]

Create a spdy-got wrapper module that sets the agent for you? Little bit like gh-got.

[sindresorhus]

I guess we could try/catch it, but let's see what @floatdrop says.

[floatdrop]

@sindresorhus I don't have clear opinion on this right now – there are many things to consider:

• Is current interface of got suitable enough for HTTP2 features?
• What will happen to users, that bundle got to browser?
• Will spdy module increase performance or not?

But I like idea and have service on sight, that could be ported to HTTP2 to test this.

What I'm certain about - this should be major release :smile:

I will take deep look into it on weekend.

[sindresorhus]

@floatdrop To be clear how it would work. Node.js module resolution reads dependencies recursively upwards in the dependency tree. That means we can try/catch trying to require spdy even though we don't directly depend on it, and we'll be able to require it if the user that also depends on got, depends on spdy. I think that's the best way to support it right now.

[sindresorhus]

Relevant: https://github.com/nodejs/node-eps/pull/38

[pietermees]

Seems like this is going to merge soon: https://github.com/nodejs/node/pull/14239

[pietermees]

I was wondering if there was any update on this now that it has made its way into Node 8?

[paambaati]

http/2 has now been in core for a while now. Even though the module's stability is marked as experimental, it would be worthwhile to start looking into this.

[sindresorhus]

I welcome a PR for this, but it's not something I can prioritize right now.

[sindresorhus]

Relevant request issue: https://github.com/request/request/issues/2033

[szmarczak]

https://github.com/szmarczak/http2-wrapper

[sindresorhus]

Relevant Node.js issue about bringing HTTP2 support out of experimental: https://github.com/nodejs/node/issues/19453

[hisco]

I've created an http2-client that completely mimcs the http / https native modules while supporting http2.

It was very easy to integrate it with this module (and a few others) :
http2-got - I literally created this integration module today.

However, it has very few lines of code, all it's behavior comes from : http2-client , got

Hope it somehow helps...

[hisco]

Forgot to say, it's obviously negotiates with the server the correct protocol (ALPN) to choose, https vs http2.

I would be happy to contribute in any way I can.

[sindresorhus]

@hisco You should compare notes with @szmarczak. He made https://github.com/szmarczak/http2-wrapper for the same purpose. Also see: https://github.com/sindresorhus/got#experimental-http2-support

[hisco]

I actually learned on @szmarczak effort a few minutes ago.

@szmarczak It's a very impressive http2 / https compatible implementation - if you would like to compare note or join forces let me know 😄 .

From a very quick look on this source code https://github.com/szmarczak/http2-wrapper I think that it doesn't handle the http2 vs https negotiation.
Therefore, While the two might seem to have the same purpose, the http2-client module have an additional purpose to make the decision of which protocol to use transparent to the user and based on server compatibility.

Yes, that's the kind of integration I made, got is a very easy module to extend =)

Thanks!

[szmarczak]

From a very quick look on this source code szmarczak/http2-wrapper I think that it doesn't handle the http2 vs https negotiation.

I don't think the https module automatically chooses when to use HTTP and when HTTPS, right? Like http is for HTTP only and https is for HTTPS only, so http2-wrapper is for HTTP/2 only.

As you pointed out, it's very easy to implemented the protocol negotiation using ALPN :)

I've created an http2-client that completely mimcs the http / https native modules while supporting http2.

I checked right now, I disagree. It doesn't not mimic the original http/https module behavior when using http2 :)

[hisco]

@szmarczak You are correct =) , my mistake.
However, I do think that ALPN https1 vs https1.1 is negotiated by core modules.
Therefore, I totally agree it cannot actually mimc... I meant to say mimic the interface, while I'm sure there are some differences.

[szmarczak]

Yeah, sure.

got uses modules which use the http/https API. http2-wrapper was created to use http2 like http, so there's no need to change your code when you need to support H2.

[pietermees]

@szmarczak my understanding is that it doesn't yet support connection reuse like you would do with an agent, correct? Persistent connections are an important capability for performance reasons. Is there any plan to add support for that?

[hisco]

@szmarczak If a server supports only H1.1 / H2 I afraid that it will require the client to make changes in his code.

I think that the following should be the analogy:
Should a client code differently to make an https request to https1.1 server vs https1 server.
As such I hope clients shouldn't distinguish between http2 vs http1.1 - only when using compatibility mode of course...

[szmarczak]

If a server supports only H1.1 / H2 I afraid that it will require the client to make changes in his code.

Not necessarily. We could make a TCP connection, get ALPN and make a request with that socket by passing createConnection option. But yeah, it isn't best practice.

[szmarczak]

@pietermees

my understanding is that it doesn't yet support connection reuse like you would do with an agent, correct?

Yes, it does:

there's a session option (accepts ClientHttp2Session) instead of the agent option.

You just need to do:

const session = http2.connect(authority);
const request = wrapper.request({...options, session});
const secondRequest = wrapper.request({...options, session});

[szmarczak]

@pietermees

Example:

const http2 = require('http2');
const {request} = require('http2-wrapper');
const got = require('got');
const h2got = got.extend({request});

const session = http2.connect('https://google.com');

(async () => {
    await h2got('https://google.com/someUrl', {session});
    await h2got('https://google.com/someUrl', {session});
})();

[pietermees]

@szmarczak right, I guess my point was that there is a need for an equivalent helper like Agent to manage ClientHttp2Sessions :)
With http/https 1.1 this is transparently handled for you simply by passing in the right agent option.
This component would have a few responsibilities:

• Establishing a new connection when none is available or max concurrent streams has been reached on the existing one
• Detecting connection failures and re-establishing a new session when needed
• Probably expose both an Agent interface for h1 and a ClientHttp2SessionManager for h2 so that it can be used transparently and handle ALPN for you. It can then establish a connection, figure out through ALPN what the remote host supports, add the connection to that connection pool, and remember this for subsequent requests.

[szmarczak]

@pietermees Could you make a http2-wrapper issue? So I don't forget :)

[pietermees]

@szmarczak https://github.com/szmarczak/http2-wrapper/issues/6

[pietermees]

@szmarczak @sindresorhus if we would like to use https://github.com/zentrick/alpn-agent to negotiate h2 vs https with ALPN, we'd need to make a modification to got so the request extend function doesn't need to synchronously return a response object. We only know what response object we want to use (http2 or https) once ALPN negotiation completes.
Any objections or concerns about making that change?

[szmarczak]

New version of http2-wrapper released: 0.4.0. Many bug fixes etc. Upgrade very recommended. It's much stable now.

Here's Got example for HTTP2 with ALPN negotiation (supports HTTP, HTTPS and HTTP2) :tada:

https://gist.github.com/szmarczak/59db2fa713aa7f52949d343d9cf93ff6

[pietermees]

@szmarczak awesome! I still have a test on my todo list, will let you know ;)

[paambaati]

@szmarczak Thanks for the release! Is there a way I can modify the initial connect request options? I tried setting it in beforeRequest but I get a Parse Error.

https://gist.github.com/paambaati/5e52b5bc42b2e6eebcfb55213d1853a1

[szmarczak]

@paambaati The problem is you are trying pass ClientRequest to options.request, while it should be a function. Just revert your changes and do this:

    const {body: h2body} = await got('https://nghttp2.org/httpbin/headers', {
        json: true,
        headerTableSize: 65536,
        maxConcurrentStreams: 1000,
        initialWindowSize: 6291456
    });

[szmarczak]

[email protected] has dedicated function for resolving ALPN using HTTP request options.
Now, it's much simpler to create Got instance with ALPN negotiation :D

const http2 = require('http2-wrapper');
const {extend: gotExtend} = require('got');

const got = gotExtend({
    hooks: {
        beforeRequest: [
            async options => {
                if (options.protocol === 'https:' && !options.request) {
                    const {alpnProtocol, socket} = await http2.auto.resolveALPN({
                        ...options,
                        resolveSocket: !options.createConnection
                    });

                    if (socket) {
                        options.createConnection = () => socket;
                    }

                    if (alpnProtocol === 'h2') {
                        options.request = http2.request;
                    } else {
                        options.session = options.socketSession;
                    }
                }
            }
        ]
    }
});

[pietermees]

@szmarczak I still feel like haven’t solved the connection caching and reuse question well. As far as I can tell we’ll still establish a new Socket for every request. I’ve tried to address this in alpn-agent and I’ll try to splice that into the example in the next couple of days.

[szmarczak]

@pietermees You're right. It is so, because to solve https://github.com/szmarczak/http2-wrapper/issues/6 session[kState].pendingStreams (+some events) needs to be exposed. Let's raise a Node issue about that.

[pietermees]

Per the continued discussion on that thread, I think there's enough in place to provide a solution today, without changes to Node. But I'll try to prove that out with an example :)

[pietermees]

@szmarczak I've created a demo using got with @zentrick/alpn-agent and http2-wrapper here.

Seems to work great!

You can see debug output by running the script with DEBUG=alpn-agent* env var. You'll see how it caches TLS sessions, reuses connections, and deals transparently with h1 and h2.

[IssueHuntBot]

@issuehunt has funded $200.00 to this issue.

---

• Submit pull request via IssueHunt to receive this reward.
• Want to contribute? Chip in to this issue via IssueHunt.
• Checkout the IssueHunt Issue Explorer to see more funded issues.
• Need help from developers? Add your repository on IssueHunt to raise funds.

[szmarczak]

@pietermees I just figured out that providing agent will cause sockets to be not reused. I don't think we can reuse sockets at all because people want the default Agent to manage the sockets.

Making the H1 Agent use our sockets is just too hard, IMO not worth it. I propose this:

1. Try to retrieve the ALPN protocol from our cache.
2. If the cache's empty, then open a new connection, retrieve the ALPN protocol and close the connection. If someone uses Agent the socket can't be reused either.

[pietermees]

@szmarczak could you explain why agent would prevent sockets from being reused? My understanding is that it's the opposite: agent with keepAlive: true is required to reuse sockets?

I'm concerned an ALPN cache might introduce some unexpected issues and behavior:

1. The same DNS name can point to many IPs, some of which can have one config, and some of which can have another.
2. Similarly, the same IP can change configuration over time, which matters for long running applications that are up for weeks or months.

[szmarczak]

could you explain why agent would prevent sockets from being reused? My understanding is that it's the opposite: agent with keepAlive: true is required to reuse sockets?

I mean it reuses sockets for HTTP2. But to reuse sockets for HTTP1 you need to provide another agent because the H2 agent is not compatible with H1 agent. So in total there will be two sockets.

Reusing H2 sockets for H1 is a bad idea because H2 sockets must not be malformed. There are different settings for H2 and H1...

Another example: imagine all seats for free sessions are taken and you've just finished your request. The session is free and it's being closed. If there's some pending H1 request it will throw an error.

The same DNS name can point to many IPs, some of which can have one config, and some of which can have another.

Good point.

Similarly, the same IP can change configuration over time, which matters for long running applications that are up for weeks or months.

I'll look into this.

[rainder]

@pietermees Thanks for the example. However, there seems to be a serious incompatibility with got implementation. Request just crashes the app on failure. Easy steps to reproduce:

• make runTest('http2-alpn', 'https://some-domain.com') to call every second so that you can make sure connection gets reused
• after few successful requests stop the server for some-domain.com
• get the following error which kills the client app

/Users/endriu/Developer/test/node_modules/@szmarczak/http-timer/source/index.js:58
                socket.once('lookup', lookupListener);
         ^
TypeError: Cannot read property 'once' of undefined
    at HTTP2ClientRequest.request.once.socket (/Users/endriu/Developer/test/node_modules/@szmarczak/http-timer/source/index.js:58:10)
    at Object.onceWrapper (events.js:273:13)
    at HTTP2ClientRequest.emit (events.js:187:15)
    at HTTP2ClientRequest.EventEmitter.emit (domain.js:442:20)
    at HTTP2ClientRequest.origin.emit.args [as emit] (/Users/endriu/Developer/test/node_modules/@szmarczak/http-timer/source/index.js:37:11)
    at HTTP2ClientRequest.process.nextTick (/Users/endriu/Developer/test/node_modules/http2-wrapper/source/client-request.js:112:31)
    at process._tickCallback (internal/process/next_tick.js:61:11)
[nodemon] app crashed - waiting for file changes before starting...

[leodutra]

How is the support for HTTP2?
Stable?

[szmarczak]

It's 99.99% stable, but the PR has not been merged yet due to a servername Node.js bug.
The workaround would be to create a Promise to check every 1ms if the servername is not false, but that doesn't look right.

[leodutra]

@szmarczak absolutely. It smells like no tests were done for that result.

[szmarczak]

@leodutra You're right. I've looked it Node tests right now and it seems they do check false servername, but only for the server-side.

[issuehunt-app[bot]]

@sindresorhus has rewarded $180.00 to @szmarczak. See it on IssueHunt

• :moneybag: Total deposit: $200.00
• :tada: Repository reward(0%): $0.00
• :wrench: Service fee(10%): $20.00