GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — HTTP/2 createServer() options to pass custom Request and Response classes by hekike · Pull Request #15560 · nodejs/node

Conversation

@hekike

@hekike hekike commented Sep 22, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Checklist

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Affected core subsystem(s)

  • http2

Motivation

Provide a simple and performant way to extend the original Http2ServerRequest and Http2ServerResponse classes in web frameworks like express or restify 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 overwrite req and res prototypes at every request

  • restify extends the original Node Core IncomingMessage and ServerResponse Objects

Object.setPrototypeOf:

Warning: Changing the [[Prototype]] of an object is, by the nature of how modern JavaScript engines optimize property accesses, a very slow operation, in every browser and JavaScript engine. The effects on performance of altering inheritance are subtle and far-flung, and are not limited to simply the time spent in obj.proto = ... statement, but may extend to any code that has access to any object whose [[Prototype]] has been altered. If you care about performance you should avoid setting the [[Prototype]] of an object. Instead, create a new object with the desired [[Prototype]] using Object.create(). - MDN

How it works

class MyServerRequest extends http2.Http2ServerRequest {
  getUserAgent() {
    return this.headers['user-agent'] || 'unknown';
  }
}

class MyServerResponse extends http2.Http2ServerResponse {
  status(code, contentType) {
    this.writeHead(code, { 'Content-Type': contentType });
  }
}

const server = http2.createServer({
  ServerRequest: MyServerRequest,
  ServerResponse: MyServerResponse
};

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

scatter-plot-requests-2

aggregating variable: streams

 requests        version     rate confidence.interval
     5000       original 6157.151            46.30018
     5000         params 6118.722            52.94544
     5000 setPrototypeOf 6127.015            46.04977
    10000       original 7373.277            54.71278
    10000         params 7318.096            55.83396
    10000 setPrototypeOf 7363.882            46.79015
    20000       original 8003.976            53.02454
    20000         params 8237.659            48.35231
    20000 setPrototypeOf 7999.081            50.75252
    50000       original 7870.669            63.91681
    50000         params 8347.653            54.88024
    50000 setPrototypeOf 7838.831            62.77288

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

scatter-plot-streams-2

aggregating variable: requests

 streams        version     rate confidence.interval
     100       original 7321.768            152.6445
     100         params 7506.819            164.8038
     100 setPrototypeOf 7317.199            143.0783
     500       original 7364.365            149.7678
     500         params 7566.271            174.6796
     500 setPrototypeOf 7385.408            143.0759
    1000       original 7372.903            140.9144
    1000         params 7518.531            172.2680
    1000 setPrototypeOf 7335.217            147.1282
    2500       original 7331.950            145.9298
    2500         params 7502.725            172.1327
    2500 setPrototypeOf 7320.819            157.1094
    5000       original 7385.907            145.3467
    5000         params 7492.245            174.1755
    5000 setPrototypeOf 7302.101            149.7285
   10000       original 7330.715            155.2385
   10000         params 7446.603            190.6992
   10000 setPrototypeOf 7332.468            145.8346

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

  • HTTP/1 createServer() options to pass custom Request and Response classes #15752

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jsumners, bricss, and StarpTech reacted with thumbs up emoji
All reactions
  • 👍 3 reactions
@nodejs-github-bot nodejs-github-bot added the http2 Issues and PRs related to the http2 subsystem. label Sep 22, 2017
lpinca
lpinca reviewed Sep 24, 2017
View reviewed changes
Comment thread doc/api/http2.md Outdated

@lpinca lpinca Sep 24, 2017

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@lpinca lpinca Sep 25, 2017

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread doc/api/http2.md Outdated

@lpinca lpinca Sep 24, 2017

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/http2/core.js Outdated

@lpinca lpinca Sep 24, 2017

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@apapirovski apapirovski Sep 24, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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 .calls can be eliminated.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lpinca reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread benchmark/http2/inheritance.js Outdated

@lpinca lpinca Sep 24, 2017

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread lib/internal/http2/core.js Outdated

@lpinca lpinca Sep 24, 2017

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@apapirovski apapirovski Sep 24, 2017

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
apapirovski
apapirovski reviewed Sep 24, 2017
View reviewed changes
Comment thread lib/internal/http2/core.js Outdated

@apapirovski apapirovski Sep 24, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
apapirovski
apapirovski reviewed Sep 24, 2017
View reviewed changes
Comment thread benchmark/http2/inheritance.js Outdated

@apapirovski apapirovski Sep 24, 2017

Copy link
Copy Markdown
Contributor

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'}`);

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@apapirovski apapirovski Sep 24, 2017

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
apapirovski
apapirovski reviewed Sep 24, 2017
View reviewed changes
Comment thread benchmark/http2/inheritance.js Outdated

@apapirovski apapirovski Sep 24, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@hekike hekike Sep 24, 2017

Copy link
Copy Markdown
Contributor Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@apapirovski

apapirovski commented Sep 24, 2017

Copy link
Copy Markdown
Contributor

Thanks for doing this @hekike! I left some small suggestions for changes but I don't know what opinion @mcollina & @jasnell will have re: whether this is something that gets added or not.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
apapirovski
apapirovski reviewed Sep 24, 2017
View reviewed changes
Comment thread lib/internal/http2/core.js Outdated

@apapirovski apapirovski Sep 24, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jasnell

jasnell commented Sep 24, 2017

Copy link
Copy Markdown
Member

Definitely good with this approach in general but will do a review on Monday.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented Sep 25, 2017

Copy link
Copy Markdown
Member

@dougwilson is this something that you could make some use of in Express. Do you need it also for require('http')?

Express would be the main consumer of this feature, and if do not need it I'm 👎 as it increases the API surface area.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@hekike

hekike commented Sep 25, 2017

Copy link
Copy Markdown
Contributor Author

@mcollina restify could benefit from it as well. The current approach extends the Node Core's IncomingMessage and ServerResponse prototypes. See https://github.com/restify/node-restify/blob/master/lib/request.js#L19 cc @yunong
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented Sep 25, 2017

Copy link
Copy Markdown
Member

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 👍 .
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mcollina
mcollina reviewed Sep 25, 2017
View reviewed changes
Comment thread doc/api/http2.md Outdated

@mcollina mcollina Sep 25, 2017

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@apapirovski

apapirovski commented Sep 25, 2017

Copy link
Copy Markdown
Contributor

If this is going to be added to http2, it will need to be allowHTTP1 aware (so accept IncomingMessage & ServerResponse) which means even larger API surface.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@apapirovski

apapirovski commented Sep 25, 2017

Copy link
Copy Markdown
Contributor

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@hekike
hekike force-pushed the feat/http2-request-response-extend branch from d4a9c75 to be96525 Compare September 25, 2017 12:16
@hekike

hekike commented Sep 25, 2017

Copy link
Copy Markdown
Contributor Author

@apapirovski thanks for checking it. I fixed your suggestions, here is the latest benchmark output after the changes:

By requests:

cat scatter.csv | Rscript benchmark/scatter.R --xaxis requests --category version --plot scatter-plot.png --log

aggregating variable: streams

 requests        version     rate confidence.interval
     5000       original 6017.385           103.08612
     5000         params 6005.038            94.99691
     5000 setPrototypeOf 5899.892           116.80510
    10000       original 7066.274           130.80891
    10000         params 7133.153           118.89924
    10000 setPrototypeOf 7028.677           133.74257
    20000       original 7684.556           144.31313
    20000         params 8041.344           119.24608
    20000 setPrototypeOf 7791.633           111.70869
    50000       original 7754.759           116.79295
    50000         params 8187.066           123.17560
    50000 setPrototypeOf 7827.398           102.14100

X axis: total number of requests
Y axis: rate of operations

scatter-plot-requests-3

By streams:

cat scatter.csv | Rscript benchmark/scatter.R --xaxis streams --category version --plot scatter-plot.png --log

aggregating variable: requests

 streams        version     rate confidence.interval
     100       original 7030.053            183.8673
     100         params 7270.266            232.4791
     100 setPrototypeOf 6951.486            232.5603
     500       original 7154.344            214.5401
     500         params 7466.283            210.6520
     500 setPrototypeOf 7216.214            203.2741
    1000       original 7274.672            187.8303
    1000         params 7414.865            200.5134
    1000 setPrototypeOf 7285.713            183.2550
    2500       original 7206.308            189.6652
    2500         params 7301.195            216.3267
    2500 setPrototypeOf 7090.802            213.5673
    5000       original 7002.380            216.8112
    5000         params 7312.006            192.2240
    5000 setPrototypeOf 7113.685            183.5417
   10000       original 7116.705            193.1916
   10000         params 7285.285            214.3297
   10000 setPrototypeOf 7163.500            184.8569

X axis: concurrency level
Y axis: rate of operations

scatter-plot-streams-3
apapirovski reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@evanlucas

evanlucas commented Sep 25, 2017

Copy link
Copy Markdown
Contributor

I would be +1 on doing this for http as well as we could use it for our internal http framework too
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented Sep 25, 2017

Copy link
Copy Markdown
Member

Can we move the discussion on adding this for require('http')? I think we might get the best value there rather than in require('http2'). Then http2 will follow http.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@hekike

hekike commented Sep 25, 2017

Copy link
Copy Markdown
Contributor Author

@mcollina good idea. The only reason why I started with http2 because I was working on restify's HTTP/2 support.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@dougwilson

dougwilson commented Sep 25, 2017

Copy link
Copy Markdown
Member

@mcollina at least with Express current design, it would not be able to use this. The main things I see are

  1. This can only be set when constructing thr HTTP server instance, which is outside of Express since it sits at a higher level.
  2. The prototype Express sets is app-specific. This means that if I have sub apps, the prototype will change as the request is entering / leaving the sub apps.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented Sep 25, 2017

Copy link
Copy Markdown
Member

@hekike would this means that this functionality is needed only by Restify? I'm trying to understand who needs this functionality.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell approved these changes Sep 25, 2017
View reviewed changes
@yunong

yunong commented Sep 25, 2017

Copy link
Copy Markdown
Member

@mcollina This would be great to have for both HTTP/1 and HTTP/2. We are currently running into multiple problems in restify without this. In fact, we’ve been collaborating closely with @hekike on this issue and are in full support of this PR. 🎉
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@yunong

yunong commented Sep 25, 2017

Copy link
Copy Markdown
Member

@mcollina This is something that is generally useful for most frameworks since it doesn’t require directly modifying the prototype of the request and response objects. Sure this will require the frameworks to pick up this new paradigm, but it is much better than directly modifying the prototype, which is what both restify and express does today.

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@apapirovski

apapirovski commented Sep 25, 2017

Copy link
Copy Markdown
Contributor

@yunong how do you deal with allowHTTP1: true on createSecureServer in that situation? To me that seems to be the missing piece here.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@hekike

hekike commented Sep 25, 2017

Copy link
Copy Markdown
Contributor Author

@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@apapirovski

apapirovski commented Sep 25, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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 allowHTTP1: true. See https://nodejs.org/dist/latest-v8.x/docs/api/http2.html#http2_http2_createsecureserver_options_onrequesthandler and

node/lib/internal/http2/core.js

Lines 2258 to 2269 in 4f4d55d

if (socket.alpnProtocol === false || socket.alpnProtocol === 'http/1.1') {
if (options.allowHTTP1 === true) {
// Fallback to HTTP/1.1
return httpConnectionListener.call(this, socket);
} else if (this.emit('unknownProtocol', socket)) {
// Let event handler deal with the socket
return;
} else {
// Reject non-HTTP/2 client
return socket.destroy();
}
}

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mcollina

mcollina commented Sep 25, 2017

Copy link
Copy Markdown
Member

@hekike can you open up a PR for http? Then we will adjust this accordingly to support both http1 and http2.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@hekike

hekike commented Sep 26, 2017

Copy link
Copy Markdown
Contributor Author

@mcollina sure, it will take a couple of days.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell
jasnell requested a review from mcollina September 26, 2017 20:00
apapirovski
apapirovski reviewed Feb 9, 2018
View reviewed changes
Comment thread doc/api/http2.md Outdated

@apapirovski apapirovski Feb 9, 2018

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Peter Marton Peter Marton
http2: add Http2ServerRequest and Http2ServerResponse options to crea…
bdca0b7 
…teServer
@hekike
hekike force-pushed the feat/http2-request-response-extend branch from 32ccd60 to bdca0b7 Compare February 9, 2018 17:50
@hekike

hekike commented Feb 9, 2018

Copy link
Copy Markdown
Contributor Author

@apapirovski thanks, fixed!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
apapirovski pushed a commit that referenced this pull request Feb 9, 2018
Peter Marton @apapirovski
http2: add req and res options to server creation
e5f101f 
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]>
@apapirovski

apapirovski commented Feb 9, 2018

Copy link
Copy Markdown
Contributor

Thanks for bearing with us @hekike. This took way too long.

(FYI, I made some adjustments to the commit message to make it fit within our guidelines.)

Landed in e5f101f
hekike reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@apapirovski apapirovski closed this Feb 9, 2018
@hekike
hekike deleted the feat/http2-request-response-extend branch February 9, 2018 18:36
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
Peter Marton @MylesBorins
http2: add req and res options to server creation
5dbe33c 
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]>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
Peter Marton @MylesBorins
http2: add req and res options to server creation
21ec460 
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]>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
Peter Marton @MylesBorins
http2: add req and res options to server creation
af31f59 
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]>
@MylesBorins MylesBorins mentioned this pull request Feb 21, 2018
v9.6.0 proposal #18902
Merged
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
Peter Marton @MylesBorins
http2: add req and res options to server creation
c0d6945 
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]>
@gibfahn

gibfahn commented Apr 13, 2018

Copy link
Copy Markdown
Member

Will need a backport if it should go back to v8.x (if you'd like to do a backport please change the label).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gibfahn gibfahn added dont-land-on-v6.x labels Apr 13, 2018
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
Peter Marton @kjin
http2: add req and res options to server creation
7e2181a 
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]>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
Peter Marton @MylesBorins
http2: add req and res options to server creation
ae2ebde 
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]>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
v8.11.2 proposal #20478
Merged
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Peter Marton @MayaLekova
http2: add req and res options to server creation
054fe91 
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]>
MylesBorins pushed a commit that referenced this pull request May 15, 2018
Peter Marton @MylesBorins
http2: add req and res options to server creation
0aa7698 
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]>
@MylesBorins MylesBorins removed the dont-land-on-v8.x label May 15, 2018
MylesBorins pushed a commit that referenced this pull request May 15, 2018
Peter Marton @MylesBorins
http2: add req and res options to server creation
81a4abe 
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]>
MylesBorins pushed a commit that referenced this pull request May 15, 2018
Peter Marton @MylesBorins
http2: add req and res options to server creation
852b996 
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]>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
Peter Marton @MylesBorins
http2: add req and res options to server creation
903a50e 
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]>
@MylesBorins MylesBorins mentioned this pull request Jul 9, 2018
v8.12.0 proposal #21593
Merged
rvagg pushed a commit that referenced this pull request Aug 16, 2018
Peter Marton @rvagg
http2: add req and res options to server creation
b1b0486 
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]>
@chyzwar chyzwar mentioned this pull request Feb 13, 2019
[node] Improve http2 definitions DefinitelyTyped/DefinitelyTyped#33019
Closed
7 of 9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes
@lpinca lpinca lpinca left review comments
@mcollina mcollina mcollina approved these changes
@addaleax addaleax addaleax approved these changes
+1 more reviewer
@apapirovski apapirovski apapirovski approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. http Issues and PRs related to the http subsystem. http2 Issues and PRs related to the http2 subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

13 participants

@hekike @apapirovski @jasnell @mcollina @evanlucas @dougwilson @yunong @BridgeAR @addaleax @gibfahn @lpinca @MylesBorins @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.