Skip to content

Conversation

@hrrrrustic
Copy link
Contributor

No description provided.

@ghost ghost added the area-System.Net.Http label Jul 14, 2021
@ghost
Copy link

ghost commented Jul 14, 2021

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

Issue Details
Author: hrrrrustic
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

private static bool IsPredefinedScheme(ReadOnlySpan<char> scheme)
{
if (scheme == null || scheme.Length < 3)
if (scheme.Length < 3)
Copy link
Member

Choose a reason for hiding this comment

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

A lot of the code below is now incorrect. All of the equality operators are going to do the wrong thing.

Copy link
Member

Choose a reason for hiding this comment

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

if (MaybeUri(_rawUrl!.ToLowerInvariant()) && Uri.TryCreate(_rawUrl, UriKind.Absolute, out raw_uri))
path = raw_uri.PathAndQuery;
else
path = _rawUrl;

I don't see how this is supposed to be better than just calling into Uri directly.
IMO the right optimization here would be to remove the MaybeUri entirely.

Copy link
Contributor Author

@hrrrrustic hrrrrustic Jul 14, 2021

Choose a reason for hiding this comment

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

You mean just to remove MaybeUri(_rawUrl!.ToLowerInvariant()) &&? (And the MaybeUri body)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess this is the exact same logic that is called in Uri.TryCreate

private static unsafe bool CheckKnownSchemes(long* lptr, int nChars, ref UriParser? syntax)
{
//NOTE beware of too short input buffers!
const long _HTTP_Mask0 = 'h' | ('t' << 16) | ((long)'t' << 32) | ((long)'p' << 48);
const char _HTTPS_Mask1 = 's';
const int _WS_Mask = 'w' | ('s' << 16);
const long _WSS_Mask = 'w' | ('s' << 16) | ((long)'s' << 32) | ((long)':' << 48);
const long _FTP_Mask = 'f' | ('t' << 16) | ((long)'p' << 32) | ((long)':' << 48);
const long _FILE_Mask0 = 'f' | ('i' << 16) | ((long)'l' << 32) | ((long)'e' << 48);
const long _GOPHER_Mask0 = 'g' | ('o' << 16) | ((long)'p' << 32) | ((long)'h' << 48);
const int _GOPHER_Mask1 = 'e' | ('r' << 16);
const long _MAILTO_Mask0 = 'm' | ('a' << 16) | ((long)'i' << 32) | ((long)'l' << 48);

Copy link
Member

Choose a reason for hiding this comment

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

You mean just to remove MaybeUri(_rawUrl!.ToLowerInvariant()) &&? (And the MaybeUri body)

Yes, if Uri.TryCreate isn't able to perform well here, it should be improved instead. Duplicating such logic makes no sense.

Copy link
Member

Choose a reason for hiding this comment

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

Some of the failures may be due to the fact Uri accepts implicit file Uris as "absolute", so just the path may be accepted as absolute.
Could be mitigated by replacing MaybeUri with StartsWith("http", OrdinalIgnoreCase) or just reverting the change.

@hrrrrustic hrrrrustic changed the title Remove string allocation in HttpListenerRequest Remove redundant uri check in HttpListenerRequest Jul 16, 2021
@hrrrrustic hrrrrustic marked this pull request as draft July 16, 2021 08:35
@terrajobst terrajobst added the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2021
@hrrrrustic hrrrrustic closed this Jul 21, 2021
@hrrrrustic hrrrrustic reopened this Jul 21, 2021
@hrrrrustic
Copy link
Contributor Author

hrrrrustic commented Jul 21, 2021

😢
Thought that this will retrigger CI

UPD: Oh, yes, it works 😄

@karelz
Copy link
Member

karelz commented Jul 21, 2021

Http3_MsQuic test failures are tracked in #56090. Should be fixed and merged shortly. Sorry for the inconvenience!

@hrrrrustic hrrrrustic closed this Jul 22, 2021
@hrrrrustic hrrrrustic reopened this Jul 22, 2021
@hrrrrustic hrrrrustic closed this Aug 12, 2021
@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.

Labels

area-System.Net.Http community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants