-
Notifications
You must be signed in to change notification settings - Fork 10.9k
Optimizing ChunkingCookieManager #31625
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
Changes from 10 commits
6157a0b
7922591
5f811cd
57a9b65
9a55381
2a2d15a
df9ee2f
7710230
ffb399b
3a11950
ab3e803
d312bc9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| #nullable enable | ||
| *REMOVED*Microsoft.AspNetCore.Http.Features.FeatureCollection.Set<TFeature>(TFeature instance) -> void | ||
| *REMOVED*Microsoft.AspNetCore.Http.Features.IFeatureCollection.Get<TFeature>() -> TFeature | ||
| *REMOVED*Microsoft.AspNetCore.Http.Features.IFeatureCollection.Set<TFeature>(TFeature instance) -> void | ||
| *REMOVED*Microsoft.AspNetCore.Http.Features.IHttpBufferingFeature | ||
| *REMOVED*Microsoft.AspNetCore.Http.Features.IHttpBufferingFeature.DisableRequestBuffering() -> void | ||
| *REMOVED*Microsoft.AspNetCore.Http.Features.IHttpBufferingFeature.DisableResponseBuffering() -> void | ||
| *REMOVED*Microsoft.AspNetCore.Http.Features.IHttpSendFileFeature | ||
| *REMOVED*Microsoft.AspNetCore.Http.Features.IHttpSendFileFeature.SendFileAsync(string! path, long offset, long? count, System.Threading.CancellationToken cancellation) -> System.Threading.Tasks.Task! | ||
| *REMOVED*Microsoft.AspNetCore.Http.Features.IServerVariablesFeature.this[string! variableName].get -> string! | ||
| *REMOVED*Microsoft.AspNetCore.Http.ISession.TryGetValue(string! key, out byte[]! value) -> bool | ||
| Microsoft.AspNetCore.Http.Features.FeatureCollection.Set<TFeature>(TFeature? instance) -> void | ||
| Microsoft.AspNetCore.Http.Features.IFeatureCollection.Get<TFeature>() -> TFeature? | ||
| Microsoft.AspNetCore.Http.Features.IFeatureCollection.Set<TFeature>(TFeature? instance) -> void | ||
| Microsoft.AspNetCore.Http.Features.IServerVariablesFeature.this[string! variableName].get -> string? | ||
| Microsoft.AspNetCore.Http.ISession.TryGetValue(string! key, out byte[]? value) -> bool | ||
| Microsoft.AspNetCore.Http.Features.FeatureCollection.FeatureCollection(int initialCapacity) -> void | ||
| Microsoft.AspNetCore.Http.IResponseCookies.Append(System.ReadOnlySpan<System.Collections.Generic.KeyValuePair<string!, string!>> keyValuePairs, Microsoft.AspNetCore.Http.CookieOptions! options) -> void |
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| #nullable enable | ||
| *REMOVED*Microsoft.AspNetCore.Http.Features.FeatureCollection.Set<TFeature>(TFeature instance) -> void | ||
| *REMOVED*Microsoft.AspNetCore.Http.Features.IFeatureCollection.Get<TFeature>() -> TFeature | ||
| *REMOVED*Microsoft.AspNetCore.Http.Features.IFeatureCollection.Set<TFeature>(TFeature instance) -> void | ||
| *REMOVED*Microsoft.AspNetCore.Http.Features.IHttpBufferingFeature | ||
| *REMOVED*Microsoft.AspNetCore.Http.Features.IHttpBufferingFeature.DisableRequestBuffering() -> void | ||
| *REMOVED*Microsoft.AspNetCore.Http.Features.IHttpBufferingFeature.DisableResponseBuffering() -> void | ||
| *REMOVED*Microsoft.AspNetCore.Http.Features.IHttpSendFileFeature | ||
| *REMOVED*Microsoft.AspNetCore.Http.Features.IHttpSendFileFeature.SendFileAsync(string! path, long offset, long? count, System.Threading.CancellationToken cancellation) -> System.Threading.Tasks.Task! | ||
| *REMOVED*Microsoft.AspNetCore.Http.Features.IServerVariablesFeature.this[string! variableName].get -> string! | ||
| *REMOVED*Microsoft.AspNetCore.Http.ISession.TryGetValue(string! key, out byte[]! value) -> bool | ||
| Microsoft.AspNetCore.Http.Features.FeatureCollection.Set<TFeature>(TFeature? instance) -> void | ||
| Microsoft.AspNetCore.Http.Features.IFeatureCollection.Get<TFeature>() -> TFeature? | ||
| Microsoft.AspNetCore.Http.Features.IFeatureCollection.Set<TFeature>(TFeature? instance) -> void | ||
| Microsoft.AspNetCore.Http.Features.IServerVariablesFeature.this[string! variableName].get -> string? | ||
| Microsoft.AspNetCore.Http.ISession.TryGetValue(string! key, out byte[]? value) -> bool | ||
| Microsoft.AspNetCore.Http.Features.FeatureCollection.FeatureCollection(int initialCapacity) -> void |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -88,6 +88,56 @@ public void Append(string key, string value, CookieOptions options) | |||||||||||||||||||||||||||
| Headers[HeaderNames.SetCookie] = StringValues.Concat(Headers[HeaderNames.SetCookie], cookieValue); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| /// <inheritdoc /> | ||||||||||||||||||||||||||||
| public void Append(ReadOnlySpan<KeyValuePair<string, string>> keyValuePairs, CookieOptions options) | ||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||
| if (options == null) | ||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||
| throw new ArgumentNullException(nameof(options)); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // SameSite=None cookies must be marked as Secure. | ||||||||||||||||||||||||||||
| if (!options.Secure && options.SameSite == SameSiteMode.None) | ||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||
| if (_logger == null) | ||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||
| var services = _features.Get<Features.IServiceProvidersFeature>()?.RequestServices; | ||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We want to log from these components like this.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the review, I kind of feel there is something missing after "like this", could you please tell me if I am missing something here?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @davidfowl this was copied from the existing code above, we can discuss it separately from this PR. aspnetcore/src/Http/Http/src/Internal/ResponseCookies.cs Lines 59 to 71 in 00b551e
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct I did not fully look at this yet and just copied from the original |
||||||||||||||||||||||||||||
| _logger = services?.GetService<ILogger<ResponseCookies>>(); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| if (_logger != null) | ||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||
| foreach (var keyValuePair in keyValuePairs) | ||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||
| Log.SameSiteCookieNotSecure(_logger, keyValuePair.Key); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| var setCookieHeaderValue = new SetCookieHeaderValue(string.Empty) | ||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||
| Domain = options.Domain, | ||||||||||||||||||||||||||||
| Path = options.Path, | ||||||||||||||||||||||||||||
| Expires = options.Expires, | ||||||||||||||||||||||||||||
| MaxAge = options.MaxAge, | ||||||||||||||||||||||||||||
| Secure = options.Secure, | ||||||||||||||||||||||||||||
| SameSite = (Net.Http.Headers.SameSiteMode)options.SameSite, | ||||||||||||||||||||||||||||
| HttpOnly = options.HttpOnly | ||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| var cookierHeaderValue = setCookieHeaderValue.ToString()[1..]; | ||||||||||||||||||||||||||||
| var cookies = new string[keyValuePairs.Length]; | ||||||||||||||||||||||||||||
| var position = 0; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| foreach (var keyValuePair in keyValuePairs) | ||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||
| cookies[position] = string.Concat(_enableCookieNameEncoding ? Uri.EscapeDataString(keyValuePair.Key) : keyValuePair.Key, "=", Uri.EscapeDataString(keyValuePair.Value), cookierHeaderValue); | ||||||||||||||||||||||||||||
|
uabarahona marked this conversation as resolved.
Outdated
|
||||||||||||||||||||||||||||
| position++; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| Headers.Append(HeaderNames.SetCookie, cookies); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| /// <inheritdoc /> | ||||||||||||||||||||||||||||
| public void Delete(string key) | ||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| using BenchmarkDotNet.Attributes; | ||
| using Microsoft.AspNetCore.Authentication.Cookies; | ||
| using Microsoft.AspNetCore.Http; | ||
| using Microsoft.Extensions.Primitives; | ||
| using Microsoft.Net.Http.Headers; | ||
|
|
||
| namespace Microsoft.AspNetCore.Security | ||
| { | ||
| public class ChunkingCookieManagerBenchmark | ||
| { | ||
| private ChunkingCookieManager _chunkingCookieManager; | ||
| private HttpContext _httpContext; | ||
| private CookieOptions _cookieOptions; | ||
| private string _stringToAdd; | ||
|
|
||
| [GlobalSetup] | ||
| public void GlobalSetup() | ||
| { | ||
| _chunkingCookieManager = new ChunkingCookieManager() | ||
| { | ||
| ChunkSize = 86 | ||
| }; | ||
|
|
||
| _httpContext = new DefaultHttpContext(); | ||
|
|
||
| _cookieOptions = new CookieOptions() | ||
| { | ||
| Domain = "foo.com", | ||
| Path = "/", | ||
| Secure = true | ||
| }; | ||
|
|
||
| _httpContext.Request.Headers["Cookie"] = new[] | ||
| { | ||
| "TestCookie=chunks-7; domain=foo.com; path=/; secure", | ||
|
uabarahona marked this conversation as resolved.
Outdated
|
||
| "TestCookieC1=abcdefghi; domain=foo.com; path=/; secure", | ||
| "TestCookieC2=jklmnopqr; domain=foo.com; path=/; secure", | ||
| "TestCookieC3=stuvwxyz0; domain=foo.com; path=/; secure", | ||
| "TestCookieC4=123456789; domain=foo.com; path=/; secure", | ||
| "TestCookieC5=ABCDEFGHI; domain=foo.com; path=/; secure", | ||
| "TestCookieC6=JKLMNOPQR; domain=foo.com; path=/; secure", | ||
| "TestCookieC7=STUVWXYZ; domain=foo.com; path=/; secure" | ||
| }; | ||
|
|
||
| _stringToAdd = "abcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ"; | ||
| } | ||
|
|
||
|
|
||
| [Benchmark] | ||
| public void AppendCookies() | ||
| { | ||
| _chunkingCookieManager.AppendResponseCookie(_httpContext, "TestCookie1", _stringToAdd, _cookieOptions); | ||
| _httpContext.Response.Headers[HeaderNames.SetCookie] = StringValues.Empty; | ||
| } | ||
|
|
||
| [Benchmark] | ||
| public void DeleteCookies() | ||
| { | ||
| _chunkingCookieManager.DeleteCookie(_httpContext, "TestCookie", _cookieOptions); | ||
| _httpContext.Response.Headers[HeaderNames.SetCookie] = StringValues.Empty; | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.