Fix IAsyncEnumerable controller methods to allow setting headers

[BrennanConroy]

Fix IAsyncEnumerable controller methods to allow setting headers

Description

When writing Json responses in MVC, we started flushing headers immediately before using JsonSerializer in order to avoid some unnecessary buffering in Kestrel when writing starts before headers are flushed. This had the unintended side-effect of making IAsyncEnumerable<T> MVC methods no longer able to set headers due to the fact that the whole method becomes the IAsyncEnumerable and we pass that directly to JsonSerializer, which means we've already flushed headers by the time the MVC method starts running. Resulting in an exception on the server and a bad response on the client.

Fixes #57895

Customer Impact

MVC methods that returned IAsyncEnumerable<T> will now fail if they modify response headers.

Regression?

• Yes
• No

Regressed from 8.0

Risk

• High
• Medium
• Low

Small targeted fix. Added test coverage for scenario.

Verification

• Manual (required)
• Automated

Packaging changes reviewed?

• Yes
• No
• N/A

[dotnet-policy-service]

Hi @BrennanConroy. This PR was just approved to be included in the upcoming servicing release. Somebody from the @dotnet/aspnet-build team will get it merged when the branches are open. Until then, please make sure all the CI checks pass and the PR is reviewed.

[halter73]

Should we do something for minimal APIs? I don't think it's as important as MVC because you cannot yield return from a lambda which are typically used in minimal APIs.

CS1621: The yield statement cannot be used inside an anonymous method or lambda expression.

However, you can run into the with a named function which isn't super rare. And at that point, the repro code looks very similar to what you have for an MVC action.

var app = WebApplication.Create(args);
app.MapGet("/", Test);
app.Run();

async IAsyncEnumerable<string> Test(HttpContext context)
{
    var testValue = await myService.GetHeaderValueAsync();
    context.Response.Headers["Test"] = testValue;

    await foreach (var item in myService.GetItemsAsync())
    {
        yield return item;
    }
}

I agree that we probably don't need to update WriteAsJsonAsync considering the code required to run into this issue would be far more complicated and unlikely than code that achieved the same thing without issue. You would need to write something like this:

app.MapGet("/", async (HttpContext context) =>
{
    async IAsyncEnumerable<string> Test()
    {
        var testValue = await myService.GetHeaderValueAsync();
        context.Response.Headers["Test"] = testValue;

        await foreach (var item in myService.GetItemsAsync())
        {
            yield return item;
        }
    }

    await context.Response.WriteAsJsonAsync(Test());
});

Instead of just:

app.MapGet("/", async (HttpContext context) =>
{
    async IAsyncEnumerable<string> Test()
    {
        await foreach (var item in myService.GetItemsAsync())
        {
            yield return item;
        }
    }

    var testValue = await myService.GetHeaderValueAsync();
    context.Response.Headers["Test"] = testValue;

    await context.Response.WriteAsJsonAsync(Test());
});

I suppose with weird enough layering it's possible that someone might try to mutate the response in an IAsyncEnumerable generator, but I think the risk goes significantly down when the generator isn't the MVC action or minimal route handler itself.

[BrennanConroy]

Pretty sure minimal just calls into WriteAsJsonAsync, so looks like we'll want to fix the extension methods as well.

[halter73]

Wouldn't it be better to test this after yielding?

Suggested change

	Assert.False(responseBodyFeature.StartCalled);
	await Task.Yield();
	await Task.Yield();
	Assert.False(responseBodyFeature.StartCalled);

I wonder if we should add an end-to-end test where we try adding a header after yielding.

[halter73]

Can we add a method to AsyncEnumerableHelper like the following?

Task StartResponseIfPossibleAsync(this HttpResponse response, Type? typeToSerialize)

It seems like it would reduce a lot of repetitive HasStarted and null checks.