Don't allocate the FormFeature eagerly

[davidfowl]

• Expose FormOptions on DefaultHttpContext
• Use those options on DefaultHttpContext when the FormFeature is initialized

This is a little hacky but works well for the 98.456% case.

cc @benaadams

[benaadams]

Is this going to be a problem if you seal DefaultHttpContext?

[davidfowl]

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

[benaadams]

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

(Otherwise just rename DefaultHttpContext to HttpContext)

[davidfowl]

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.

[davidfowl]

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
    };
}

[davidfowl]

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

• 688ad19
• a11c68a
• ea344bf

[NinoFloris]

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.

[davidfowl]

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.

[NinoFloris]

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.

[davidfowl]

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.

[davidfowl]

🆙 📅

[Tratcher]

[Edit] nevermind

[davidfowl]

@Tratcher anymore feedback? It's actually much cleaner now.

[Tratcher]

Any measurable perf improvement?

[davidfowl]

Allocates a bit less 😄 . 48% strings now!

Old:
1,925,886

New:
1,930,479