Skip to content
This repository was archived by the owner on Nov 20, 2018. It is now read-only.

Don't allocate for ResponseCookiesFeature #699

Closed
wants to merge 1 commit into from
Closed

Don't allocate for ResponseCookiesFeature #699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions samples/SampleApp/PooledHttpContextFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ namespace SampleApp
{
public class PooledHttpContextFactory : IHttpContextFactory
{
private readonly ObjectPool<StringBuilder> _builderPool;
private readonly IHttpContextAccessor _httpContextAccessor;
private readonly Stack<PooledHttpContext> _pool = new Stack<PooledHttpContext>();

Expand All @@ -28,7 +27,6 @@ public PooledHttpContextFactory(ObjectPoolProvider poolProvider, IHttpContextAcc
throw new ArgumentNullException(nameof(poolProvider));
}

_builderPool = poolProvider.CreateStringBuilderPool();
_httpContextAccessor = httpContextAccessor;
}

Expand All @@ -39,9 +37,6 @@ public HttpContext Create(IFeatureCollection featureCollection)
throw new ArgumentNullException(nameof(featureCollection));
}

var responseCookiesFeature = new ResponseCookiesFeature(featureCollection, _builderPool);
featureCollection.Set<IResponseCookiesFeature>(responseCookiesFeature);

PooledHttpContext httpContext = null;
lock (_pool)
{
Expand Down
3 changes: 2 additions & 1 deletion src/Microsoft.AspNetCore.Http.Abstractions/project.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@
"type": "build"
},
"NETStandard.Library": "1.6.1-*",
"System.Text.Encodings.Web": "4.3.0-*"
"System.Text.Encodings.Web": "4.3.0-*",
"Microsoft.Extensions.Primitives": "1.1.0-*"
},
"frameworks": {
"net451": {
Expand Down
6 changes: 1 addition & 5 deletions src/Microsoft.AspNetCore.Http/Features/ResponseCookiesFeature.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,6 @@ public class ResponseCookiesFeature : IResponseCookiesFeature
// Lambda hoisted to static readonly field to improve inlining https://github.com/dotnet/roslyn/issues/13624
private readonly static Func<IFeatureCollection, IHttpResponseFeature> _nullResponseFeature = f => null;

// Object pool will be null only in test scenarios e.g. if code news up a DefaultHttpContext.
private readonly ObjectPool<StringBuilder> _builderPool;

private FeatureReferences<IHttpResponseFeature> _features;
private IResponseCookies _cookiesCollection;

Expand Down Expand Up @@ -50,7 +47,6 @@ public ResponseCookiesFeature(IFeatureCollection features, ObjectPool<StringBuil
}

_features = new FeatureReferences<IHttpResponseFeature>(features);
_builderPool = builderPool;
}

private IHttpResponseFeature HttpResponseFeature => _features.Fetch(ref _features.Cache, _nullResponseFeature);
Expand All @@ -63,7 +59,7 @@ public IResponseCookies Cookies
if (_cookiesCollection == null)
{
var headers = HttpResponseFeature.Headers;
_cookiesCollection = new ResponseCookies(headers, _builderPool);
_cookiesCollection = new ResponseCookies(headers, null);
}

return _cookiesCollection;
Expand Down
6 changes: 0 additions & 6 deletions src/Microsoft.AspNetCore.Http/HttpContextFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Text;
using Microsoft.AspNetCore.Http.Features;
using Microsoft.Extensions.ObjectPool;
using Microsoft.Extensions.Options;
Expand All @@ -11,7 +10,6 @@ namespace Microsoft.AspNetCore.Http
{
public class HttpContextFactory : IHttpContextFactory
{
private readonly ObjectPool<StringBuilder> _builderPool;
private readonly IHttpContextAccessor _httpContextAccessor;
private readonly FormOptions _formOptions;

Expand All @@ -31,7 +29,6 @@ public HttpContextFactory(ObjectPoolProvider poolProvider, IOptions<FormOptions>
throw new ArgumentNullException(nameof(formOptions));
}

_builderPool = poolProvider.CreateStringBuilderPool();
_formOptions = formOptions.Value;
_httpContextAccessor = httpContextAccessor;
}
Expand All @@ -43,9 +40,6 @@ public HttpContext Create(IFeatureCollection featureCollection)
throw new ArgumentNullException(nameof(featureCollection));
}

var responseCookiesFeature = new ResponseCookiesFeature(featureCollection, _builderPool);
featureCollection.Set<IResponseCookiesFeature>(responseCookiesFeature);

var httpContext = new DefaultHttpContext(featureCollection);
if (_httpContextAccessor != null)
{
Expand Down
42 changes: 2 additions & 40 deletions src/Microsoft.AspNetCore.Http/Internal/ResponseCookies.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@ namespace Microsoft.AspNetCore.Http.Internal
/// </summary>
public class ResponseCookies : IResponseCookies
{
private readonly ObjectPool<StringBuilder> _builderPool;

/// <summary>
/// Create a new wrapper.
/// </summary>
Expand All @@ -30,7 +28,6 @@ public ResponseCookies(IHeaderDictionary headers, ObjectPool<StringBuilder> buil
}

Headers = headers;
_builderPool = builderPool;
}

private IHeaderDictionary Headers { get; set; }
Expand All @@ -44,25 +41,7 @@ public void Append(string key, string value)
{
Path = "/"
};

string cookieValue;
if (_builderPool == null)
{
cookieValue = setCookieHeaderValue.ToString();
}
else
{
var stringBuilder = _builderPool.Get();
try
{
setCookieHeaderValue.AppendToStringBuilder(stringBuilder);
cookieValue = stringBuilder.ToString();
}
finally
{
_builderPool.Return(stringBuilder);
}
}
var cookieValue = setCookieHeaderValue.ToString();

Headers[HeaderNames.SetCookie] = StringValues.Concat(Headers[HeaderNames.SetCookie], cookieValue);
}
Expand All @@ -86,24 +65,7 @@ public void Append(string key, string value, CookieOptions options)
HttpOnly = options.HttpOnly,
};

string cookieValue;
if (_builderPool == null)
{
cookieValue = setCookieHeaderValue.ToString();
}
else
{
var stringBuilder = _builderPool.Get();
try
{
setCookieHeaderValue.AppendToStringBuilder(stringBuilder);
cookieValue = stringBuilder.ToString();
}
finally
{
_builderPool.Return(stringBuilder);
}
}
var cookieValue = setCookieHeaderValue.ToString();

Headers[HeaderNames.SetCookie] = StringValues.Concat(Headers[HeaderNames.SetCookie], cookieValue);
}
Expand Down
100 changes: 97 additions & 3 deletions src/Microsoft.Net.Http.Headers/SetCookieHeaderValue.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Collections.Generic;
using System.Diagnostics.Contracts;
using System.Text;
using Microsoft.Extensions.Primitives;

namespace Microsoft.Net.Http.Headers
{
Expand Down Expand Up @@ -89,10 +90,103 @@ public string Value
// name="val ue"; expires=Sun, 06 Nov 1994 08:49:37 GMT; max-age=86400; domain=domain1; path=path1; secure; httponly
public override string ToString()
{
StringBuilder header = new StringBuilder();
AppendToStringBuilder(header);
const string equals = "=";
const string separator = "; ";

return header.ToString();
var length = _name.Length + equals.Length + _value.Length;

string expires = null;
string maxAge = null;

if (Expires.HasValue)
{
expires = HeaderUtilities.FormatDate(Expires.Value);
length += separator.Length + ExpiresToken.Length + equals.Length + expires.Length;
}

if (MaxAge.HasValue)
{
maxAge = HeaderUtilities.FormatInt64((long)MaxAge.Value.TotalSeconds);
length += separator.Length + MaxAgeToken.Length + equals.Length + maxAge.Length;
}

if (Domain != null)
{
length += separator.Length + DomainToken.Length + equals.Length + Domain.Length;
}

if (Path != null)
{
length += separator.Length + PathToken.Length + equals.Length + Path.Length;
}

if (Secure)
{
length += separator.Length + SecureToken.Length;
}

if (HttpOnly)
{
length += separator.Length + HttpOnlyToken.Length;
}

var sb = new InplaceStringBuilder(length);

sb.Append(_name);
sb.Append(equals);
sb.Append(_value);

if (expires != null)
{
sb.Append(separator);

sb.Append(ExpiresToken);
sb.Append(equals);
sb.Append(expires);
}

if (maxAge != null)
{
sb.Append(separator);

sb.Append(MaxAgeToken);
sb.Append(equals);
sb.Append(maxAge);
}

if (Domain != null)
{
sb.Append(separator);

sb.Append(DomainToken);
sb.Append(equals);
sb.Append(Domain);
}

if (Path != null)
{
sb.Append(separator);

sb.Append(PathToken);
sb.Append(equals);
sb.Append(Path);
}

if (Secure)
{
sb.Append(separator);

sb.Append(SecureToken);
}

if (HttpOnly)
{
sb.Append(separator);

sb.Append(HttpOnlyToken);
}

return sb.ToString();
}

/// <summary>
Expand Down
48 changes: 15 additions & 33 deletions test/Microsoft.AspNetCore.Http.Tests/ResponseCookiesTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,27 +11,11 @@ namespace Microsoft.AspNetCore.Http.Tests
{
public class ResponseCookiesTest
{
private static readonly ObjectPool<StringBuilder> _builderPool =
new DefaultObjectPoolProvider().Create<StringBuilder>(new StringBuilderPooledObjectPolicy());

public static TheoryData BuilderPoolData
{
get
{
return new TheoryData<ObjectPool<StringBuilder>>
{
null,
_builderPool,
};
}
}

[Theory]
[MemberData(nameof(BuilderPoolData))]
public void DeleteCookieShouldSetDefaultPath(ObjectPool<StringBuilder> builderPool)
[Fact]
public void DeleteCookieShouldSetDefaultPath()
{
var headers = new HeaderDictionary();
var cookies = new ResponseCookies(headers, builderPool);
var cookies = new ResponseCookies(headers, null);
var testcookie = "TestCookie";

cookies.Delete(testcookie);
Expand All @@ -43,12 +27,11 @@ public void DeleteCookieShouldSetDefaultPath(ObjectPool<StringBuilder> builderPo
Assert.Contains("expires=Thu, 01 Jan 1970 00:00:00 GMT", cookieHeaderValues[0]);
}

[Theory]
[MemberData(nameof(BuilderPoolData))]
public void NoParamsDeleteRemovesCookieCreatedByAdd(ObjectPool<StringBuilder> builderPool)
[Fact]
public void NoParamsDeleteRemovesCookieCreatedByAdd()
{
var headers = new HeaderDictionary();
var cookies = new ResponseCookies(headers, builderPool);
var cookies = new ResponseCookies(headers, null);
var testcookie = "TestCookie";

cookies.Append(testcookie, testcookie);
Expand All @@ -66,15 +49,15 @@ public static TheoryData EscapesKeyValuesBeforeSettingCookieData
get
{
// key, value, object pool, expected
return new TheoryData<string, string, ObjectPool<StringBuilder>, string>
return new TheoryData<string, string, string>
{
{ "key", "value", null, "key=value" },
{ "key,", "!value", null, "key%2C=%21value" },
{ "ke#y,", "val^ue", null, "ke%23y%2C=val%5Eue" },
{ "key", "value", _builderPool, "key=value" },
{ "key,", "!value", _builderPool, "key%2C=%21value" },
{ "ke#y,", "val^ue", _builderPool, "ke%23y%2C=val%5Eue" },
{ "base64", "QUI+REU/Rw==", _builderPool, "base64=QUI%2BREU%2FRw%3D%3D" },
{ "key", "value", "key=value" },
{ "key,", "!value", "key%2C=%21value" },
{ "ke#y,", "val^ue", "ke%23y%2C=val%5Eue" },
{ "key", "value", "key=value" },
{ "key,", "!value", "key%2C=%21value" },
{ "ke#y,", "val^ue", "ke%23y%2C=val%5Eue" },
{ "base64", "QUI+REU/Rw==", "base64=QUI%2BREU%2FRw%3D%3D" },
};
}
}
Expand All @@ -84,11 +67,10 @@ public static TheoryData EscapesKeyValuesBeforeSettingCookieData
public void EscapesKeyValuesBeforeSettingCookie(
string key,
string value,
ObjectPool<StringBuilder> builderPool,
string expected)
{
var headers = new HeaderDictionary();
var cookies = new ResponseCookies(headers, builderPool);
var cookies = new ResponseCookies(headers, null);

cookies.Append(key, value);

Expand Down