Skip to content

Don't allocate the FormFeature eagerly #6511

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jan 9, 2019
Merged

Don't allocate the FormFeature eagerly #6511

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a2574fd
Don't allocate the FormFeature eagerly
davidfowl Jan 9, 2019
7df5d60
Made the default FormOptions internal since they are mutable
davidfowl Jan 9, 2019
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
2 changes: 2 additions & 0 deletions src/Http/Http/src/DefaultHttpContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ public void Uninitialize()
_websockets?.Uninitialize();
}

public FormOptions FormOptions { get; set; }

private IItemsFeature ItemsFeature =>
_features.Fetch(ref _features.Cache.Items, _newItemsFeature);

Expand Down
4 changes: 1 addition & 3 deletions src/Http/Http/src/Features/FormFeature.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.Features
{
public class FormFeature : IFormFeature
{
private static readonly FormOptions DefaultFormOptions = new FormOptions();

private readonly HttpRequest _request;
private readonly FormOptions _options;
private Task<IFormCollection> _parsedFormTask;
Expand All @@ -32,7 +30,7 @@ public FormFeature(IFormCollection form)
Form = form;
}
public FormFeature(HttpRequest request)
: this(request, DefaultFormOptions)
: this(request, FormOptions.Default)
{
}

Expand Down
4 changes: 3 additions & 1 deletion src/Http/Http/src/Features/FormOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// 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.IO;
Expand All @@ -8,6 +8,8 @@ namespace Microsoft.AspNetCore.Http.Features
{
public class FormOptions
{
internal static readonly FormOptions Default = new FormOptions();

public const int DefaultMemoryBufferThreshold = 1024 * 64;
public const int DefaultBufferBodyLengthLimit = 1024 * 1024 * 128;
public const int DefaultMultipartBoundaryLengthLimit = 128;
Expand Down
5 changes: 2 additions & 3 deletions src/Http/Http/src/HttpContextFactory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,12 @@ public HttpContext Create(IFeatureCollection featureCollection)
_httpContextAccessor.HttpContext = httpContext;
}

var formFeature = new FormFeature(httpContext.Request, _formOptions);
featureCollection.Set<IFormFeature>(formFeature);
httpContext.FormOptions = _formOptions;

return httpContext;
}

private static HttpContext CreateHttpContext(IFeatureCollection featureCollection)
private static DefaultHttpContext CreateHttpContext(IFeatureCollection featureCollection)
{
if (featureCollection is IHttpContextContainer container)
{
Expand Down
2 changes: 1 addition & 1 deletion src/Http/Http/src/IHttpContextContainer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,6 @@ namespace Microsoft.AspNetCore.Http
{
public interface IHttpContextContainer
{
HttpContext HttpContext { get; }
DefaultHttpContext HttpContext { get; }
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 going to be a problem if you seal DefaultHttpContext?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ehhhh, maybe? For that 1% that want to make a derived version. This was to avoid the casting in the default HttpContextFactory

Copy link
Member

@benaadams benaadams Jan 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, so HttpContext can still be overriden and flow, it isn't bonded to DefaultHttpContext?

(Otherwise just rename DefaultHttpContext to HttpContext)

Copy link
Member Author

@davidfowl davidfowl Jan 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The scenario here is you want to implement a server with a custom lifetime management of the HttpContext that isn't a DefaultHttpContext. This change ties you to DefaultHttpContext. The alternative would be to cast detect the cast.

The another approach would be to only assign this field once since it can't change per request anyways (that would likely mitigate the cost of the downcast) but there's no way to know if this is the first time.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is what it would look like.

private HttpContext CreateHttpContext(IFeatureCollection featureCollection)
{
    if (featureCollection is IHttpContextContainer container)
    {
        var context = container.HttpContext;
        if (context is DefaultHttpContext defaultHttpContext)
        {
            defaultHttpContext.FormOptions = _formOptions;
        }
        else
        {
            var formFeature = new FormFeature(context.Request, _formOptions);
            featureCollection.Set<IFormFeature>(formFeature);
        }
    }

    return new DefaultHttpContext(featureCollection)
    {
        FormOptions = _formOptions
    };
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm asking whether creating a factory is enough to get full equivalence

I don't even know what you're asking for? equivalence of what?

why is HttpProtocol specifically implementing IDefaultHttpContextContainer

Copy link

Choose a reason for hiding this comment

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

I don't even know what you're asking for? equivalence of what?

My ask is simple: What if I want equivalent reusability semantics to DefaultHttpContext, but have reasons to create my own HttpContext fields.

Now DefaultHttpContext is sealed, what to do?

If it's possible — without having to create my own Kestrel protocol too — then there's full equivalence between DefaultHttpContext and my own HttpContext.
If it's not possible then there's only equivalence that comes at a performance penalty.

I think that's an important aspect to highlight with respect to these changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

My ask is simple: What if I want equivalent reusability semantics to DefaultHttpContext, but have reasons to create my own HttpContext fields.
Now DefaultHttpContext is sealed, what to do?

I think it's fine to then file a bug and request it be unsealed. Until then it's sealed because nobody has done that (or not enough that I think it matters). When the masses complain about this change then we'll have some data we can use to unseal the type. Thus far my theory is that 0.01% of people do this so it won't affect anymore in real life.

I think that's an important aspect to highlight with respect to these changes.

The change was marked as breaking and the relevant parties know and hopefully, people try out the previews and give feedback with concrete scenarios, not hypothetical ones.

Copy link

Choose a reason for hiding this comment

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

But wasn't there a straightforward fix for HttpContextContainer to support HttpContext instead?

All I'm saying, if it's low effort/trade-off to keep the full equivalence then that would be my preferred option.

Copy link
Member Author

@davidfowl davidfowl Feb 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

But wasn't there a straightforward fix for HttpContextContainer to support HttpContext instead?
All I'm saying, if it's low effort/trade-off to keep the full equivalence then that would be my preferred option.

More complicated code that i'm not willing to write because of the narrow-ness of the scenario. This is a new extensibility point introduced in 3.0 that only one of our servers supports. If you have a concrete scenario, file an issue with a concrete use case and we'll evaluate it.

}
}
2 changes: 1 addition & 1 deletion src/Http/Http/src/Internal/DefaultHttpRequest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ public sealed class DefaultHttpRequest : HttpRequest
// Lambdas hoisted to static readonly fields to improve inlining https://github.com/dotnet/roslyn/issues/13624
private readonly static Func<IFeatureCollection, IHttpRequestFeature> _nullRequestFeature = f => null;
private readonly static Func<IFeatureCollection, IQueryFeature> _newQueryFeature = f => new QueryFeature(f);
private readonly static Func<HttpRequest, IFormFeature> _newFormFeature = r => new FormFeature(r);
private readonly static Func<DefaultHttpRequest, IFormFeature> _newFormFeature = r => new FormFeature(r, r._context.FormOptions ?? FormOptions.Default);
private readonly static Func<IFeatureCollection, IRequestCookiesFeature> _newRequestCookiesFeature = f => new RequestCookiesFeature(f);
private readonly static Func<IFeatureCollection, IRouteValuesFeature> _newRouteValuesFeature = f => new RouteValuesFeature();
private readonly static Func<HttpContext, IRequestBodyPipeFeature> _newRequestBodyPipeFeature = context => new RequestBodyPipeFeature(context);
Expand Down
22 changes: 20 additions & 2 deletions src/Http/Http/test/Features/FormFeatureTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,24 @@ public async Task ReadFormAsync_0ContentLength_ReturnsEmptyForm()
Assert.Same(FormCollection.Empty, formCollection);
}

[Fact]
public async Task FormFeatureReadsOptionsFromDefaultHttpContext()
{
var context = new DefaultHttpContext();
context.Request.ContentType = "application/x-www-form-urlencoded; charset=utf-8";
context.FormOptions = new FormOptions
{
ValueCountLimit = 1
};

var formContent = Encoding.UTF8.GetBytes("foo=bar&baz=2");
context.Request.Body = new NonSeekableReadStream(formContent);

var exception = await Assert.ThrowsAsync<InvalidDataException>(() => context.Request.ReadFormAsync());

Assert.Equal("Form value count limit 1 exceeded.", exception.Message);
}

[Theory]
[InlineData(true)]
[InlineData(false)]
Expand Down Expand Up @@ -391,7 +409,7 @@ public async Task ReadFormAsync_ValueCountLimitExceeded_Throw(bool bufferRequest
IFormFeature formFeature = new FormFeature(context.Request, new FormOptions() { BufferBody = bufferRequest, ValueCountLimit = 2 });
context.Features.Set<IFormFeature>(formFeature);

var exception = await Assert.ThrowsAsync<InvalidDataException> (() => context.Request.ReadFormAsync());
var exception = await Assert.ThrowsAsync<InvalidDataException>(() => context.Request.ReadFormAsync());
Assert.Equal("Form value count limit 2 exceeded.", exception.Message);
}

Expand All @@ -416,7 +434,7 @@ public async Task ReadFormAsync_ValueCountLimitExceededWithFiles_Throw(bool buff
IFormFeature formFeature = new FormFeature(context.Request, new FormOptions() { BufferBody = bufferRequest, ValueCountLimit = 2 });
context.Features.Set<IFormFeature>(formFeature);

var exception = await Assert.ThrowsAsync<InvalidDataException> (() => context.Request.ReadFormAsync());
var exception = await Assert.ThrowsAsync<InvalidDataException>(() => context.Request.ReadFormAsync());
Assert.Equal("Form value count limit 2 exceeded.", exception.Message);
}

Expand Down
2 changes: 1 addition & 1 deletion src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ public CancellationToken RequestAborted

protected HttpResponseHeaders HttpResponseHeaders { get; } = new HttpResponseHeaders();

HttpContext IHttpContextContainer.HttpContext
DefaultHttpContext IHttpContextContainer.HttpContext
{
get
{
Expand Down