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.

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

Initial support proposal for http2 #3390

Closed
wants to merge 12 commits into from
Closed

Initial support proposal for http2 #3390

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
58b88f1
Initial support proposal for http2
PiniH Aug 9, 2017
43f7c01
Seperate to files and revert breaking api
phouri Sep 1, 2017
f98568f
Require parseurl and don't emit warning on setter
phouri Sep 1, 2017
17efcbf
Lint
phouri Sep 1, 2017
b9276c7
Http2 Example
phouri Sep 1, 2017
2e1e78b
Lint...
phouri Sep 1, 2017
878bf5f
Fix example
phouri Sep 3, 2017
e0552bd
Remove try/catch in example
phouri Sep 3, 2017
d3620d2
Remove defineGetter from utils
phouri Sep 3, 2017
e424883
Rename function name
phouri Sep 3, 2017
be2aed1
Remove overridding setter for statusMessage - will land in Node soon
phouri Sep 5, 2017
2ac75f2
Remove unneeded decorators after node js fixes
phouri Sep 12, 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
34 changes: 34 additions & 0 deletions examples/http2/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
var http2 = require('http2');
var fs = require('fs');
var path = require('path');
var express = require('../../');
var keys = path.join(__dirname, 'keys');
var app = express();
var style = fs.readFileSync(path.resolve(__dirname, './static/style.css'), 'utf8');
function pushStyle(res) {
res.createPushResponse({
':path': '/style.css',
':status': 200,
'content-type': 'text/css'
}, function(err, newResponse) {
newResponse.setHeader('content-type', 'text/css');
newResponse.end(style);
});
}

app.use(express.static('static', {setHeaders: function(res, file) {
if (file.indexOf('index.html') > -1) {
pushStyle(res);
}
}}));

var server = http2.createSecureServer({
key: fs.readFileSync(path.join(keys, 'test_key.pem')),
cert: fs.readFileSync(path.join(keys, 'test_cert.pem'))
}, app);

/* istanbul ignore next */
if (!module.parent) {
server.listen(3000);
console.log('Express started on port 3000');
}
16 changes: 16 additions & 0 deletions examples/http2/keys/test_cert.pem
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
-----BEGIN CERTIFICATE-----
MIICjzCCAfgCCQCduAjYszOZ3DANBgkqhkiG9w0BAQUFADCBizELMAkGA1UEBhMC
Q04xEzARBgNVBAgTCkd1YW5nIERvbmcxFDASBgNVBAcTC0d1YW5nIFpob3VlMQ4w
DAYDVQQKEwVUQkVEUDENMAsGA1UECxMEVEVTVDEQMA4GA1UEAxMHZmVuZ21rMjEg
MB4GCSqGSIb3DQEJARYRZmVuZ21rMkBnbWFpbC5jb20wHhcNMTIwOTIzMTQxMDI5
WhcNMTIxMDIzMTQxMDI5WjCBizELMAkGA1UEBhMCQ04xEzARBgNVBAgTCkd1YW5n
IERvbmcxFDASBgNVBAcTC0d1YW5nIFpob3VlMQ4wDAYDVQQKEwVUQkVEUDENMAsG
A1UECxMEVEVTVDEQMA4GA1UEAxMHZmVuZ21rMjEgMB4GCSqGSIb3DQEJARYRZmVu
Z21rMkBnbWFpbC5jb20wgZ8wDQYJKoZIhvcNAQEBBQADgY0AMIGJAoGBALOEtchk
KBK8WTqwXR2Aov2mc0+igyQTGbxBDSyyULHPiecMqOBHs5bV4DL1pc/01hLKIp4T
2j2KNTTmeivrtKd3wMQL7A+IgyqdmeqRi98pYUylFZrHxb9Kiwm7mpHanodmgnTT
zOluEpi/K9h9zM0DbIOynsOh9/w4E2Aq6JvrAgMBAAEwDQYJKoZIhvcNAQEFBQAD
gYEAnPd0JvCKQQBrm9jI6TkJKmfBa4NH0wUpMQv/bo2NWw1tA8fTQYb0S4aTep5Q
JdYctLQeE7abY1fpXFIwFY/FC0rE3alkEK+4PlCXvHGTYMiq90oH0JtlEqYTdTWJ
i99gtHarMEfzejyY3VDa2XFGmZrQCP6Co5NGDjAEr2A4ECg=
-----END CERTIFICATE-----
15 changes: 15 additions & 0 deletions examples/http2/keys/test_key.pem
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
-----BEGIN RSA PRIVATE KEY-----
MIICXQIBAAKBgQCzhLXIZCgSvFk6sF0dgKL9pnNPooMkExm8QQ0sslCxz4nnDKjg
R7OW1eAy9aXP9NYSyiKeE9o9ijU05nor67Snd8DEC+wPiIMqnZnqkYvfKWFMpRWa
x8W/SosJu5qR2p6HZoJ008zpbhKYvyvYfczNA2yDsp7Doff8OBNgKuib6wIDAQAB
AoGAAp2tdHUZLGS4PCWzxalJNr8FMSTiGlV464hbI8qZaG3oyYgisdn5oPoO4U85
ElW0BOQTKxCI/pqT+ehd4WP25u+RXBqOSfpIRQvY2RjXmeyrkDEZWATP/BUa/Oqa
0YitEsAXvt3pQli+LVc9GZSFZQECgwDVdAs4n7DdQlkLwIECQQDmFL9rIE/6wF3h
fJkvPFs67MJgMF/T4omLnv/FGSH7KBpjFHts9AbPIGjD1dadRpmHxk7ahbSTKMxu
uoZ1R1irAkEAx73MW4fJDQZDdJHwskYyGXuL99Fcr8xz6YZv75tm5O3eF2a/UvoO
UIgDGpTIWFrm+gli27p3J0rJhhOiI4JJwQJAYOjUR3bwuRlVcahdjTvK4WLf7Evz
0PdWH+z0pjwTyAn4M0tpQVb3lz57YiErqEsYV8v7Yqd2i5VfpjQCdlt6yQJBAIpm
7kph/SLEO0tzsGenEiHsJKFT9bhun8ape7h4YsSwOdrXPC0fzXlptVTe0S+/1Rpe
FJ0SSGv2e0snIYsfRUECQQCP8VOp3IIE8beytDoqn3QbWvobx94NVhHKUX5UB6C+
bhY0LpTTFb8VMfSkICZXhbpcKf5zIdRjOh0ZLDeZJl5v
-----END RSA PRIVATE KEY-----
16 changes: 16 additions & 0 deletions examples/http2/static/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<!DOCTYPE html>
<html>
<head>
<script src="s1.js" type="text/javascript"></script>
<script src="s2.js" type="text/javascript"></script>
<script src="s3.js" type="text/javascript"></script>
<link rel="stylesheet" href="/style.css">
</head>
<body>
<div>
Express with Http2 Example
Check out the network tab, notice how all the connections use the same connection ID
and the style gets via push (most of the time :) )
</div>
</body>
</html>
1 change: 1 addition & 0 deletions examples/http2/static/s1.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
console.log('Loaded');
1 change: 1 addition & 0 deletions examples/http2/static/s2.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
console.log('Loaded');
1 change: 1 addition & 0 deletions examples/http2/static/s3.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
console.log('Loaded');
8 changes: 8 additions & 0 deletions examples/http2/static/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
body {
margin: 0;
padding: 0;
height: 100vh;
display: flex;
justify-content: center;
align-items: center;
}
8 changes: 8 additions & 0 deletions lib/application.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ var debug = require('debug')('express:application');
var View = require('./view');
var http = require('http');
var compileETag = require('./utils').compileETag;
var isHttp2Suported = require('./utils').isHttp2Supported;
var compileQueryParser = require('./utils').compileQueryParser;
var compileTrust = require('./utils').compileTrust;
var deprecate = require('depd')('express');
Expand Down Expand Up @@ -99,6 +100,13 @@ app.defaultConfiguration = function defaultConfiguration() {
setPrototypeOf(this.response, parent.response)
setPrototypeOf(this.engines, parent.engines)
setPrototypeOf(this.settings, parent.settings)

//set prototype for http2 requests/response
if (isHttp2Suported) {
setPrototypeOf(this.http2Request, parent.http2Request)
setPrototypeOf(this.http2Response, parent.http2Response)
}

});

// setup locals
Expand Down
19 changes: 17 additions & 2 deletions lib/express.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ var Route = require('./router/route');
var Router = require('./router');
var req = require('./request');
var res = require('./response');

var isHttp2Supported = require('./utils').isHttp2Supported;
/**
* Expose `createApplication()`.
*/
Expand All @@ -34,7 +34,7 @@ exports = module.exports = createApplication;
*/

function createApplication() {
var app = function(req, res, next) {
var app = function (req, res, next) {
app.handle(req, res, next);
};

Expand All @@ -51,6 +51,21 @@ function createApplication() {
app: { configurable: true, enumerable: true, writable: true, value: app }
})

if (isHttp2Supported) {
var http2Req = require('./http2Request');
var http2Res = require('./http2Response');
app.http2Request = Object.create(http2Req, {
app: { configurable: true, enumerable: true, writable: true, value: app }
});

app.http2Response = Object.create(http2Res, {
app: { configurable: true, enumerable: true, writable: true, value: app }
});
}




app.init();
return app;
}
Expand Down
40 changes: 40 additions & 0 deletions lib/http2Request.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/*!
* express
* Copyright(c) 2009-2013 TJ Holowaychuk
* Copyright(c) 2013 Roman Shtylman
* Copyright(c) 2014-2015 Douglas Christopher Wilson
* MIT Licensed
*/

'use strict';

/**
* Module dependencies.
* @private
*/
var http2 = require('http2');
var http2Constants = http2.constants;
var decorator = require('./requestDecorator');
var defineGetter = require('./utils').defineGetter;
var http2Req = decorator(Object.create(http2.Http2ServerRequest.prototype));
/**
* Override default http2 path to return only pathname
* as it returns full url
* Gets the full URL from the headers
* Creates a mock object for parse to cache on and read from
* as url in http2 refers to this.path which will results in an error
Copy link

Choose a reason for hiding this comment

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

@jasnell this looks fine to me for compatibility, pinging you in to take a look :)

Copy link
Contributor

Choose a reason for hiding this comment

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

As noted below, I don't think this is something we want to do; one of the key aspects od Express (and this is so key that all official middleware are written this way) is that you can write anything using onlt the base type and you can plop that in Express and it'll work 100% the same. If you look like pretty much all our middleware, we don't even test them against Express because of this core philosophy.

*/
defineGetter(http2Req, 'url', function path() {
Copy link
Author

Choose a reason for hiding this comment

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

@dougwilson
I found a better solution, in retrospect my solution was bad :)

I changed it to basically work the same, except that I rewrite the getter for url to give the raw path (avoiding the endless loop on get path / url until it explodes).

Basically what happens in http2:
get path -> overridden by the decorator to return parse object pathname
set path -> sets the full path in the headers
get url -> brings the full path from headers (instead of this.path as in the unedited Http2ServerRequest)
set url -> sets the path

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks better, but i think the function should be named url instead of path. We also are not going to land sugar functions that monkey patch the underlying http module regardless. Express 3 and prior used to do that and it was a disaster in the long run, because it makes assumptions based on the inner implementation of Node.js core. The HTTP2 compat layer really makes it scary to even attempt to make the implementation assumptions this makes. The docs from Node.js:

This API targets only the public API of the HTTP/1, however many modules uses internal methods or state, and those are not supported as it is a completely different implementation.

Does Node.js think that replacing the setters is something that is sustainable? Can this be noted in the docs?

In fact https://nodejs.org/dist/latest-v8.x/docs/api/http2.html#http2_class_http2_http2serverrequest doesn't even document req.path. Why are we working around something undocumented in Node.js? Seems very dangerious.

Copy link
Author

Choose a reason for hiding this comment

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

Oh, right, forgot the function.

You are right - this does seem a bit dangerous, perhaps some opinion from Node.js team as to what we can do here would be best. Unfortunately without this change it just doesn't work.

@benjamingr @jasnell

Copy link

Choose a reason for hiding this comment

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

@nodejs/http2

var headers = this.headers;
if (headers === undefined) {
return '';
}
return headers[http2Constants.HTTP2_HEADER_PATH];
});

/**
* Module exports.
* @public
*/

module.exports = http2Req;
40 changes: 40 additions & 0 deletions lib/http2Response.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/*!
* express
* Copyright(c) 2009-2013 TJ Holowaychuk
* Copyright(c) 2014-2015 Douglas Christopher Wilson
* MIT Licensed
*/

'use strict';

/**
* Module dependencies.
* @private
*/
var Http2ServerResponse = require('http2').Http2ServerResponse;
var decorator = require('./responseDecorator');
/**
* Response prototype.
* @public
*/

var res = decorator(Object.create(Http2ServerResponse.prototype));

/**
* Add setter for statusMessage as it's deprecated, node doesn't set setter only getter
* so without setting this setter it fails
* Remove if node adds deprecation setter as well
*/
Object.defineProperty(res, 'statusMessage', {
set: function (x) {
Copy link

Choose a reason for hiding this comment

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

I had no idea you can define a setter separately from a getter, but apparently this works.

This can be a set() {} by the way (shorthand property)

Copy link

Choose a reason for hiding this comment

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

Nevermind, it can't, I forgot this needs to run on old Node since it's not versioned with core. Sorry for the misleading comment!

Copy link
Contributor

Choose a reason for hiding this comment

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

We should just wait for the setter to be in Node.js core, as the wrapper claims to be compatible for you only use the public API and setting the statusMessage is public API.

Copy link
Author

Choose a reason for hiding this comment

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

I will try to see what to do about the status message on the node part then.

However one thing I noticed is that express always tries to set
statusMessage which if node add setter will result in deprecation message
if you use express with http2 - any ideas how to avoid that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would say just change where it is being set to base that on the http version property of the request.

Copy link
Contributor

Choose a reason for hiding this comment

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

The change to Node.js core add a setter looks like it will land very soon (PR nodejs/node#15193) so we won't nee the custom setter any longer.

Copy link
Author

Choose a reason for hiding this comment

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

Awesome, will remove it then.

Copy link

Choose a reason for hiding this comment

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

Landed in Version 8.5.0

//Empty setter for statusMessage
}
});

/**
* Module exports.
* @public
*/

module.exports = res;

17 changes: 13 additions & 4 deletions lib/middleware/init.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,18 @@
*/

'use strict';

/**
* Module dependencies.
* @private
*/

var setPrototypeOf = require('setprototypeof')
var setPrototypeOf = require('setprototypeof');
var isHttp2Supported = require('../utils').isHttp2Supported;
var http2Request = null;

if (isHttp2Supported) {
http2Request = require('http2').Http2ServerRequest;
}
/**
* Initialization middleware, exposing the
* request and response to each other, as well
Expand All @@ -31,9 +35,14 @@ exports.init = function(app){
req.res = res;
res.req = req;
req.next = next;
if (isHttp2Supported && req instanceof http2Request) {
Copy link

@apapirovski apapirovski Sep 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This should be checked by looking at req.httpVersionMajor instead.

Copy link
Author

Choose a reason for hiding this comment

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

Not sure about this:
What if spdy-http2/node-spdy#316 is indeed fixed yet node-spdy still uses the http1 IncomingMessage?

Copy link

Choose a reason for hiding this comment

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

This actually seems fine now that I have fresh eyes. I don't know why I thought it was an issue anyway, I guess just stared at the code too long.

setPrototypeOf(req, app.http2Request)
setPrototypeOf(res, app.http2Response)
} else {
setPrototypeOf(req, app.request)
setPrototypeOf(res, app.response)
}

setPrototypeOf(req, app.request)
setPrototypeOf(res, app.response)

res.locals = res.locals || Object.create(null);

Expand Down
Loading