Add support for http2 #98

tony19 · 2016-06-17T13:07:37Z

Inspired by Surma's http2 go server, I thought it would be cool to test Polymer apps with http2 (although this PR uses a Node solution). I'm hoping to get this PR in so that it could be rolled into polymer-cli.

Note that h2 requires ALPN, which is only supported in Node 5 or newer. The server will not start on an older Node version.

This PR adds a few CLI flags for h2. If the TLS key and certificate are unspecified when using https/h2, they're automatically generated upon starting the server.

$ polyserve --help

  A development server for Polymer projects

polyserve

Options

...
  -P, --protocol string        The server protocol to use {h2, https/1.1,
                               http/1.1}. Defaults to "http/1.1".
  --key string                 Path to TLS certificate private key file for
                               https. Defaults to "key.pem".
  --cert string                Path to TLS certificate file for https. Defaults
                               to "cert.pem"
  --manifest string            Path to h2-push manifest
  --help                       Shows this help message

Example usage

Start server with http (1.1) (default):

$ polyserve -c .

Start server with https (1.1):

$ polyserve -c . -P https/1.1

Start server with http2:

$ polyserve -c . -P h2

Start http2 server with push manifest:

$ polyserve -c . -P h2 --manifest push_manifest.json

Start https server with specific TLS certificate and key:

$ polyserve -c . -P https/1.1 --key /path/to/key.pem --cert /path/to/cert.pem

Verifying HTTP2

Open Chrome's DevTools > Network tab
Right-click a column header, and enable the Protocol column.
CD into root directory of Polymer app source
Run: polyserve -c . -P h2 -o
Ignore Chrome's warning ("Your connection is not private"), caused by the self-signed certificate, and proceed to localhost.
In DevTools > Network tab, observe the Protocol column shows "h2".

Verifying HTTP2 push

Create a push-manifest (multi-file format). For example, use http2-push-manifest to generate a manifest for Polymer CLI's shop-app template:
```
 $ cd /path/to/shop-app-template/
 $ http2-push-manifest -f index.html -f index.html
```
Specifying -f index.html twice "tricks" it into using the multi-file format for that same file. The end result will list many files, including Bower dependencies. In my localhost tests, I noticed pushing many files actually slows down page-load (needs more performance testing).

Edit push_manifest.json so that only a few files are pushed. Also replace "index.html" with "/src/shop-app.html".

For example:

{
"/src/shop-app.html": {
  "/src/shop-image.html": {
    "weight": 1,
    "type": "document"
  },
  "/src/shop-button.html": {
    "weight": 1,
    "type": "document"
  },
  "/src/shop-home.html": {
    "weight": 1,
    "type": "document"
  }
}

Run: polyserve -c . -P h2 --manifest push_manifest.json -o
Open Chrome's DevTools > Network tab, and refresh the Shop App's home page.
In DevTools > Network tab, filter for shop- files, and verify the Initiator column indicates Push/Other for the files specified in the push-manifest.

TimvdLippe · 2016-08-30T12:10:55Z

This pullrequest has been opened quite a while ago, but I really would like this feature integrated in the Polymer CLI. Currently we are shipping for HTTP2 and have to do our performance testing on our live server (with prefix, to not test on production 😉 ). Would be a lot better to have it integrated directly :)

justinfagnani · 2016-08-30T17:26:09Z

src/start_server.ts

+ * @returns {Promise<T>}
+ */
+function writeFile(filename: string, contents: any) {
+  let p = new Promise((resolve, reject) => {


just directly return this Promise

justinfagnani · 2016-08-30T17:38:41Z

Hey @tony19 just started to take a look at this. Sorry it's been so long.

I just realized that since we're compiling to ES6 we can use async functions. They'll make the promise using code much simpler.

Also, I don't think we can use destructuring in node 4. The tests pass because those code paths aren't hit yet.

justinfagnani · 2016-08-30T17:39:52Z

src/start_server.ts

@@ -71,7 +76,8 @@ export function startServer(options: ServerOptions): Promise<http.Server> {
        resolve(options);
      });
    }
-  }).then<http.Server>((opts) => startWithPort(opts));
+  }).then<http.Server>((opts) => startWithPort(opts))
+    .catch((e) => console.error('ERROR: Server failed to start:', e));


You need to propagate the error, so throw it after the console.error

tony19 · 2016-08-31T05:34:11Z

@justinfagnani Thanks for the thorough review. I actually forgot about this PR. I'll update it to address your comments soon.

tony19 · 2016-09-29T11:23:30Z

@justinfagnani I've addressed all comments and added h2 push support. Note that h2 requires ALPN, which is only supported in Node5+.

justinfagnani · 2016-09-30T18:20:47Z

@tony19 I just made some changes that require a rebase. Sorry! (if you can give me permissions on the branch, I'll update).

Looking at the new changes today :)

justinfagnani

Looking good with a few changes and a rebase. And I want to try it out :) Thanks for the work!

justinfagnani · 2016-10-02T04:24:15Z

custom_typings/spdy.d.ts

@@ -0,0 +1,148 @@
+// copied from node's "http2" module, which should have same API as spdy


Any chance of trying to upstream this into DefinitelyTyped?

Will do. I'll note the PR here when ready.

typings for pem: DefinitelyTyped/DefinitelyTyped#11666
typings for spdy: DefinitelyTyped/DefinitelyTyped#11672

justinfagnani · 2016-10-02T04:28:47Z

src/args.ts

@@ -69,4 +69,34 @@ export let args : ArgDescriptor[] = [
      ' Defaults to "index.html".',
    type: String,
  },
+  {
+    name: 'protocol',
+    alias: 'h',


We're using h as an alias for help in the CLI. Things are easier if there aren't any clashes. How about P?

justinfagnani · 2016-10-02T04:31:13Z

src/args.ts

+  },
+  {
+    name: 'cert',
+    alias: 't',


given we share aliases with the CLI by default, how about leaving off alias for options with decent defaults. I'd leave off all but protocol.

justinfagnani · 2016-10-02T04:52:44Z

src/start_server.ts

+ * @param {ServerOptions} options
+ * @returns {Promise<http.Server>} Promise of server
+ */
+function createServer(app: any, options: ServerOptions): Promise<http.Server> {


this function would look a bit nicer with async/await

justinfagnani · 2016-10-02T04:53:08Z

src/start_server.ts

+  if (isHttps(options.protocol)) {
+    p = getTLSCertificate(options.keyPath, options.certPath)
+        .then((keys) => {
+          let opt = {


const for opt and server

justinfagnani · 2016-10-02T04:53:15Z

src/start_server.ts

+          return Promise.resolve(server);
+        });
+  } else {
+    let spdyOptions = {protocols: [options.protocol], plain: true, ssl: false};


justinfagnani · 2016-10-02T04:53:28Z

src/start_server.ts

+ * @returns {Promise<http.Server>} Promise of server
+ */
+function startWithPort(userOptions: ServerOptions): Promise<http.Server> {
+  let options = applyDefaultOptions(userOptions);


justinfagnani · 2016-10-02T05:00:33Z

test/start_server_test.js

+    suite('node5+', () => {
+      // test() requires old-style function to use `this.skip()`,
+      // where `this` is the current test instance, so don't use
+      // arrow-function for the test callback.


you could just change the suite definition function depending on the node version:

const suiteDef = _nodeVersion < 5 ? suite.skip : suite; suiteDef('node5+', () => { // ... });

Added support for serving files over http2, including new CLI flags: -h, --protocol string The server protocol to use {h2, https/1.1, http/1.1}. Defaults to "http/1.1". -k, --key string Path to TLS certificate private key file for https. Defaults to "key.pem". -t, --cert string Path to TLS certificate file for https. Defaults to "cert.pem". Note h2 requires ALPN, which is only supported in Node 5 or newer.

rictic · 2016-10-02T17:43:40Z

src/start_server.ts

+  if (!port || port < 0) {
+    port = await new Promise<number>(resolve => {
+      findPort(8080, 8180, (ports: number[]) => {
+        resolve(ports[0]);


It'd be nice if this function threw if no ports were unavailable.

Something like:

let ports = [8080, 8180]; if (!port || port < 0) { ports = [port]; } return new Promise<number>((resolve, reject) => { findPort(ports, (availablePorts: number[]) => { if (availablePorts.length > 0) { resolve(ports[0]); return; } reject(new Error(`No available ports for http server to bind to. Tried: ${ports.join(', ')}`)); }); });

Ah, wait, you handle this below when creating the server which is the better time to handle this error. On the other hand, it looks like this returns undefined if no port option is given and neither 8080 or 8180 is available. Not sure what the http server library would do with an undefined port.

Interestingly, I think find-port has a bug because it still resolves with a port number if none are available. The redundant error checking in startWithPort() catches the issue. The getport module (which has the same API as find-port) correctly returns an error in a callback. We could switch if you prefer.

I wouldn't mind switching, but let's just add a TODO for now and get this PR in :)

it looks like this returns undefined if no port option is given and neither 8080 or 8180 is available.

@rictic When port is undefined, !port === true, so the body of the if-statement would run to find the next available port.

Not sure what the http server library would do with an undefined port.

@rictic It would throw an error: RangeError: "port" argument must be >= 0 and < 65536

rictic · 2016-10-02T17:47:25Z

src/start_server.ts

+      console.error(portInUseMessage(options.port));
+    }
+    console.warn('rejecting with err', err);
+    throw new Error(err);


This won't reject the startWithPort returned promise, but it would be nice if it did.

Yeah, good catch.

I assumed throwing an error anywhere in async/await would reject, but it seems that doesn't apply inside an event handler. Now, I'm not sure how to use async/await with createServer. I'll probably have to thenify this section of code (revert to previous promise code). Any suggestions?

Yep, good catch!

async/await still helps a little here, in that you can await a Promise created inline and rejections will be wired up correctly. Something like:

await new Promise((resolve, reject) => { server.listen(options.port, options.hostname, () => { handleServerReady(options); resolve(); }); server.on('error', reject); }); return server;

Cool, that works. Done

Added support for pushing files with h2 requests, including new CLI flag: --manifest string Path to h2-push manifest Usage: 1. Generate push manifest with http2-push-manifest node module (use multi-file format) 2. Run: polyserve --manifest push_manifest.json -P h2

justinfagnani · 2016-10-03T16:30:29Z

@tony19 I just tried to run this branch and I'm getting:

ERROR: Server failed to start: { Error: ENOENT: no such file or directory, open 'cert.pem'
    at Error (native) errno: -2, code: 'ENOENT', syscall: 'open', path: 'cert.pem' }

tony19 · 2016-10-03T17:52:02Z

@justinfagnani Fixed

justinfagnani · 2016-10-05T04:23:26Z

Awesome, thanks!

justinfagnani reviewed Aug 30, 2016
View reviewed changes

tony19 force-pushed the http2-draft branch 3 times, most recently from 93f22cd to ca7127d Compare September 29, 2016 10:37

justinfagnani suggested changes Oct 2, 2016

View reviewed changes

tony19 force-pushed the http2-draft branch from ca7127d to bdebb16 Compare October 2, 2016 13:46

rictic reviewed Oct 2, 2016

View reviewed changes

tony19 added 2 commits October 2, 2016 17:30

Add custom typings for pem and spdy

7ce32bf

tony19 force-pushed the http2-draft branch from bdebb16 to 4d31fda Compare October 2, 2016 22:47

tony19 added 2 commits October 3, 2016 12:47

Add http2 tests

74f555c

Update README for h2 flags

304db74

Refactor to use async/await

169a4a6

tony19 force-pushed the http2-draft branch from 4d31fda to 169a4a6 Compare October 3, 2016 17:49

justinfagnani merged commit aa54202 into Polymer:master Oct 5, 2016

		@@ -0,0 +1,148 @@
		// copied from node's "http2" module, which should have same API as spdy

Add support for http2 #98

Add support for http2 #98

Conversation

tony19 commented Jun 17, 2016 • edited Loading

Example usage

Verifying HTTP2

Verifying HTTP2 push

TimvdLippe commented Aug 30, 2016

Choose a reason for hiding this comment

justinfagnani commented Aug 30, 2016

Choose a reason for hiding this comment

tony19 commented Aug 31, 2016

tony19 commented Sep 29, 2016

justinfagnani commented Sep 30, 2016

justinfagnani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tony19 Oct 2, 2016 • edited Loading

Choose a reason for hiding this comment

tony19 Oct 2, 2016 • edited Loading

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

Choose a reason for hiding this comment

justinfagnani Oct 2, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rictic Oct 2, 2016 • edited Loading

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

justinfagnani commented Oct 3, 2016

tony19 commented Oct 3, 2016

justinfagnani commented Oct 5, 2016

tony19 commented Jun 17, 2016 •

edited

Loading

tony19 Oct 2, 2016 •

edited

Loading

tony19 Oct 2, 2016 •

edited

Loading

justinfagnani Oct 2, 2016 •

edited

Loading

rictic Oct 2, 2016 •

edited

Loading