Refactored to use the fields from the HttpMethods class #1128
Conversation
|
Hi @mikaelm12, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! The agreement was validated by .NET Foundation and real humans are currently evaluating your PR. TTYL, DNFBOT; |
| [InlineData("PUT / HTTP/1.1", ' ', true, MemoryPoolIteratorExtensions.HttpPutMethod)] | ||
| [InlineData("OPTIONS / HTTP/1.1", ' ', true, MemoryPoolIteratorExtensions.HttpOptionsMethod)] | ||
| [InlineData("TRACE / HTTP/1.1", ' ', true, MemoryPoolIteratorExtensions.HttpTraceMethod)] | ||
| [InlineData("CONNECT / HTTP/1.1", ' ', true,"CONNECT")] |
There was a problem hiding this comment.
Any reason to hardcode "CONNECT" rather than using HttpMethods.Connect, as is done in the product code?
There was a problem hiding this comment.
Can't use HttpMethods.Connect here because its not a const.
| @@ -279,7 +270,7 @@ public static bool GetKnownMethod(this MemoryPoolIterator begin, out string know | |||
|
|
|||
| if ((value & _mask4Chars) == _httpGetMethodLong) | |||
| { | |||
| knownMethod = HttpGetMethod; | |||
| knownMethod = HttpMethods.Get; | |||
There was a problem hiding this comment.
Might be a tiny performance degradation since the value is changing from const to static readonly. Is there a reason the properties on HttpMethods are static readonly rather than const? I see the HttpMethods type is new for 1.1, so we should be able to change the properties to const now. After 1.1 ships it would be a breaking change.
I don't see any possible way the value of these properties would change in the future, so const should be safe.
There was a problem hiding this comment.
There was a problem hiding this comment.
I doubt the performance will degrade
| @@ -12,16 +13,6 @@ public static class MemoryPoolIteratorExtensions | |||
| { | |||
| private static readonly Encoding _utf8 = Encoding.UTF8; | |||
|
|
|||
| public const string HttpConnectMethod = "CONNECT"; | |||
There was a problem hiding this comment.
Are breaking changes to public APIs on internal types allowed in 1.1?
There was a problem hiding this comment.
It's in an "Internal" namespace.
| @@ -12,16 +13,6 @@ public static class MemoryPoolIteratorExtensions | |||
| { | |||
| private static readonly Encoding _utf8 = Encoding.UTF8; | |||
|
|
|||
| public const string HttpConnectMethod = "CONNECT"; | |||
There was a problem hiding this comment.
It's in an "Internal" namespace.
| @@ -279,7 +270,7 @@ public static bool GetKnownMethod(this MemoryPoolIterator begin, out string know | |||
|
|
|||
| if ((value & _mask4Chars) == _httpGetMethodLong) | |||
| { | |||
| knownMethod = HttpGetMethod; | |||
| knownMethod = HttpMethods.Get; | |||
There was a problem hiding this comment.
|
@mikaelm12 You know how to run the plaintext benchmarks, right? I really don't expect this change will make any difference, but it's easy enough to test using @mikeharder's BenchmarksDriver. |
| [InlineData("PUT / HTTP/1.1", ' ', true, MemoryPoolIteratorExtensions.HttpPutMethod)] | ||
| [InlineData("OPTIONS / HTTP/1.1", ' ', true, MemoryPoolIteratorExtensions.HttpOptionsMethod)] | ||
| [InlineData("TRACE / HTTP/1.1", ' ', true, MemoryPoolIteratorExtensions.HttpTraceMethod)] | ||
| [InlineData("CONNECT / HTTP/1.1", ' ', true,"CONNECT")] |
There was a problem hiding this comment.
Could use MemberData but meh. We could have a second set of tests that verify reference equality.
There was a problem hiding this comment.
| @@ -22,7 +22,8 @@ | |||
| "Microsoft.Extensions.TaskCache.Sources": { | |||
| "version": "1.1.0-*", | |||
| "type": "build" | |||
| } | |||
| }, | |||
| "Microsoft.AspNetCore.Http.Abstractions": "1.1.0-*" | |||
There was a problem hiding this comment.
Hmmmmm, this is a new dependency? Doesn't this come in transitively already?
There was a problem hiding this comment.
Yeah, it should be coming from Microsoft.AspNetCore.Hosting transitively.
@davidfowl @halter73 @muratg @Tratcher