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

add typescript and http/2 and server push support #1

Closed
wants to merge 125 commits into from
Closed

add typescript and http/2 and server push support #1

wants to merge 125 commits into from

Conversation

masx200
Copy link

@masx200 masx200 commented Apr 20, 2020

@masx200 masx200 changed the title add typescript and http/2 support add typescript and http/2 and server push support Apr 20, 2020
@pimterry
Copy link
Member

This is awesome! It's also quite a big change though. I'm probably not going to merge this directly in its full form, but I would like to merge some parts from it into the current project structure.

I have a couple of questions though:

  • You've changed your fork to GPL. I'm not sure that's totally valid (I think you need to at least keep the MIT license in addition to the GPL one), and I don't really want my version of this lib to be GPL (I keep most of my tiny libs like this under more relaxed licenses). Are you happy for me to merge some of these changes as MIT instead?

  • You're supporting SPDY and using the npm spdy library instead of using the new built-in http2. Is there a specific reason for that? Just wondering if there's an important feature that http2 is missing, or if you've seen lots of clients that support SPDY but don't support http2 somewhere.

  • You're missing the __httpPeekedData workaround and the allowHalfOpen TLS fix. Is that because they're solved a different way in this implementation, or just because you haven't merged them into your changes?

Either way, thanks for sending this my way, looks really interesting, nice work!

@masx200 masx200 closed this Apr 21, 2020
@masx200
Copy link
Author

masx200 commented Apr 21, 2020

I have changed the license to MIT.

@masx200
Copy link
Author

masx200 commented Apr 21, 2020

About allowHalfOpenTLS,
I am using net.server.
I found that the default is socket.allowHalfOpen === false.

@masx200
Copy link
Author

masx200 commented Apr 21, 2020

With spdy module you can create HTTP2 / SPDY servers in node.js with natural http module interface and fallback to regular https (for browsers that don't support neither HTTP2, nor SPDY yet).

The spdy module can implement all functions of the http 2 protocol and provide APIs similar to the HTTPS module.

@masx200
Copy link
Author

masx200 commented Apr 21, 2020

Is there a reproducible code base for __httpPeekedData? I used nodejs v13 and did not encounter this problem

@masx200
Copy link
Author

masx200 commented Apr 21, 2020

mscdex/httpolyglot#13

What is the reason behind this problem?

@masx200
Copy link
Author

masx200 commented Apr 21, 2020

If there is a problem with socket.unshift (data) ;, can stream.transform be used to solve this problem?

@pimterry
Copy link
Member

I have changed the license to MIT.

Thanks!

About allowHalfOpenTLS,
I am using net.server.
I found that the default is socket.allowHalfOpen === false.

Yes, but http.Server overrides that and sets allowHalfOpen to true: https://github.com/nodejs/node/blob/b149eefa82327af9835d3c68b3fd03e0e4a25413/lib/_http_server.js#L346.

I'm not actually sure if that will cause you problems, but it will cause slightly different behaviour. That's why the original httpolyglot set it to true (which also isn't correct - it should actually be true for HTTP, false for HTTPS: mscdex/httpolyglot#11). No idea how HTTP/2 handles that!

Is there a reproducible code base for __httpPeekedData?

Here's a quick demo:

const httpolyglot = require('httpolyglot');
const fs = require('fs');

const server = httpolyglot.createServer({
  key: fs.readFileSync('./ca.key'),
  cert: fs.readFileSync('./ca.pem')
}, function(req, res) {
  res.writeHead(200, { 'Content-Type': 'text/plain' });
  res.end((req.socket.encrypted ? 'HTTPS' : 'HTTP') + ' Connection!');
}).listen(9000, 'localhost', function() {
  console.log('httpolyglot server listening on port 9000');
});

server.on('clientError', (e) => {
    console.log('Error', e.code, e.rawPacket.toString());
});

Now connect to the server and issue an invalid HTTP request, e.g.:

curl -X ABC http://localhost:9000 

The server will print:

Error HPE_INVALID_METHOD BC / HTTP/1.1

The raw packet here shows only BC as the method, not ABC.

HTTPS requests to httpolyglot correctly print ABC / HTTP/1.1. So do HTTP requests to a standard http.Server.

I haven't tested that many node versions, but this happens reliably in both Node 10.15.3 & Node 14.0.0. I think it's caused because the socket & HTTP parsers are doing various things with native buffers, and pretending that they're just normal JS streams. unshift works fine to repopulate the data and make the request work, but it doesn't repopulate it perfectly everywhere.

For my case (an HTTP debugging tool, where I report these client errors to the end users), I really need this data to be correct. Even for everybody else, having correct data here is nice for logging (and there might be other cases that use this same raw packet data, I don't know).

can stream.transform be used to solve this problem?

It might be fixable by wrapping the incoming socket with a separate stream - I haven't tested that, but similar discussions elsewhere suggested that there's a performance penalty to that option. For now, the __httpPeekedData workaround solved the problem for me 😃.

@masx200
Copy link
Author

masx200 commented Apr 27, 2020

well done

@masx200
Copy link
Author

masx200 commented Apr 28, 2020

If you want to upgrade "https" server to "http / 2" server, just replace "require ('https')" with "require ('spdy')".

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

Successfully merging this pull request may close these issues.

2 participants