-
Notifications
You must be signed in to change notification settings - Fork 30k
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
We "need" a way to create an Http2Session from a socket #16256
Comments
It’s not 100 % clear what you are asking for, but this feels like something one should already be able to do using existing APIs… Just to clarify, the TLS module’s |
Right. After that callback you have two things: A socket and a protocol name. How do you then go about to turn this socket into an Http2Session (or something on which you can perform HTTP/1.1 requests)? That's what I'm after. Those things seem very internal to Node.js and not exposed by the current API's (disclaimer: I might have overlooked them). |
Right – I think it should be possible to create HTTP or HTTP2 sessions from arbitrary streams, or at least I think the internals structures of HTTP were made with such a use case in mind… Edit: I’ll definitely be looking into this later today and make sure |
Sounds great, I didn't think about it, but perhaps this already works for http1 by setting the For It does sound like this could be a pretty simple fix with a huge impact, and given the bug #14671 I think this issue is more important to look at, since by allowing the user to first perform TLS handshake and then handover to |
The But if you were trying to do that right now, you would need (No one has really shown that #14671 is a real bug. As far as I can tell people are just not passing in the right options to |
@apapirovski How do I create an Http2Session from a socket? Did you read my comment in #14671? If you still mean that I haven't shown that it's a real bug, please explain there what I do wrong. |
So... we don't actually export const https = require('https');
const {
constants,
Http2Session
} = require('http2');
const tls = require('tls');
const tlsConnection = tls.connect({
host: 'nghttp2.org',
port: 443,
ALPNProtocols: ['h2', 'http/1.1']
});
tlsConnection.once('secureConnect', () => {
if (tlsConnection.alpnProtocol === 'h2')
withHttp2(tlsConnection);
else if (tlsConnection.alpnProtocol === 'http/1.1')
withHttp1(tlsConnection);
else
throw new Error('no valid protocol negotiated');
});
function withHttp1(socket) {
const req = https.request({
host: 'nghttp2.org',
path: '/httpbin/ip',
port: 443,
createConnection: () => socket
}, (res) => console.log(res.headers));
req.end();
}
function withHttp2(socket) {
const client = new Http2Session(constants.NGHTTP2_SESSION_CLIENT,
{},
socket);
const req = client.request({ ':path': '/httpbin/ip' });
req.on('response', (headers) => console.log(headers));
req.end();
} If you feel like it, you could probably extract parts of this to change |
@apapirovski @grantila Fwiw, I’ve opened #16267 and #16269 with the goal of making sure HTTP and HTTP/2 can use arbitrary duplex streams. For I wouldn’t consider these to be a final API for solving the particular problem you are bringing up, though. |
HTTP/2 is already set up to use arbitrary There are a couple of gotchas, however... notice that I said It is implemented that way for performance reasons and there would be no justification at all for changing that. That said, the Take a look here https://github.com/nodejs/node/blob/master/lib/internal/http2/core.js#L700 and here https://github.com/nodejs/node/blob/master/lib/internal/http2/core.js#L700 The logic within const { connectionListener } = require('http2').connectionListener;
const server = net.createServer((socket) => {
connectionListener(socket);
}); As long as Now, if you're talking about the client side, that works just about the same way. See the code here: https://github.com/nodejs/node/blob/master/lib/internal/http2/core.js#L2412-L2460 This is a small variation on the same theme with a few different requirements. Again, however, the |
looking... |
Not sure, but I think what this issue essentially boils down to is that |
Yes but it probably makes more sense for |
@apapirovski I’m not sure on this either, but I think what users would expect is that |
It's tricky. Personally, I would have a big concern around the user-friendliness of that because there are major differences. A user that enables something like Otherwise we might need to implement a whole other compatibility layer for the client side (like we did for server). Also, there probably aren't very many cases of servers that only support h2 (so someone using https will never — at least in the medium to long term — have their code stop working) whereas there are ones that only support h1. But that's just my current feelings on this... could be way off base. |
In theory, yes, and it's likely something we should work towards. We didn't do this yet for a few reasons: (a) we needed the core http/2 functionality done first, (b) we wanted to implement it in a way that intentionally did not touch the existing http/1 implementation and (c) there's a bit of work necessary to figure out how to enable this transparently. There are a couple of issues with making this all work transparently. The Agent model currently used by the http/1 clients (with or without the If someone wanted to spend some time coming up with a reasonable proposal, I certainly wouldn't be opposed to it tho. |
I would say no, not necessarily. Why not let other packages handle this (like the Can someone tell me what's wrong with my initial proposal? It separates connecting to a TLS server from the handling of HTTP/n protocols, and instead delegates this logic to the user instead. The whole issue with Disclaimer; a) The name |
Support generic `Duplex` streams through using `StreamWrap` on the server and client sides, and adding a `createConnection` method option similar to what the HTTP/1 API provides. Since HTTP2 is, as a protocol, independent of its underlying transport layer, Node.js should not enforce any restrictions on what streams its internals may use. Ref: nodejs#16256 PR-URL: nodejs#16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Support generic `Duplex` streams through more duck typing on the server and client sides. Since HTTP is, as a protocol, independent of its underlying transport layer, Node.js should not enforce any restrictions on what streams its HTTP parser may use. Ref: nodejs#16256 PR-URL: nodejs#16267 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Support generic `Duplex` streams through using `StreamWrap` on the server and client sides, and adding a `createConnection` method option similar to what the HTTP/1 API provides. Since HTTP2 is, as a protocol, independent of its underlying transport layer, Node.js should not enforce any restrictions on what streams its internals may use. Ref: #16256 PR-URL: #16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Support generic `Duplex` streams through more duck typing on the server and client sides. Since HTTP is, as a protocol, independent of its underlying transport layer, Node.js should not enforce any restrictions on what streams its HTTP parser may use. Ref: #16256 PR-URL: #16267 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Support generic `Duplex` streams through using `StreamWrap` on the server and client sides, and adding a `createConnection` method option similar to what the HTTP/1 API provides. Since HTTP2 is, as a protocol, independent of its underlying transport layer, Node.js should not enforce any restrictions on what streams its internals may use. Ref: #16256 PR-URL: #16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Support generic `Duplex` streams through using `StreamWrap` on the server and client sides, and adding a `createConnection` method option similar to what the HTTP/1 API provides. Since HTTP2 is, as a protocol, independent of its underlying transport layer, Node.js should not enforce any restrictions on what streams its internals may use. Ref: nodejs/node#16256 PR-URL: nodejs/node#16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Support generic `Duplex` streams through more duck typing on the server and client sides. Since HTTP is, as a protocol, independent of its underlying transport layer, Node.js should not enforce any restrictions on what streams its HTTP parser may use. Ref: nodejs/node#16256 PR-URL: nodejs/node#16267 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
This can now be accomplished like so: const https = require('https');
const http2 = require('http2');
const tls = require('tls');
const tlsConnection = tls.connect({
host: 'nghttp2.org',
port: 443,
ALPNProtocols: ['h2', 'http/1.1']
});
tlsConnection.once('secureConnect', () => {
if (tlsConnection.alpnProtocol === 'h2')
withHttp2(tlsConnection);
else if (tlsConnection.alpnProtocol === 'http/1.1')
withHttp1(tlsConnection);
else
throw new Error('no valid protocol negotiated');
});
function withHttp1(socket) {
const req = https.request({
host: 'nghttp2.org',
path: '/httpbin/ip',
port: 443,
createConnection: () => socket
}, (res) => console.log(res.headers));
req.end();
}
function withHttp2(socket) {
const client = http2.connect(
{ host: 'nghttp2.org', port: 443 },
{ createConnection: () => socket }
);
const req = client.request({ ':path': '/httpbin/ip' });
req.on('response', (headers) => console.log(headers));
req.end();
} That said, http2 still ideally needs an Agent implementation. |
I love the fast work you guys do, @apapirovski, @addaleax and @jasnell. Haven't yet looked into using this (btw, we'll see this in 8.9, or?) Just one thing; I don't think we need an Agent implementation for http2. You might want a "fake" agent-to-session wrapper to the compatibility layer, but those not using the compat should deal with sessions instead, right? After all, Agents are only there because http1 can't multiplex requests over the same socket. |
Support generic `Duplex` streams through using `StreamWrap` on the server and client sides, and adding a `createConnection` method option similar to what the HTTP/1 API provides. Since HTTP2 is, as a protocol, independent of its underlying transport layer, Node.js should not enforce any restrictions on what streams its internals may use. Ref: nodejs/node#16256 PR-URL: nodejs/node#16269 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Support generic `Duplex` streams through more duck typing on the server and client sides. Since HTTP is, as a protocol, independent of its underlying transport layer, Node.js should not enforce any restrictions on what streams its HTTP parser may use. Ref: nodejs/node#16256 PR-URL: nodejs/node#16267 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Going to close this issue as I believe this is handled. An agent/pool implementation should be coming by end of week (hopefully) |
Support generic `Duplex` streams through more duck typing on the server and client sides. Since HTTP is, as a protocol, independent of its underlying transport layer, Node.js should not enforce any restrictions on what streams its HTTP parser may use. Ref: #16256 PR-URL: #16267 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@jasnell Just curious, did the agent/pool implementation land? |
While building
fetch-h2
and when discussing withnode-fetch
, and while participating in #14671 I realize that we need this in Node.js; a protocol-agnosticconnect()
to a TLS-enabled web server and let the ALPN handshake allow for both"h2"
and"http/1.1"
. Without this, we won't be able to connect to an https-server without knowing in beforehand if that server wants to (or even can!) speak HTTP/1.1 or HTTP/2.I'm thinking of something like
tls.handoverConnect()
which doesn't take application-level (http) arguments (path, headers, etc), it just connects and returns (through a callback, likely) the protocol ("http/1.1" or "h2") and a corresponding session.A question is what the session data is. In case of HTTP/2, it should be an Http2Session, but for HTTP/1.1 it must be something where the user can perform the request, and it should probably be integrated with Agents. There's unfortunately an inheritance jungle in the design of these old functions, like
https.request()
where ip/tcp/application-level options are merged into one blob, but the "https/1.1 session" here (let's call itHttp1SessionProxy
) should be something that looks like thehttp
module, in how it has arequest()
function, with the tcp/ip-level options removed.Let's say this feature belongs in
tls
(which I think it does):In this example, perhaps an Agent could be passed in the
connectOpts
, so that if the session turns out to be HTTP/1.1, the connection will be added to this agent (and by default the global Agent is used, just like traditionalhttp{s}.request()
).Any thoughts on this? I guess it's a good time to look into this now, as the
http2
module is being tested in the wild, but before it's "stable".The text was updated successfully, but these errors were encountered: