Skip to content

Enable HTTP/2 & Push Promise #5856

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

Closed
Tratcher opened this issue Apr 6, 2015 · 22 comments
Closed

Enable HTTP/2 & Push Promise #5856

Tratcher opened this issue Apr 6, 2015 · 22 comments
Labels
affected-few This issue impacts only small number of customers area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-httpsys severity-major This label is used by an internal tool
Milestone

Comments

@Tratcher
Copy link
Member

Tratcher commented Apr 6, 2015

Edit: Proof of concept provided below https://github.com/aspnet/HttpSysServer/issues/106#issuecomment-291283012

How do we detect if the request is HTTP/2 before calling the Push method?

Also, should/can we expose things like request priority?

Fowler has asked us to set the HttpContext.Request.Protocol to "http/2" rather than it's current "http/1.1". HttpClient is doing something similar.

Response headers and body can be streamlined. HTTP/2 does not require us to manually chunk the response. All responses are treated like the old 1.0 connection:close mode.

@Tratcher
Copy link
Member Author

@muratg
Copy link
Contributor

muratg commented Jul 25, 2016

@DamianEdwards Do we want HTTP/2 in WebListener?

(just for comparison - we won't be doing it in Kestrel any time soon)

cc @shirhatti

@DamianEdwards
Copy link
Member

Not a high priority for now. Backlog it until we get more customers asking for it.

@Tratcher
Copy link
Member Author

RE: aspnet/HttpAbstractions#371

@Tratcher
Copy link
Member Author

The HTTP_REQUEST_FLAG_HTTP2 flag was added to HTTP_REQUEST.Flags some time after Win10 RTM.
https://msdn.microsoft.com/en-us/library/windows/desktop/aa364612(v=vs.85).aspx

@darrelmiller
Copy link

Enabling HTTP/2 for WebListener would allow API Management to support HTTP/2 and we have internal and external customers asking for it.

@Tratcher
Copy link
Member Author

Tratcher commented Dec 9, 2016

@darrelmiller HTTP/2 is already available (Win10), no changes were required, only PushPromise is missing.

@darrelmiller
Copy link

I just tried making a HTTP/2 request using HttpClient with WinHttpHandler and WebListener hosted using Damian's OwinWebListenerHost and it wasn't working. Maybe I'm just doing it wrong.

@Tratcher
Copy link
Member Author

Tratcher commented Dec 9, 2016

A) SSL is required. B) it's hard to tell if you succeeded, nothing in the server data structures tells you. Your best bet is to use a browser and its debugger tools.

@darrelmiller
Copy link

Thanks. I was just being dense. It is making HTTP/2 requests now. Do I actually need to migrate to WebListener, or would HttpListener work automatically too?

@Tratcher
Copy link
Member Author

Tratcher commented Dec 9, 2016

WebListener, HttpListener and IIS all inherit the same HTTP/2 functionality from Http.Sys.

@damianh
Copy link

damianh commented Dec 16, 2016

@darrelmiller OwinWebListenerHost is fork of the katana HttpListener impl. It may need some changes to support HTTP/2...

@darrelmiller
Copy link

@damianh It did work, but I actually didn't need to change to WebListener at all. HttpListener worked just fine with HTTP/2.

@tpeczek
Copy link

tpeczek commented Apr 3, 2017

Hi,

I did an implementation of HTTP/2 with Server Push for HttpSysServer here and described it here. Please let me know if this could be a valuable pull request (if the design is in line with team approach and this is a right time). Of course I would first make a pull request with IHttp2Feature to HttpAbstractions as it seem to belong there.

@Tratcher
Copy link
Member Author

Tratcher commented Apr 3, 2017

@tpeczek Looks like a good start. I think you duplicated existing code in several places (e.g. IndexOfKnownHeader).

I don't think we'd merge it until we look at the whole end-to-end, especially for the Feature interface. aspnet/HttpAbstractions#371. We want to make sure to expose everything we need to and that it works well for both HttpSysServer and Kestrel.

@tpeczek
Copy link

tpeczek commented Apr 4, 2017

@Tratcher Yes the HTTP_REQUEST_HEADER_ID is a lazy copy of HTTP_RESPONSE_HEADER_ID while they should share a base class to avoid duplication. There are probably other places which can be improved as well (for example a private method reused by both SerializeHeaders for not known headers etc.).

I'll be happy to iterate over this to get this into acceptable state if/when team decides this is something to go with.

@Tratcher
Copy link
Member Author

Tratcher commented Apr 4, 2017

We are definitely doing HTTP/2, just not any time soon. Too many of Kestrel's dependencies are blocked.

You've got the basic outline we'll need, feel free to keep iterating and we'll pick it up when we're ready.

@tpeczek
Copy link

tpeczek commented Apr 6, 2017

@Tratcher I understand. I'll try to tweak some things in my spare time to make sure it is in decent shape if you ever decide to use it.

Tratcher referenced this issue in aspnet/HttpSysServer Aug 7, 2018
@Tratcher
Copy link
Member Author

Tratcher commented Aug 8, 2018

Request.Protocol has been set to "HTTP/2" for 2.2. Push promise is not in scope for this release.

@BKStrelioff
Copy link

Where is HTTP/2 Push in the roadmap? Should we use what tpeczek has done to date until official support released?

@Tratcher
Copy link
Member Author

Maybe for 3.0.

@aspnet-hello aspnet-hello transferred this issue from aspnet/HttpSysServer Dec 18, 2018
@aspnet-hello aspnet-hello added this to the Backlog milestone Dec 18, 2018
@aspnet-hello aspnet-hello added area-servers enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-httpsys labels Dec 18, 2018
@Tratcher Tratcher added affected-few This issue impacts only small number of customers severity-major This label is used by an internal tool labels Oct 12, 2020 — with ASP.NET Core Issue Ranking
@Tratcher
Copy link
Member Author

Tratcher commented Dec 4, 2020

Closing as we've decided not to support push. See #4249 (comment) for details.

@Tratcher Tratcher closed this as completed Dec 4, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Jan 3, 2021
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affected-few This issue impacts only small number of customers area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-httpsys severity-major This label is used by an internal tool
Projects
None yet
Development

No branches or pull requests

9 participants