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

Don't use H2 detection preflights & caching in Node versions where it's not required #49

Closed
pimterry opened this issue Aug 18, 2020 · 16 comments

Comments

@pimterry
Copy link
Contributor

As far as I can tell, the TLS preflight requests and detected protocol caching in https://github.com/szmarczak/http2-wrapper/blob/master/source/auto.js exists purely because of nodejs/node#33343. Is that right?

This bug doesn't exist in many of the recent Node versions (e.g. in my case I'm using Node 14.6). It would be nice to skip all that logic in those cases. That has two main benefits:

  • Saves a duplicate TLS handshake and the associated RTT for the first connections to each host and for hosts that fall out of the cache.
  • No cache invalidation issues. In my case, my tests just started failing because they reuse a port for some test servers, not all of which have the same H2 configuration, so cached initial values broke later requests.
  • Simplifies the code in those cases, and starts towards a path where this can be dropped completely and simplify the code fully (although I know that can't happen any time soon).

I've added some notes on the node issue about the fix. I think this is unnecessary from Node 14.3+, and I'm hopeful we can get it sorted in an v12 release sometime soon too. What do you think?

@szmarczak
Copy link
Owner

Is that right?

If you mean

// https://github.com/nodejs/node/issues/33343
socket.destroy();

then yes.

Saves a duplicate TLS handshake and the associated RTT for the first connections to each host and for hosts that fall out of the cache.

If you know that the server supports HTTP/2 then there's no point in using http2.auto.

No cache invalidation issues. In my case, my tests just started failing because they reuse a port for some test servers, not all of which have the same H2 configuration, so cached initial values broke later requests.

That's not possible. Agent creates a new session on every option change:

const nameKeys = [
// `http2.connect()` options
'maxDeflateDynamicTableSize',
'maxSessionMemory',
'maxHeaderListPairs',
'maxOutstandingPings',
'maxReservedRemoteStreams',
'maxSendHeaderBlockLength',
'paddingStrategy',
// `tls.connect()` options
'localAddress',
'path',
'rejectUnauthorized',
'minDHSize',
// `tls.createSecureContext()` options
'ca',
'cert',
'clientCertEngine',
'ciphers',
'key',
'pfx',
'servername',
'minVersion',
'maxVersion',
'secureProtocol',
'crl',
'honorCipherOrder',
'ecdhCurve',
'dhparam',
'secureOptions',
'sessionIdContext'
];

@pimterry
Copy link
Contributor Author

If you know that the server supports HTTP/2 then there's no point in using http2.auto.

I don't know in advance if the server supports HTTP/2, and I do need http2.auto.

In the case where you use auto and it does happen to support HTTP/2 though, http2-wrapper currently requires two TLS handshakes for the first connection, right?

With this bug fixed, that's not strictly necessary. We could do a single TLS handshake, and then decide whether to do HTTP/1 or HTTP/2 afterwards, without creating a new socket, similar to what the HTTP/1 HTTPS connections already do with installSocket.

This isn't a big problem, but it does seem a bit wasteful to make the connection twice if we don't need to.

That's not possible. Agent creates a new session on every option change

In my case the options aren't changed. I make exactly the same request, and in some tests it goes to an H1-only server, and in some tests it goes to an H2-only server. With http2-wrapper that second request fails.

This is because I have different tests that use many different mock servers, but which often share the exact same URL (because they're test servers, and they just use the first localhost port that's free).

@szmarczak
Copy link
Owner

in some tests it goes to an H1-only server, and in some tests it goes to an H2-only server. With http2-wrapper that second request fails.

http2wrapper.auto.protocolCache.clear();

http2-wrapper currently requires two TLS handshakes for the first connection, right?

Yes, this will be fixed in an upcoming release.

@pimterry
Copy link
Contributor Author

http2wrapper.auto.protocolCache.clear();

Neat, that's definitely helpful, thanks 👍.

Yes, this will be fixed in an upcoming release.

Ok, amazing! That's what I'm proposing really, great to hear that that's planned. Are there details anywhere on how that'll work? Will it work for Node < 14.3 too?

@szmarczak
Copy link
Owner

Are there details anywhere on how that'll work?

I haven't looked deeper into this.

@pimterry
Copy link
Contributor Author

Fair enough. Let me know if there's anything I can do to help.

@szmarczak
Copy link
Owner

szmarczak commented Sep 25, 2020

Done 27d437a although there are still some things to address:

  • some of the code should be moved to https://github.com/szmarczak/resolve-alpn
  • agent.protocol => agent.protocols (an array) (maybe in the future, but not now)
  • before a new release is published, some parts of http2.Agent need to be rebased (e.g. _tryToCreateNewSession(options, origin) => _processQueue())
  • also agent.js coverage needs to hit 100% (including branches).
  • maybe the queue logic should just go down to one layer queue[args] instead of two queue[options][origin]

@szmarczak
Copy link
Owner

If I have the time then I'll do another beta release (so many of them already! :P) this weekend.

@pimterry
Copy link
Contributor Author

Amazing, thanks! Definitely looking forward to the next release, great work 👍

@szmarczak
Copy link
Owner

Actually I'm not sure if it's be possible to reuse TLS sockets for HTTP/2. See nodejs/node#35475

@szmarczak
Copy link
Owner

nodejs/node@75202d9 we just need to wait for an actual release

@szmarczak
Copy link
Owner

szmarczak commented Nov 5, 2020

Node.js v15.1.0 just got released, it should be possible to fix this issue now.

@szmarczak
Copy link
Owner

Meh, nodejs/node#35981

@szmarczak
Copy link
Owner

Node.js v15.3.0 was released yesterday, I'll try to fix this ASAP.

@szmarczak
Copy link
Owner

Fixed on master. Node.js v15.3.0 is required for HTTP/2 socket reuse.

I'll try to make a release this weekend.

@szmarczak
Copy link
Owner

I still keep finding new Node.js bugs hehe :P nodejs/node#34854

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants