Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add SslApplicationProtocol.Http3 #56775

Merged
merged 7 commits into from
Aug 4, 2021
Merged

Add SslApplicationProtocol.Http3 #56775

merged 7 commits into from
Aug 4, 2021

Conversation

i3arnon
Copy link
Contributor

@i3arnon i3arnon commented Aug 3, 2021

Fix #1293

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 3, 2021
@ghost
Copy link

ghost commented Aug 3, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix #1293

Author: i3arnon
Assignees: -
Labels:

area-System.Net.Http, new-api-needs-documentation
Milestone: -

stephentoub
stephentoub reviewed Aug 3, 2021
View reviewed changes
src/libraries/System.Net.Security/src/System/Net/Security/SslApplicationProtocol.cs
private static readonly byte[] s_http2Utf8 = new byte[] { 0x68, 0x32 }; // "h2"
private static readonly byte[] s_http11Utf8 = new byte[] { 0x68, 0x74, 0x74, 0x70, 0x2f, 0x31, 0x2e, 0x31 }; // "http/1.1"

// Refer to IANA on ApplicationProtocols: https://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml#alpn-protocol-ids
// h2
/// <summary>Defines a <see cref="SslApplicationProtocol"/> instance for HTTP 3.0.</summary>
public static readonly SslApplicationProtocol Http3 = new SslApplicationProtocol(s_http3Utf8, copy: false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karelz, should this require preview? Should

runtime/src/libraries/System.Net.Primitives/src/System/Net/HttpVersion.cs

Line 17 in 79c4144

public static readonly Version Version30 = new Version(3, 0);
?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ATM, we don't do that for HttpVersion.Http3 since you can easily construct the version manually and this is very similar.

Either way, if we decide to annotate this, we should be consistent and do the same thing with Http3 version.

ManickaP
ManickaP reviewed Aug 3, 2021
View reviewed changes
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are also places in the loopback server and h/3 tests where "h3" is used and should be replaced as well.

src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionPool.cs Outdated Show resolved Hide resolved
@i3arnon
Copy link
Contributor Author

i3arnon commented Aug 3, 2021 •
edited
Loading

There are also places in the loopback server and h/3 tests where "h3" is used and should be replaced as well.

Should Http3Options.Alpn (and relevant references) now be SslApplicationProtocol.Http3.ToString() calls?
It won't actually allocate a new string as that's handled inside the ToString but it does add a reference equality check to each instantiation..

@ManickaP
Copy link
Member

ManickaP commented Aug 3, 2021

There are also places in the loopback server and h/3 tests where "h3" is used and should be replaced as well.

Should Http3Options.Alpn (and relevant references) now be SslApplicationProtocol.Http3.ToString() calls?
It won't actually allocate a new string as that's handled inside the ToString but it does add a reference equality check call to each instantiation..

Since it's only in tests, it doesn't matter that much. You could also change the type from string to SslApplicationProtocol but I'm not sure how much will that help.

ManickaP
ManickaP approved these changes Aug 4, 2021
View reviewed changes
Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution, LGTM.

@ManickaP ManickaP merged commit 6468942 into dotnet:main Aug 4, 2021
@i3arnon i3arnon deleted the http3-api branch August 4, 2021 12:34
This was referenced Aug 5, 2021
Merged
Open
antonfirsov added a commit that referenced this pull request Aug 6, 2021
@antonfirsov
Fix HttpStress build (#56912)
6f19f67 
#56775 broke HttpStress.
@karelz karelz added this to the 6.0.0 milestone Aug 17, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Sep 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub left review comments

@geoffkizer geoffkizer geoffkizer left review comments

@ManickaP ManickaP ManickaP approved these changes

Assignees
No one assigned
Labels
area-System.Net.Http community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

Add HTTP/3 APIs
5 participants
@i3arnon @ManickaP @stephentoub @geoffkizer @karelz