Optimizing ChunkingCookieManager (#31625)

uabarahona · Tratcher · web-flow · commit 3c228e5b15bd · 2021-04-20T19:22:14.000Z
* Adding initial refactor to improve perfromance on ChunkingCookieManager

* Moving new API to correct file

* Adding some interface implementation for tests

* Addressing feedback given by asp.net team

* Remove spaces

* Changing to use readonlyspan

* Adding DIM as discussion point

* CollectionMarshal

* Removing unnecesary implementations and fixing public api

* Fixing public api for different targets

* Addressing feedback from asp.net team

* Update src/Http/Http.Features/src/IResponseCookies.cs

Co-authored-by: Chris Ross &lt;Tratcher@Outlook.com&gt;
diff --git a/src/Http/Http.Features/src/IResponseCookies.cs b/src/Http/Http.Features/src/IResponseCookies.cs
@@ -1,6 +1,11 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+#if NET6_0_OR_GREATER
+using System;
+using System.Collections.Generic;
+#endif
+
 namespace Microsoft.AspNetCore.Http
 {
     /// <summary>
@@ -23,6 +28,21 @@ public interface IResponseCookies
         /// <param name="options"><see cref="CookieOptions"/> included in the new cookie setting.</param>
         void Append(string key, string value, CookieOptions options);
 
+#if NET6_0_OR_GREATER
+        /// <summary>
+        /// Add elements of specified collection as cookies.
+        /// </summary>
+        /// <param name="keyValuePairs">Key value pair collections whose elements will be added as cookies.</param>
+        /// <param name="options"><see cref="CookieOptions"/> included in new cookie settings.</param>
+        void Append(ReadOnlySpan<KeyValuePair<string, string>> keyValuePairs, CookieOptions options)
+        {
+            foreach (var keyValuePair in keyValuePairs)
+            {
+                Append(keyValuePair.Key, keyValuePair.Value, options);
+            }
+        }
+#endif
+
         /// <summary>
         /// Sets an expired cookie.
         /// </summary>
diff --git a/src/Http/Http.Features/src/Microsoft.AspNetCore.Http.Features.csproj b/src/Http/Http.Features/src/Microsoft.AspNetCore.Http.Features.csproj
@@ -1,4 +1,4 @@
-<Project Sdk="Microsoft.NET.Sdk">
+﻿<Project Sdk="Microsoft.NET.Sdk">
 
   <PropertyGroup>
     <Description>ASP.NET Core HTTP feature interface definitions.</Description>
@@ -10,10 +10,14 @@
     <Nullable>enable</Nullable>
   </PropertyGroup>
 
-
   <ItemGroup>
     <Reference Include="Microsoft.Extensions.Primitives" />
     <Reference Include="System.IO.Pipelines" />
   </ItemGroup>
 
+  <ItemGroup>
+    <AdditionalFiles Include="PublicAPI/$(TargetFramework)/PublicAPI.Shipped.txt" />
+    <AdditionalFiles Include="PublicAPI/$(TargetFramework)/PublicAPI.Unshipped.txt" />
+  </ItemGroup>
+
 </Project>
diff --git a/src/Http/Http.Features/src/PublicAPI/net461/PublicAPI.Shipped.txt b/src/Http/Http.Features/src/PublicAPI/net461/PublicAPI.Shipped.txt
diff --git a/src/Http/Http.Features/src/PublicAPI/net461/PublicAPI.Unshipped.txt b/src/Http/Http.Features/src/PublicAPI/net461/PublicAPI.Unshipped.txt
@@ -14,4 +14,4 @@ Microsoft.AspNetCore.Http.Features.IFeatureCollection.Get<TFeature>() -> TFeatur
 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.Features.FeatureCollection.FeatureCollection(int initialCapacity) -> void
diff --git a/src/Http/Http.Features/src/PublicAPI/net6.0/PublicAPI.Shipped.txt b/src/Http/Http.Features/src/PublicAPI/net6.0/PublicAPI.Shipped.txt
diff --git a/src/Http/Http.Features/src/PublicAPI/net6.0/PublicAPI.Unshipped.txt b/src/Http/Http.Features/src/PublicAPI/net6.0/PublicAPI.Unshipped.txt
@@ -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
diff --git a/src/Http/Http.Features/src/PublicAPI/netstandard2.0/PublicAPI.Shipped.txt b/src/Http/Http.Features/src/PublicAPI/netstandard2.0/PublicAPI.Shipped.txt
diff --git a/src/Http/Http.Features/src/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt b/src/Http/Http.Features/src/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt
@@ -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
diff --git a/src/Http/Http/src/Internal/ResponseCookies.cs b/src/Http/Http/src/Internal/ResponseCookies.cs
@@ -88,6 +88,57 @@ 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<IServiceProvidersFeature>()?.RequestServices;
+                    _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)
+            {
+                var key = _enableCookieNameEncoding ? Uri.EscapeDataString(keyValuePair.Key) : keyValuePair.Key;
+                cookies[position] = string.Concat(key, "=", Uri.EscapeDataString(keyValuePair.Value), cookierHeaderValue);
+                position++;
+            }
+
+            Headers.Append(HeaderNames.SetCookie, cookies);
+        }
+
         /// <inheritdoc />
         public void Delete(string key)
         {
diff --git a/src/Mvc/Mvc.ViewFeatures/test/CookieTempDataProviderTest.cs b/src/Mvc/Mvc.ViewFeatures/test/CookieTempDataProviderTest.cs
@@ -501,4 +501,4 @@ public override byte[] Serialize(IDictionary<string, object> values)
             }
         }
     }
-}
+}
diff --git a/src/Security/CookiePolicy/src/ResponseCookiesWrapper.cs b/src/Security/CookiePolicy/src/ResponseCookiesWrapper.cs
@@ -2,7 +2,9 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using System.Collections.Generic;
 using System.Diagnostics;
+using System.Runtime.InteropServices;
 using Microsoft.AspNetCore.Builder;
 using Microsoft.AspNetCore.Http;
 using Microsoft.AspNetCore.Http.Features;
@@ -152,6 +154,33 @@ public void Append(string key, string value, CookieOptions options)
             }
         }
 
+        public void Append(ReadOnlySpan<KeyValuePair<string, string>> keyValuePairs, CookieOptions options)
+        {
+            if (options == null)
+            {
+                throw new ArgumentNullException(nameof(options));
+            }
+
+            var nonSuppressedValues = new List<KeyValuePair<string, string>>(keyValuePairs.Length);
+
+            foreach (var keyValuePair in keyValuePairs)
+            {
+                var key = keyValuePair.Key;
+                var value = keyValuePair.Value;
+
+                if (ApplyAppendPolicy(ref key, ref value, options))
+                {
+                    nonSuppressedValues.Add(KeyValuePair.Create(key, value));
+                }
+                else
+                {
+                    _logger.CookieSuppressed(keyValuePair.Key);
+                }
+            }
+
+            Cookies.Append(CollectionsMarshal.AsSpan(nonSuppressedValues), options);
+        }
+
         private bool ApplyAppendPolicy(ref string key, ref string value, CookieOptions options)
         {
             var issueCookie = CanTrack || options.IsEssential;
diff --git a/src/Security/CookiePolicy/test/CookieChunkingTests.cs b/src/Security/CookiePolicy/test/CookieChunkingTests.cs
@@ -3,6 +3,7 @@
 
 using System;
 using Microsoft.AspNetCore.Http;
+using Microsoft.Net.Http.Headers;
 using Xunit;
 
 namespace Microsoft.AspNetCore.Internal
@@ -30,7 +31,7 @@ public void AppendLargeCookie_WithOptions_Appended()
             {
                 Domain = "foo.com",
                 HttpOnly = true,
-                SameSite = SameSiteMode.Strict,
+                SameSite = Http.SameSiteMode.Strict,
                 Path = "/bar",
                 Secure = true,
                 Expires = now.AddMinutes(5),
@@ -145,5 +146,59 @@ public void DeleteChunkedCookieWithOptions_AllDeleted()
                 "TestCookieC7=; expires=Thu, 01 Jan 1970 00:00:00 GMT; domain=foo.com; path=/; secure",
             }, cookies);
         }
+
+
+
+        [Fact]
+        public void DeleteChunkedCookieWithOptionsAndResponseCookies_AllDeleted()
+        {
+            var chunkingCookieManager = new ChunkingCookieManager();
+            HttpContext httpContext = new DefaultHttpContext();
+            
+            httpContext.Request.Headers["Cookie"] = new[]
+            {
+                "TestCookie=chunks-7",
+                "TestCookieC1=abcdefghi",
+                "TestCookieC2=jklmnopqr",
+                "TestCookieC3=stuvwxyz0",
+                "TestCookieC4=123456789",
+                "TestCookieC5=ABCDEFGHI",
+                "TestCookieC6=JKLMNOPQR",
+                "TestCookieC7=STUVWXYZ"
+            };
+
+            var cookieOptions = new CookieOptions()
+            {
+                Domain = "foo.com",
+                Path = "/",
+                Secure = true
+            };
+
+            httpContext.Response.Headers[HeaderNames.SetCookie] = new[]
+            {
+                "TestCookie=chunks-7; domain=foo.com; path=/; secure",
+                "TestCookieC1=STUVWXYZ; domain=foo.com; path=/; secure",
+                "TestCookieC2=123456789; 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"
+            };
+
+            chunkingCookieManager.DeleteCookie(httpContext, "TestCookie", cookieOptions);
+            Assert.Equal(8, httpContext.Response.Headers[HeaderNames.SetCookie].Count);
+            Assert.Equal(new[]
+            {
+                "TestCookie=; expires=Thu, 01 Jan 1970 00:00:00 GMT; domain=foo.com; path=/; secure",
+                "TestCookieC1=; expires=Thu, 01 Jan 1970 00:00:00 GMT; domain=foo.com; path=/; secure",
+                "TestCookieC2=; expires=Thu, 01 Jan 1970 00:00:00 GMT; domain=foo.com; path=/; secure",
+                "TestCookieC3=; expires=Thu, 01 Jan 1970 00:00:00 GMT; domain=foo.com; path=/; secure",
+                "TestCookieC4=; expires=Thu, 01 Jan 1970 00:00:00 GMT; domain=foo.com; path=/; secure",
+                "TestCookieC5=; expires=Thu, 01 Jan 1970 00:00:00 GMT; domain=foo.com; path=/; secure",
+                "TestCookieC6=; expires=Thu, 01 Jan 1970 00:00:00 GMT; domain=foo.com; path=/; secure",
+                "TestCookieC7=; expires=Thu, 01 Jan 1970 00:00:00 GMT; domain=foo.com; path=/; secure"
+            }, httpContext.Response.Headers[HeaderNames.SetCookie]);
+        }
     }
 }
diff --git a/src/Security/perf/Microbenchmarks/ChunkingCookieManagerBenchmark.cs b/src/Security/perf/Microbenchmarks/ChunkingCookieManagerBenchmark.cs
@@ -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",
+                "TestCookieC1=abcdefghi",
+                "TestCookieC2=jklmnopqr",
+                "TestCookieC3=stuvwxyz0",
+                "TestCookieC4=123456789",
+                "TestCookieC5=ABCDEFGHI",
+                "TestCookieC6=JKLMNOPQR",
+                "TestCookieC7=STUVWXYZ"
+            };
+
+            _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;
+        }
+    }
+}
diff --git a/src/Security/perf/Microbenchmarks/Microsoft.AspNetCore.Security.Microbenchmarks.csproj b/src/Security/perf/Microbenchmarks/Microsoft.AspNetCore.Security.Microbenchmarks.csproj
@@ -1,16 +1,17 @@
 <Project Sdk="Microsoft.NET.Sdk">
-
+  
   <PropertyGroup>
     <TargetFramework>$(DefaultNetCoreTargetFramework)</TargetFramework>
     <OutputType>Exe</OutputType>
     <ServerGarbageCollection>true</ServerGarbageCollection>
     <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
     <RootNamespace>Microsoft.AspNetCore.Security</RootNamespace>
   </PropertyGroup>
-
+  
   <ItemGroup>
-    <Reference Include="Microsoft.AspNetCore.Authorization.Policy" />
     <Reference Include="BenchmarkDotNet" />
+    <Reference Include="Microsoft.AspNetCore.Authorization.Policy" />
+    <Reference Include="Microsoft.AspNetCore.Authentication.Cookies" />
     <Reference Include="Microsoft.AspNetCore.Http" />
     <Reference Include="Microsoft.Extensions.DependencyInjection" />
     <Reference Include="Microsoft.Extensions.Logging" />
diff --git a/src/Shared/ChunkingCookieManager/ChunkingCookieManager.cs b/src/Shared/ChunkingCookieManager/ChunkingCookieManager.cs

Original file line number	Diff line number	Diff line change
`@@ -501,4 +501,4 @@ public override byte[] Serialize(IDictionary<string, object> values)`
`501`	`501`	`}`
`502`	`502`	`}`
`503`	`503`	`}`
`504`		`-}`
	`504`	`+}`