Improve HTTP2 documentation. #16366

jsha · 2017-10-21T19:09:27Z

Provide section headings for server-side and client side examples.
Add error handling and TLS to server-side example, following example of https.
Add error handling, TLS, more efficient Buffer usage, and header printing to
client example.

Fixes #16345 /cc @apapirovski

Provide section headings for server-side and client side examples. Add error handling and TLS to server-side example, following example of `https`. Add error handling, TLS, more efficient Buffer usage, and header printing to client example.

apapirovski

Just some nits. Thanks for taking this on! 👍

apapirovski · 2017-10-21T19:16:53Z

doc/api/http2.md

+req.on('data', (d) => data.push(d));
+req.on('end', () => {
+  console.log();
+  console.log(Buffer.concat(data).toString('utf8'));


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

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.

(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)

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…

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?

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.

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!

apapirovski · 2017-10-21T19:18:10Z

doc/api/http2.md

-
-const client = http2.connect('http://localhost:80');
+var http2 = require('http2');
+const client = http2.connect('https://localhost');


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.

Also connect to port 8443 in client, and add require('fs')

addaleax · 2017-10-22T11:47:56Z

doc/api/http2.md

+To generate the certificate and key for this example, run:
+
+```bash
+openssl req -newkey rsa:2048 -nodes -sha256 -subj '/CN=localhost' -x509 -keyout localhost-privkey.pem -out localhost-cert.pem


tiny style nit: Can you wrap at 80 characters? You can use backslashes to escape end-of-lines in bash, i.e.

fooprog arg1 arg2 arg3 \ arg4 arg5

is the same as fooprog arg1 arg2 arg3 arg4 arg5

addaleax · 2017-10-22T11:51:00Z

doc/api/http2.md

+req.on('data', (d) => data.push(d));
+req.on('end', () => {
+  console.log();
+  console.log(Buffer.concat(data).toString('utf8'));


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…

jasnell · 2017-10-22T16:22:31Z

doc/api/http2.md

-
-const client = http2.connect('http://localhost:80');
+var http2 = require('http2');
+var fs = require('fs');


nit: const http2 = require('http2'); and const fs = require('fs');

jasnell · 2017-10-22T16:22:49Z

doc/api/http2.md

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


nit: for (const name in headers) {

jasnell

LGTM with the requested nits addressed.

apapirovski · 2017-10-23T12:54:21Z

doc/api/http2.md

-req.on('data', (d) => data.push(d));
+req.setEncoding('utf8');
+let rawData = '';
+req.on('data', (chunk) => { rawData += chunk; });
 req.on('end', () => {
  console.log();


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.)

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" + ...)?

apapirovski · 2017-10-25T17:23:03Z

Landed in 3621889 (with fixes for the linter)

Thanks @jsha!

Provide section headings for server-side and client side examples. Add error handling and TLS to server-side example, following example of `https`. Add error handling, TLS, more efficient Buffer usage, and header printing to client example. PR-URL: #16366 Fixes: #16345 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Provide section headings for server-side and client side examples. Add error handling and TLS to server-side example, following example of `https`. Add error handling, TLS, more efficient Buffer usage, and header printing to client example. PR-URL: nodejs/node#16366 Fixes: nodejs/node#16345 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Provide section headings for server-side and client side examples. Add error handling and TLS to server-side example, following example of `https`. Add error handling, TLS, more efficient Buffer usage, and header printing to client example. PR-URL: #16366 Fixes: #16345 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Provide section headings for server-side and client side examples. Add error handling and TLS to server-side example, following example of `https`. Add error handling, TLS, more efficient Buffer usage, and header printing to client example. PR-URL: nodejs/node#16366 Fixes: nodejs/node#16345 Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de>

Improve HTTP2 documentation.

564fa85

Provide section headings for server-side and client side examples. Add error handling and TLS to server-side example, following example of `https`. Add error handling, TLS, more efficient Buffer usage, and header printing to client example.

nodejs-github-bot added doc Issues and PRs related to the documentations. http2 Issues or PRs related to the http2 subsystem. labels Oct 21, 2017

jsha mentioned this pull request Oct 21, 2017

Document error handling in http2 client connect example #16345

Closed

apapirovski approved these changes Oct 21, 2017

View reviewed changes

Add localhost cert instructions.

e0e5aea

Also connect to port 8443 in client, and add require('fs')

addaleax approved these changes Oct 22, 2017

View reviewed changes

jasnell reviewed Oct 22, 2017

View reviewed changes

jasnell approved these changes Oct 22, 2017

View reviewed changes

jsha added 2 commits October 22, 2017 15:09

Consts and line wrap.

cb0acb6

Go back to string concatenation.

7a44c9d

trivikr approved these changes Oct 23, 2017

View reviewed changes

apapirovski reviewed Oct 23, 2017

View reviewed changes

tniessen approved these changes Oct 24, 2017

View reviewed changes

apapirovski closed this Oct 25, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

gibfahn mentioned this pull request Oct 31, 2017

v8.9.0 proposal #16630

Merged

MylesBorins added the dont-land-on-v6.x label Nov 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve HTTP2 documentation. #16366

Improve HTTP2 documentation. #16366

jsha commented Oct 21, 2017 •

edited

Loading

apapirovski left a comment

apapirovski Oct 21, 2017

jsha Oct 21, 2017

jsha Oct 21, 2017

addaleax Oct 22, 2017

jsha Oct 22, 2017

apapirovski Oct 22, 2017

jsha Oct 22, 2017

apapirovski Oct 21, 2017

addaleax Oct 22, 2017

addaleax Oct 22, 2017

jasnell Oct 22, 2017

jasnell Oct 22, 2017

jasnell left a comment

apapirovski Oct 23, 2017 •

edited

Loading

jsha Oct 23, 2017

apapirovski commented Oct 25, 2017 •

edited

Loading

Improve HTTP2 documentation. #16366

Improve HTTP2 documentation. #16366

Conversation

jsha commented Oct 21, 2017 • edited Loading

apapirovski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

apapirovski Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apapirovski commented Oct 25, 2017 • edited Loading

jsha commented Oct 21, 2017 •

edited

Loading

apapirovski Oct 23, 2017 •

edited

Loading

apapirovski commented Oct 25, 2017 •

edited

Loading