-
-
Notifications
You must be signed in to change notification settings - Fork 16.3k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Initial support proposal for http2 #3390
Changes from 6 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,40 @@ | ||
var isHttp2Supported = require('../../lib/utils').isHttp2Supported; | ||
if (isHttp2Supported) { | ||
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. ...and sincd this is a http2 example, just remove this wrapper. |
||
var fs = require('fs'); | ||
var path = require('path'); | ||
var express = require('../../'); | ||
var http2 = require('http2'); | ||
var keys = path.join(__dirname, 'keys'); | ||
var app = express(); | ||
var style = fs.readFileSync('./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.get('/', function(req, res, next){ | ||
pushStyle(res); | ||
next(); | ||
}); | ||
app.get('/index.html', function(req, res, next) { | ||
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 see you are trying to show an example of pushing thr dtyle sheet for html. You missed /index works as well. Thr static middleware has a callback you can use to see the file it is about to send, which would be a better example instead of trying to enumerate them as routes. 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 didn't know about the static callback :) 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. Sorry, on holiday and wasn't easy to lookup :) so the setHeader could be used, but seems to really be an abuse of it. Looking into it, what I was thinking was the "file" event that is in send. So I wonder if as part of getting serve-static/send/deps working with http2, another callback should be added in order to make pushing file assets very straight forward. 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. Adding support for the new http2 push is something I was wondering how to do, because I think the best way express could solve it is to make it go through another route when you hit one route you know would require the other. i.e. when requiring index.html send it also to style.css (assuming no static is used), but I have no clue where to begin ;) How do I use that file event from send? I thought it was pretty deep inside. |
||
pushStyle(res); | ||
next(); | ||
}) | ||
app.use(express.static('static')); | ||
|
||
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'); | ||
} | ||
} |
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.
The examples should not be reaching into Express internals.
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.
I think this can (and should) be exposed as a public API (isHttp2Supported)