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

Commit 1f2d8bb

Browse files
dougbudougbu
committed
PR comments: Switch approaches
- `IHttpContextFactory` instance is a singleton instantiated from CI - make `HttpContextFactory` `ObjectPoolProvider` and `ResponseCookiesFeature`-aware - apply same pattern to sample `PooledHttpContextFactory` - pool is not currently configurable; defaults are fine for response cookies - if we need (policy) configuration, would add an `IOptions<HttpContextFactorySettings>` - `ResponseCookies` works fine if no `ObjectPoolProvider` is available
1 parent 0dcacac commit 1f2d8bb

File tree

5 files changed

+100
-53
lines changed

5 files changed

+100
-53
lines changed

‎samples/SampleApp/PooledHttpContextFactory.cs

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,24 +1,56 @@
11
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

4+
using System;
45
using System.Collections.Generic;
6+
using System.Text;
57
using Microsoft.AspNetCore.Http;
68
using Microsoft.AspNetCore.Http.Features;
9+
using Microsoft.AspNetCore.Http.Features.Internal;
10+
using Microsoft.Extensions.ObjectPool;
711

812
namespace SampleApp
913
{
1014
public class PooledHttpContextFactory : IHttpContextFactory
1115
{
16+
private readonly ObjectPool<StringBuilder> _builderPool;
1217
private readonly IHttpContextAccessor _httpContextAccessor;
1318
private readonly Stack<PooledHttpContext> _pool = new Stack<PooledHttpContext>();
1419

20+
public PooledHttpContextFactory()
21+
: this(poolProvider: null, httpContextAccessor: null)
22+
{
23+
}
24+
25+
public PooledHttpContextFactory(ObjectPoolProvider poolProvider)
26+
: this(poolProvider, httpContextAccessor: null)
27+
{
28+
}
29+
1530
public PooledHttpContextFactory(IHttpContextAccessor httpContextAccessor)
31+
: this(poolProvider: null, httpContextAccessor: httpContextAccessor)
1632
{
33+
}
34+
35+
public PooledHttpContextFactory(ObjectPoolProvider poolProvider, IHttpContextAccessor httpContextAccessor)
36+
{
37+
_builderPool = poolProvider?.CreateStringBuilderPool();
1738
_httpContextAccessor = httpContextAccessor;
1839
}
1940

2041
public HttpContext Create(IFeatureCollection featureCollection)
2142
{
43+
if (featureCollection == null)
44+
{
45+
throw new ArgumentNullException(nameof(featureCollection));
46+
}
47+
48+
if (_builderPool != null)
49+
{
50+
var responseCookiesFeature = new ResponseCookiesFeature(featureCollection, _builderPool);
51+
featureCollection.Set<IResponseCookiesFeature>(responseCookiesFeature);
52+
}
53+
2254
PooledHttpContext httpContext = null;
2355
lock (_pool)
2456
{

‎src/Microsoft.AspNetCore.Http.Features/IResponseCookiesFeature.cs

Lines changed: 0 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -8,25 +8,6 @@ namespace Microsoft.AspNetCore.Http.Features
88
/// </summary>
99
public interface IResponseCookiesFeature
1010
{
11-
/// <summary>
12-
/// Initial capacity of instances used in the implementation of this feature, in <see cref="char"/>s.
13-
/// </summary>
14-
/// <remarks>
15-
/// For example, the initial capacity of <see cref="System.Text.StringBuilder"/> instances obtained
16-
/// from an object pool.
17-
/// </remarks>
18-
int InitialPooledInstanceCapacity { get; set; }
19-
20-
/// <summary>
21-
/// Maximum retained capacity of instances used in the implementation of this feature, in <see cref="char"/>s.
22-
/// Instances larger than this will be deleted rather than preserved in the pool.
23-
/// </summary>
24-
/// <remarks>
25-
/// For example, the maximum retained capacity of <see cref="System.Text.StringBuilder"/> instances obtained
26-
/// from an object pool.
27-
/// </remarks>
28-
int MaximumRetainedPooledInstanceCapacity { get; set; }
29-
3011
/// <summary>
3112
/// Cookies which will be returned with the response.
3213
/// </summary>

‎src/Microsoft.AspNetCore.Http/Features/ResponseCookiesFeature.cs

Lines changed: 23 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

4+
using System;
5+
using System.Text;
46
using Microsoft.AspNetCore.Http.Internal;
57
using Microsoft.Extensions.ObjectPool;
68

@@ -11,8 +13,9 @@ namespace Microsoft.AspNetCore.Http.Features.Internal
1113
/// </summary>
1214
public class ResponseCookiesFeature : IResponseCookiesFeature
1315
{
16+
private readonly ObjectPool<StringBuilder> _builderPool;
17+
1418
private FeatureReferences<IHttpResponseFeature> _features;
15-
private FeatureReferences<IServiceProvidersFeature> _services;
1619
private IResponseCookies _cookiesCollection;
1720

1821
/// <summary>
@@ -23,24 +26,31 @@ public class ResponseCookiesFeature : IResponseCookiesFeature
2326
/// <see cref="IResponseCookiesFeature"/> and the <see cref="IHttpResponseFeature"/>.
2427
/// </param>
2528
public ResponseCookiesFeature(IFeatureCollection features)
29+
: this(features, builderPool: null)
2630
{
27-
_features = new FeatureReferences<IHttpResponseFeature>(features);
28-
_services = new FeatureReferences<IServiceProvidersFeature>(features);
2931
}
3032

31-
/// <inheritdoc />
32-
/// <value>Default is <c>100</c> <see cref="char"/>s.</value>
33-
public int InitialPooledInstanceCapacity { get; set; } = new StringBuilderPooledObjectPolicy().InitialCapacity;
33+
/// <summary>
34+
/// Initializes a new <see cref="ResponseCookiesFeature"/> instance.
35+
/// </summary>
36+
/// <param name="features">
37+
/// <see cref="IFeatureCollection"/> containing all defined features, including this
38+
/// <see cref="IResponseCookiesFeature"/> and the <see cref="IHttpResponseFeature"/>.
39+
/// </param>
40+
/// <param name="builderPool">The <see cref="ObjectPool{T}"/>, if available.</param>
41+
public ResponseCookiesFeature(IFeatureCollection features, ObjectPool<StringBuilder> builderPool)
42+
{
43+
if (features == null)
44+
{
45+
throw new ArgumentNullException(nameof(features));
46+
}
3447

35-
/// <inheritdoc />
36-
/// <value>Default is <c>4048</c> <see cref="char"/>s.</value>
37-
public int MaximumRetainedPooledInstanceCapacity { get; set; } =
38-
new StringBuilderPooledObjectPolicy().MaximumRetainedCapacity;
48+
_features = new FeatureReferences<IHttpResponseFeature>(features);
49+
_builderPool = builderPool;
50+
}
3951

4052
private IHttpResponseFeature HttpResponseFeature => _features.Fetch(ref _features.Cache, f => null);
4153

42-
private IServiceProvidersFeature ServiceProvidersFeature => _services.Fetch(ref _services.Cache, f => null);
43-
4454
/// <inheritdoc />
4555
public IResponseCookies Cookies
4656
{
@@ -49,15 +59,7 @@ public IResponseCookies Cookies
4959
if (_cookiesCollection == null)
5060
{
5161
var headers = HttpResponseFeature.Headers;
52-
53-
var serviceProvider = ServiceProvidersFeature.RequestServices;
54-
var provider = (ObjectPoolProvider)serviceProvider.GetService(typeof(ObjectPoolProvider)) ??
55-
new DefaultObjectPoolProvider();
56-
var pool = provider.CreateStringBuilderPool(
57-
InitialPooledInstanceCapacity,
58-
MaximumRetainedPooledInstanceCapacity);
59-
60-
_cookiesCollection = new ResponseCookies(headers, pool);
62+
_cookiesCollection = new ResponseCookies(headers, _builderPool);
6163
}
6264

6365
return _cookiesCollection;

‎src/Microsoft.AspNetCore.Http/HttpContextFactory.cs

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,30 +1,59 @@
11
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
33

4+
using System;
5+
using System.Text;
46
using Microsoft.AspNetCore.Http.Features;
7+
using Microsoft.AspNetCore.Http.Features.Internal;
8+
using Microsoft.Extensions.ObjectPool;
59

610
namespace Microsoft.AspNetCore.Http.Internal
711
{
812
public class HttpContextFactory : IHttpContextFactory
913
{
10-
private IHttpContextAccessor _httpContextAccessor;
14+
private readonly ObjectPool<StringBuilder> _builderPool;
15+
private readonly IHttpContextAccessor _httpContextAccessor;
1116

12-
public HttpContextFactory() : this(httpContextAccessor: null)
17+
public HttpContextFactory()
18+
: this(poolProvider: null, httpContextAccessor: null)
19+
{
20+
}
21+
22+
public HttpContextFactory(ObjectPoolProvider poolProvider)
23+
: this(poolProvider, httpContextAccessor: null)
1324
{
1425
}
1526

1627
public HttpContextFactory(IHttpContextAccessor httpContextAccessor)
28+
: this(poolProvider: null, httpContextAccessor: httpContextAccessor)
1729
{
30+
}
31+
32+
public HttpContextFactory(ObjectPoolProvider poolProvider, IHttpContextAccessor httpContextAccessor)
33+
{
34+
_builderPool = poolProvider?.CreateStringBuilderPool();
1835
_httpContextAccessor = httpContextAccessor;
1936
}
2037

2138
public HttpContext Create(IFeatureCollection featureCollection)
2239
{
40+
if (featureCollection == null)
41+
{
42+
throw new ArgumentNullException(nameof(featureCollection));
43+
}
44+
45+
if (_builderPool != null)
46+
{
47+
var responseCookiesFeature = new ResponseCookiesFeature(featureCollection, _builderPool);
48+
featureCollection.Set<IResponseCookiesFeature>(responseCookiesFeature);
49+
}
50+
2351
var httpContext = new DefaultHttpContext(featureCollection);
2452
if (_httpContextAccessor != null)
2553
{
2654
_httpContextAccessor.HttpContext = httpContext;
2755
}
56+
2857
return httpContext;
2958
}
3059

‎src/Microsoft.AspNetCore.Http/ResponseCookies.cs

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -21,17 +21,13 @@ public class ResponseCookies : IResponseCookies
2121
/// Create a new wrapper
2222
/// </summary>
2323
/// <param name="headers">The <see cref="IHeaderDictionary"/> for the response.</param>
24-
/// <param name="builderPool">The <see cref="ObjectPool{T}"/> used.</param>
24+
/// <param name="builderPool">The <see cref="ObjectPool{T}"/>, if available.</param>
2525
public ResponseCookies(IHeaderDictionary headers, ObjectPool<StringBuilder> builderPool)
2626
{
2727
if (headers == null)
2828
{
2929
throw new ArgumentNullException(nameof(headers));
3030
}
31-
if (builderPool == null)
32-
{
33-
throw new ArgumentNullException(nameof(builderPool));
34-
}
3531

3632
Headers = headers;
3733
_builderPool = builderPool;
@@ -54,15 +50,22 @@ public void Append(string key, string value)
5450
};
5551

5652
string cookieValue;
57-
var stringBuilder = _builderPool.Get();
58-
try
53+
if (_builderPool == null)
5954
{
60-
setCookieHeaderValue.AppendToStringBuilder(stringBuilder);
61-
cookieValue = stringBuilder.ToString();
55+
cookieValue = setCookieHeaderValue.ToString();
6256
}
63-
finally
57+
else
6458
{
65-
_builderPool.Return(stringBuilder);
59+
var stringBuilder = _builderPool.Get();
60+
try
61+
{
62+
setCookieHeaderValue.AppendToStringBuilder(stringBuilder);
63+
cookieValue = stringBuilder.ToString();
64+
}
65+
finally
66+
{
67+
_builderPool.Return(stringBuilder);
68+
}
6669
}
6770

6871
Headers[HeaderNames.SetCookie] = StringValues.Concat(Headers[HeaderNames.SetCookie], cookieValue);

0 commit comments

Comments
 (0)