-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
HTTP/2 createServer() options to pass custom Request and Response classes #15560
Conversation
doc/api/http2.md
Outdated
@@ -1474,6 +1474,12 @@ added: v8.4.0 | |||
used to determine the padding. See [Using options.selectPadding][]. | |||
* `settings` {[Settings Object][]} The initial settings to send to the | |||
remote peer upon connection. | |||
* `ServerRequest` {Http2ServerRequest} Specifies the ServerRequest class to |
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.
Nit: I would use http2.Http2ServerRequest
for consistency.
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.
@hekike sorry I wasn't clear. I didn't mean to change the option name but it's type from {Http2ServerRequest}
to {http2.Http2ServerRequest}
because this is what it is used in the rest of the file.
doc/api/http2.md
Outdated
* `ServerRequest` {Http2ServerRequest} Specifies the ServerRequest class to | ||
used. Useful for extending the original `Http2ServerRequest`. | ||
Defaults to: `Http2ServerRequest` | ||
* `ServerResponse` {Http2ServerResponse} Specifies the ServerRequest class to |
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.
Same here, http2.Http2ServerResponse
.
lib/internal/http2/core.js
Outdated
@@ -2337,7 +2339,10 @@ class Http2SecureServer extends TLSServer { | |||
super(options, connectionListener); | |||
this[kOptions] = options; | |||
this.timeout = kDefaultSocketTimeout; | |||
this.on('newListener', setupCompat); | |||
this.on('newListener', (ev) => | |||
setupCompat.call(this, this[kOptions].ServerRequest, |
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 it doesn't change anything but did you try using Function.prototype.bind()
? It may be a little faster and easier to read.
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.
It doesn't even need to be bound here. setupCompat
has access to this[kOptions]
.
Edit: In fact, to correct myself, onServerStream
has access to this[kOptions]
so all the .call
s can be eliminated.
benchmark/http2/inheritance.js
Outdated
// Original prototype | ||
server = http2.createServer((req, res) => { | ||
MyServerResponse.prototype.status.call(res, 200, 'text/plain'); | ||
res.end(`User Agent: ${MyServerRequest.prototype.getUserAgent.call(req)}` |
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.
Nit: can you please remove the line feed? The next line seems to fit here.
lib/internal/http2/core.js
Outdated
@@ -2360,7 +2365,10 @@ class Http2Server extends NETServer { | |||
super(connectionListener); | |||
this[kOptions] = initializeOptions(options); | |||
this.timeout = kDefaultSocketTimeout; | |||
this.on('newListener', setupCompat); | |||
this.on('newListener', (ev) => | |||
setupCompat.call(this, this[kOptions].ServerRequest, |
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.
Ditto.
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.
Also doesn't need bind
or call
.
lib/internal/http2/core.js
Outdated
this.on('stream', onServerStream); | ||
this.on('stream', (stream, headers, flags, rawHeaders) => { | ||
onServerStream.call(this, ServerRequest, ServerResponse, | ||
stream, headers, flags, rawHeaders); |
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.
Same as above, you don't need to pass through ServerRequest and ServerResponse. onServerStream
has access to this[kOptions]
(with small changes to move kOptions to util & import it both in core & compat)
benchmark/http2/inheritance.js
Outdated
// Original prototype | ||
server = http2.createServer((req, res) => { | ||
MyServerResponse.prototype.status.call(res, 200, 'text/plain'); | ||
res.end(`User Agent: ${MyServerRequest.prototype.getUserAgent.call(req)}` |
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 'original' is slower because you're having to access a prototype of an object from outside of it's scope and then you're using call
on it. This should just be:
this.writeHead(200, { 'Content-Type': contentType });
res.end(`User Agent: ${this.headers['user-agent'] || 'unknown'}`);
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.
To expand on this, I think you would actually not even want to run the original test & the prototype test against your modified version of the code. I think those should be tested against master while the PARAMS
test case should run against your build.
benchmark/http2/inheritance.js
Outdated
// Express style prototype extension for each request | ||
server = http2.createServer((req, res) => { | ||
Object.setPrototypeOf(req, MyServerRequest.prototype); | ||
Object.setPrototypeOf(res, MyServerResponse.prototype); |
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.
This closure is harder to optimize than the one for the PARAMS option because it has to access MyServerRequest & MyServerResponse. You would probably want to declare these request handlers outside of main
.
Edit: I guess that's not really possible though because of how the h2 benchmarks are created. That makes the comparison a bit tricky, I think.
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 tried to do something like Express https://github.com/expressjs/express/blob/a4bd4373b2c3b2521ee4c499cb8e90e98f78bfa5/lib/middleware/init.js#L35 to demonstrate the benefits of the new approach. I'm not sure we should merge the benchmark as well, it's more like a demonstration of this PR.
lib/internal/http2/core.js
Outdated
stream, headers, flags, rawHeaders); | ||
onServerStream.bind(this)(this[kOptions].Http2ServerRequest, | ||
this[kOptions].Http2ServerResponse, | ||
stream, headers, flags, rawHeaders); |
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.
You can just do this.on('stream', onServerStream.bind(this, this[kOptions].Http2ServerRequest, this[kOptions].Http2ServerResponse));
The rest will work as expected.
Definitely good with this approach in general but will do a review on Monday. |
@dougwilson is this something that you could make some use of in Express. Do you need it also for Express would be the main consumer of this feature, and if do not need it I'm 👎 as it increases the API surface area. |
@mcollina |
Maybe it would be more important to have this PR for http1 asap, and then bring http2 up to speed. If framework authors have a use for it, I'm 👍 . |
doc/api/http2.md
Outdated
Defaults to: `Http2ServerRequest` | ||
* `Http2ServerRequest` {Http2ServerResponse} Specifies the ServerRequest class to | ||
used. Useful for extending the original `Http2ServerResponse`. | ||
Defaults to: `Http2ServerResponse` |
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.
We would need to add the documentation of the constructor parameters. Those are not documented atm, but if we want to support this use case we should do it.
If this is going to be added to |
ping @Fishrock123 I saw you in expressjs/express#2812 and you also did https://github.com/pillarjs/extend-proto so figured you might have some thoughts. |
d4a9c75
to
be96525
Compare
@apapirovski thanks for checking it. I fixed your suggestions, here is the latest benchmark output after the changes: By requests:
X axis: total number of requests By streams:
X axis: concurrency level |
I would be +1 on doing this for http as well as we could use it for our internal http framework too |
Can we move the discussion on adding this for |
@mcollina good idea. The only reason why I started with |
@mcollina at least with Express current design, it would not be able to use this. The main things I see are
|
@hekike would this means that this functionality is needed only by Restify? I'm trying to understand who needs this functionality. |
@mcollina This is something that is generally useful for most frameworks since it doesn’t require directly modifying the prototype of the If this lands restify itself will be moving to using this approach, for both HTTP/1 and HTTP/2. This will also allow us to provide HTTP/2 support in restify quickly, as we have a PR lined up and ready to go that will use this approach. |
@yunong how do you deal with |
@apapirovski I'm not familiar with the HTTP/1 implementation in Node, but if I understand it correctly it's not a technical difficulty "just" we need to ship them together. Right? |
Well, there needs to be a solution for http1 but also there needs to be a solution for passing through http1 req & res prototypes through http2 to http1 when a user creates a server with node/lib/internal/http2/core.js Lines 2258 to 2269 in 4f4d55d
|
@hekike can you open up a PR for http? Then we will adjust this accordingly to support both http1 and http2. |
@mcollina sure, it will take a couple of days. |
doc/api/http2.md
Outdated
@@ -1714,6 +1714,14 @@ changes: | |||
* `Http1ServerResponse` {http.ServerResponse} Specifies the ServerResponse | |||
class to used for HTTP/1 fallback. Useful for extending the original | |||
`http.ServerResponse`. **Default:** `http.ServerResponse` | |||
* `Http2ServerRequest` {http2.Http2ServerRequest} Specifies the | |||
Http2ServerRequest class to used. |
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.
One final comment as I was going through this: to use.
or to be used.
Same below.
32ccd60
to
bdca0b7
Compare
@apapirovski thanks, fixed! |
Add optional Http2ServerRequest and Http2ServerResponse options to createServer and createSecureServer. Allows custom req & res classes that extend the default ones to be used without overriding the prototype. PR-URL: #15560 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Add optional Http2ServerRequest and Http2ServerResponse options to createServer and createSecureServer. Allows custom req & res classes that extend the default ones to be used without overriding the prototype. PR-URL: #15560 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Add optional Http2ServerRequest and Http2ServerResponse options to createServer and createSecureServer. Allows custom req & res classes that extend the default ones to be used without overriding the prototype. PR-URL: #15560 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Add optional Http2ServerRequest and Http2ServerResponse options to createServer and createSecureServer. Allows custom req & res classes that extend the default ones to be used without overriding the prototype. PR-URL: #15560 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Add optional Http2ServerRequest and Http2ServerResponse options to createServer and createSecureServer. Allows custom req & res classes that extend the default ones to be used without overriding the prototype. PR-URL: #15560 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Will need a backport if it should go back to v8.x (if you'd like to do a backport please change the label). |
Add optional Http2ServerRequest and Http2ServerResponse options to createServer and createSecureServer. Allows custom req & res classes that extend the default ones to be used without overriding the prototype. PR-URL: nodejs#15560 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Add optional Http2ServerRequest and Http2ServerResponse options to createServer and createSecureServer. Allows custom req & res classes that extend the default ones to be used without overriding the prototype. Backport-PR-URL: #20456 PR-URL: #15560 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Add optional Http2ServerRequest and Http2ServerResponse options to createServer and createSecureServer. Allows custom req & res classes that extend the default ones to be used without overriding the prototype. PR-URL: nodejs#15560 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Add optional Http2ServerRequest and Http2ServerResponse options to createServer and createSecureServer. Allows custom req & res classes that extend the default ones to be used without overriding the prototype. Backport-PR-URL: #20456 PR-URL: #15560 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Add optional Http2ServerRequest and Http2ServerResponse options to createServer and createSecureServer. Allows custom req & res classes that extend the default ones to be used without overriding the prototype. Backport-PR-URL: #20456 PR-URL: #15560 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Add optional Http2ServerRequest and Http2ServerResponse options to createServer and createSecureServer. Allows custom req & res classes that extend the default ones to be used without overriding the prototype. Backport-PR-URL: #20456 PR-URL: #15560 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Add optional Http2ServerRequest and Http2ServerResponse options to createServer and createSecureServer. Allows custom req & res classes that extend the default ones to be used without overriding the prototype. Backport-PR-URL: #20456 PR-URL: #15560 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Add optional Http2ServerRequest and Http2ServerResponse options to createServer and createSecureServer. Allows custom req & res classes that extend the default ones to be used without overriding the prototype. Backport-PR-URL: #20456 PR-URL: #15560 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
Motivation
Provide a simple and performant way to extend the original
Http2ServerRequest
andHttp2ServerResponse
classes in web frameworks likeexpress
orrestify
without messing with the original Response and Request handlers in Node core or overwriting the prototypes at every request.Current solutions for inheritance:
express
uses Object.setPrototypeOf to overwritereq
andres
prototypes at every requestrestify
extends the original Node Core IncomingMessage and ServerResponse ObjectsObject.setPrototypeOf:
How it works
Benchmark results
Continuously increasing concurrent and streams.
See new benchmark in this PR.
./node benchmark/scatter.js --runs 30 --set benchmarker=h2load benchmark/http2/inheritance > scatter.csv
Aggregated by requests:
cat scatter.csv | Rscript benchmark/scatter.R --xaxis requests --category version --plot scatter-plot.png --log
X axis: total number of requests
Y axis: rate of operations
Aggregated by streams:
cat scatter.csv | Rscript benchmark/scatter.R --xaxis streams --category version --plot scatter-plot.png --log
X axis: concurrency level
Y axis: rate of operations
From the result, aggregated by
requests
number you can see that this new extension type (passing custom classes as options) is significantly faster for higher concurrency and request number.Related PR