-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
Initial support proposal for http2 #3390
Changes from 7 commits
58b88f1
43f7c01
f98568f
17efcbf
b9276c7
2e1e78b
878bf5f
e0552bd
d3620d2
e424883
be2aed1
2ac75f2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
try { | ||
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'); | ||
} | ||
} catch (_) { | ||
console.log('Http2 is not supported'); | ||
} |
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----- |
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----- |
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> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
console.log('Loaded'); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
console.log('Loaded'); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
console.log('Loaded'); |
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; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
/*! | ||
* 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)); | ||
var parse = require('parseurl'); | ||
/** | ||
* 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 :) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, 'path', function path() { | ||
var path; | ||
var headers = this.headers; | ||
if (headers === undefined) { | ||
return ''; | ||
} | ||
path = headers[http2Constants.HTTP2_HEADER_PATH]; | ||
this.parsedObject = this.parsedObject || {url: path}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The req.path should reflect any changes made to req.url over thr course of the request (for example middleware cut thr matched part). This is accidentally caching the first access value. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does not, if the url is set, it will rewrite the path, which in turn will rewrite the url on mock object and cause parse url to rerun. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are correct, I see the object alteration below, sorry! It would be awesome if it could just use the same cache that the routing used, but if this is the only way, then we'll just have to live with the perf hit req.path will get. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The only performance hit (to the best of my understanding is the creation of the new object - otherwise it uses the same caching mechanism and everything works exactly the same way, instead of operation on the req object though it operates on a new (only created once) object, and the cache is set on it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's not the case here. The router itself in Express is prepopulating the cache, but this req.path is using a separate cache, requiring a double parse. Since the movement through middleware will keep altering req.url, the internal prepopulation the router is doing for that is not getting picked up here. For example, the router will populate the cache and then in a middleware you call req.path it has to parse again instead of reusing the cache. Then the request moves on, the router alters req.url and saves the new parse in the cache. The call to req.path again requires yet another reparse since it uses a separate cache, etc. etc. |
||
this.parsedObject.url = path; | ||
return parse(this.parsedObject).pathname; | ||
}); | ||
|
||
/** | ||
* Module exports. | ||
* @public | ||
*/ | ||
|
||
module.exports = http2Req; |
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Awesome, will remove it then. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -31,9 +35,14 @@ exports.init = function(app){ | |
req.res = res; | ||
res.req = req; | ||
req.next = next; | ||
if (isHttp2Supported && req instanceof http2Request) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be checked by looking at There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure about this: There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this try wrapper. This is a http2 example, ideally the examples are how folks will write their code. Do you think everyone will be wrapping their servers in try blocks when they start using http2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't the tests running the examples? Maybe I was under a false impression here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nm, my bad, removing :)