Skip to content
This repository has been archived by the owner on Dec 19, 2018. It is now read-only.

Pool DefaultHttpContext [Design] #355

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,6 @@ namespace Microsoft.AspNet.Hosting.Server
public interface IServerFactory
{
IFeatureCollection Initialize(IConfiguration configuration);
IDisposable Start(IFeatureCollection serverFeatures, Func<IFeatureCollection, Task> application);
IDisposable Start(IFeatureCollection serverFeatures, Func<IFeatureCollection, Task<IDisposable>> application);
}
}
42 changes: 41 additions & 1 deletion src/Microsoft.AspNet.Hosting/Builder/HttpContextFactory.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// 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.

using System;
using System.Collections.Concurrent;
using Microsoft.AspNet.Http;
using Microsoft.AspNet.Http.Features;
using Microsoft.AspNet.Http.Internal;
Expand All @@ -9,9 +11,47 @@ namespace Microsoft.AspNet.Hosting.Builder
{
public class HttpContextFactory : IHttpContextFactory
{
private static int _capacity = 32 * Environment.ProcessorCount;
private static readonly ConcurrentQueue<DefaultHttpContext> _contextPool = new ConcurrentQueue<DefaultHttpContext>();
private static readonly ConcurrentQueue<FeatureCollection> _featureCollectionPool = new ConcurrentQueue<FeatureCollection>();

public HttpContext CreateHttpContext(IFeatureCollection featureCollection)
{
return new DefaultHttpContext(new FeatureCollection(featureCollection));
DefaultHttpContext context;
if (_contextPool.TryDequeue(out context))
{
context.Initalize(CreateFeatureCollection(featureCollection));
return context;
}
return new DefaultHttpContext(CreateFeatureCollection(featureCollection), PoolContext);
}

internal FeatureCollection CreateFeatureCollection(IFeatureCollection innerFeatureCollection)
{
FeatureCollection featureCollection;
if (_featureCollectionPool.TryDequeue(out featureCollection))
{
featureCollection.Reset(innerFeatureCollection);
return featureCollection;
}
return new FeatureCollection(innerFeatureCollection, PoolFeatureCollection);
}

internal void PoolContext(DefaultHttpContext context)
{
// Benign race condition
if (_contextPool.Count < _capacity)
{
_contextPool.Enqueue(context);
}
}
internal void PoolFeatureCollection(FeatureCollection context)
{
// Benign race condition
if (_featureCollectionPool.Count < _capacity)
{
_featureCollectionPool.Enqueue(context);
}
}
}
}
3 changes: 3 additions & 0 deletions src/Microsoft.AspNet.Hosting/Internal/HostingEngine.cs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ public virtual IDisposable Start()
async features =>
{
var httpContext = contextFactory.CreateHttpContext(features);

httpContext.ApplicationServices = _applicationServices;
var requestIdentifier = GetRequestIdentifier(httpContext);

Expand All @@ -79,6 +80,8 @@ public virtual IDisposable Start()
contextAccessor.HttpContext = httpContext;
await application(httpContext);
}

return httpContext;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this so httpContext gets disposed? What are we waiting for?

I would just wrap the call to await application(httpContext) in a using statement. Once the Task returned from the application is completed, you should be safe to dispose the httpContext.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@halter73 He tried that first, see #355 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the HttpContext supposed to be usable in OnCompleted? I think the simplest solution is to not support it. It's not like you would be able to change the response at that point.

I'm not a big fan of returning an IDisposable here, because it's not obvious why it's necessary. It could definitely be a gotcha for anyone implementing or using the IServerFactory interface.

I would even prefer having a "OnTearDown" IHttpResponseFeature that runs after OnCompleted, only because the purpose is more clear.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not recommending any specific solution, just trying to outline what's possible. HttpContext is supported in OnCompleted, but more importantly it's supported in OnStarted which can fire after await application(httpContext) in no-body scenarios.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is open question on whether the Context is the place to dispose of the features collection: https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.AspNet.Http/DefaultHttpContext.cs#L172

Currently it is in Dispose; but the dispose never gets called as its not returned, so the features collection is never disposed?

Have raised related issue #359 as these have all been PRs where its slightly tangential.

});

_applicationLifetime.NotifyStarted();
Expand Down
3 changes: 2 additions & 1 deletion src/Microsoft.AspNet.Hosting/project.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@
"dnx451": { },
"dnxcore50": {
"dependencies": {
"System.Console": "4.0.0-beta-*"
"System.Console": "4.0.0-beta-*",
"System.Collections.Concurrent": "4.0.11-beta-*"
}
}
}
Expand Down