Fix #2294: batch processing loses HttpContext for main request.

[NicholasNoise]

Issues

This pull request fixes issue #2294.

Description

HttpContext context = null;
IHttpContextFactory httpContextFactory = originalContext.RequestServices.GetRequiredService<IHttpContextFactory>();
if (httpContextFactory != null)
{
    context = httpContextFactory.Create(features);
}
else
{
    context = new DefaultHttpContext(features);
}

IHttpContextFactory(DefaultHttpContextFactory) service should not be used to create new HttpContext, because it resets current context to new one:
https://github.com/dotnet/aspnetcore/blob/master/src/Hosting/Hosting/src/Http/DefaultHttpContextFactory.cs#L73

Checklist (Uncheck if it is not completed)

• Test cases added
• Build and test with one-click build and test script passed

[mikepizzo]

BatchTestCustomersController [](start = 48, length = 28)

Why did we get rid of the test that used two controllers? Isn't this still a valid scenario? Don't we need to make sure it works as expected?

[NicholasNoise]

I've added unused controller with #2379, it is just a cleanup.
So yes, it works as expected.

[mikepizzo]

HttpContext context = new DefaultHttpContext(features); [](start = 12, length = 55)

What are the ramifications of not using the context factory? Doesn't that mean that, if someone adds their own context factory, we don't use it for batch? Couldn't that be a problem if the developer is expecting their custom httpcontext to be used? Do we instead need a way to create the context from the factory without resetting the IHttpContextAccessor.HttpContext?

[NicholasNoise]

I do not see any other reason to use the factory for creating HttpContext from a set of sub-requests, but dependency injection. And it is unclear why a global factory (that creates context for an actual http request) should be used to instance batch sub-contexts.

If the factory is used we should make a closure of original context after processing batch changesets like this one:

        /// <inheritdoc />
        public override async Task ProcessBatchAsync(HttpContext context, RequestDelegate nextHandler)
        {
            // Retrieve current httpcontext.
            var httpContextAccessor = context.RequestServices.GetService<IHttpContextAccessor>();
            var beforeContext = httpContextAccessor?.HttpContext;

            await base.ProcessBatchAsync(context, nextHandler);

            if (httpContextAccessor != null)
            {
                httpContextAccessor.HttpContext = beforeContext;
            }
        }

But still a closure is more like a workaround than THE solution.

Keyword new is kinda bad in common, especially in the name of DI, instead we can introduce an interface IBatchHttpContextFactory to create a context of the changeset (silly refactor)

[marabooy]

Hi @mikepizzo & @NicholasNoise, I tested the pr with the two suggestions. I guess my question would be for Sub requests created during batch processing should we give users more control over the creation of the context? Since we do copy the features, headers, and cookie information. One of the issues with the current approach I do see is if the user used their own IHttpContextAccessor and IHttpContextFactory then this process would need them to do more refactors if they needed to change how batch sub requests are created by extending the BatchMiddleware.

[NicholasNoise]

Hi @marabooy
Yeah, we should give more control of the context creation. But as I've pointed out IHttpContextFactory is not a way to do so. I suggest a new abstraction IBatchHttpContextFactory for this matter.

[mikepizzo]

Hi @NicholasNoise -- thanks for the contribution!

As per my inline comments, I'm curious what the ramifications are if we don't use the injected HttpContextFactory. Eager to get your (and others') thoughts.

[NicholasNoise]

@mikepizzo
How do I make this PR unblocked? =)