Skip to content

Improve HTTP2 documentation. #16366

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

Improve HTTP2 documentation. #16366

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
564fa85
Improve HTTP2 documentation.
jsha Oct 21, 2017
e0e5aea
Add localhost cert instructions.
jsha Oct 21, 2017
cb0acb6
Consts and line wrap.
jsha Oct 22, 2017
7a44c9d
Go back to string concatenation.
jsha Oct 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 30 additions & 19 deletions doc/api/http2.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,25 @@ support for HTTP/2 protocol features. It is specifically *not* designed for
compatibility with the existing [HTTP/1][] module API. However,
the [Compatibility API][] is.

The `http2` Core API is much more symmetric between client and server than the
`http` API. For instance, most events, like `error` and `socketError`, can be
emitted either by client-side code or server-side code.

### Server-side example

The following illustrates a simple, plain-text HTTP/2 server using the
Core API:

```js
const http2 = require('http2');
const fs = require('fs');

// Create a plain-text HTTP/2 server
const server = http2.createServer();
const server = http2.createSecureServer({
key: fs.readFileSync("test/fixtures/keys/agent2-key.pem"),
cert: fs.readFileSync("test/fixtures/keys/agent2-cert.pem"),
});
server.on('error', (err) => console.error(err));
server.on('socketError', (err) => console.error(err));

server.on('stream', (stream, headers) => {
// stream is a Duplex
Expand All @@ -34,34 +45,34 @@ server.on('stream', (stream, headers) => {
stream.end('<h1>Hello World</h1>');
});

server.listen(80);
server.listen(8443);
```

Note that the above example is an HTTP/2 server that does not support SSL.
This is significant as most browsers support HTTP/2 only with SSL.
To make the above server be able to serve content to browsers,
replace `http2.createServer()` with
`http2.createSecureServer({key: /* your SSL key */, cert: /* your SSL cert */})`.
### Client-side example

The following illustrates an HTTP/2 client:

```js
const http2 = require('http2');

const client = http2.connect('http://localhost:80');
var http2 = require('http2');
const client = http2.connect('https://localhost');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the default example is using a createSecureServer, maybe this could mention that one might need either rejectUnauthorized: false or to pass the ca in order for self-signed certs to work.

Also, it should probably connect on port 8443 to match the example above.

client.on('socketError', (err) => console.error(err))
client.on('error', (err) => console.error(err))

// req is a Duplex
const req = client.request({ ':path': '/' });

req.on('response', (headers) => {
console.log(headers[':status']);
console.log(headers['date']);
req.on('response', (headers, flags) => {
for (var name in headers) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: for (const name in headers) {

console.log(name + ": " + headers[name]);
}
});

let data = '';
req.setEncoding('utf8');
req.on('data', (d) => data += d);
req.on('end', () => client.destroy());
let data = []
req.on('data', (d) => data.push(d));
req.on('end', () => {
console.log();
Copy link
Contributor

@apapirovski apapirovski Oct 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we remove this extra console.log — I think it might confuse people as to why it's there.

(The rest is good to go, I think.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to have a newline between the headers and the body, since that will make it clearer which is which. Would it be better to put the newline as part of the next line, e.g. console.log("\n" + ...)?

console.log(Buffer.concat(data).toString('utf8'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like the example with setEncoding might've been a bit clearer but I'm ok with either.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with setEncoding is that it turns each chunk into a string, meaning Buffer.concat won't work. We could join strings with Array.prototype.join, but to me, accumulating chunks in buffers is more natural. It's also more compatible with performing other transforms, like inflate or gunzip, on the resulting data.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Making it more concrete: I actually spent a while fighting a subtle bug with this, trying to inflate data that had been decoded as if it were utf-8)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of agree with @apapirovski here – you’re right, if you don’t want to receive text data, you shouldn’t use setEncoding() at all, but Buffer.concat() has the downside of having double the memory requirement, because the concatenated buffer needs to be a fresh allocation too…

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding was that doing += on a string in a for loop has bad performance: it potentially causes an allocation each time plus O(n) work to copy into the newly allocated space. Does Node specially detect and optimize for this case, e.g. by predicting the final length of the string and preallocating for it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Been a while since I read about it but pretty sure V8 has a special way of dealing with string concatenation. I believe it only gets truly concatenated when it actually gets used so if you're not also reading (or otherwise using) the string within your for loop then it'll always outperform arrays.

But anyway, it feels like this discussion is heading in the wrong direction. The main concern here should be the ease of understanding for somebody new to Node. To me, the setEncoding example is the only one that doesn't rely on understanding yet another part of Node and requires less lines to get the same point across.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it only gets truly concatenated when it actually gets used so if you're not also reading (or otherwise using) the string within your for loop then it'll always outperform arrays.

Aha, I did not know that! Thanks for updating my knowledge. I also checked the http docs, and they use the string concatenation style, so I've reverted to that.

The main concern here should be the ease of understanding for somebody new to Node.

True, although since example code so often forms the kernel of a new program, it's important not to recommend anti-patterns that a newbie might not know enough to see. Of course, in this case it turned out to not be an anti-pattern in modern environments, which is great!

client.destroy()
});
req.end();
```

Expand Down