[Minimal APIs] async handlers with single `HttpContext` parameter are not working

[gurustron]

Is there an existing issue for this?

• [X] I have searched the existing issues

Describe the bug

It seems that issue described in https://github.com/dotnet/aspnetcore/issues/39956 was not fixed but even got worse - Minimal APIs handler with single param of type HttpContext results in empty response.

app.MapPost("testpost_works", async (HttpContext context, CancellationToken ct) => await Task.FromResult(context.Request.Method + " response"));
app.MapPost("testpost_fails", Handler);
app.MapGet("testget_works", async (HttpContext context, CancellationToken ct) => await Task.FromResult(context.Request.Method + " response"));
app.MapGet("testget_fails1", async (HttpContext context) => await Task.FromResult(context.Request.Method + " response"));
app.MapGet("testget_fails2", Handler);

async Task<string> Handler(HttpContext context) => await Task.FromResult(context.Request.Method + "response");

Endpoints suffixed fails return empty result while others return "VERB response"

In addition only "works" endpoints are recognized by swagger:

Expected Behavior

All endpoints return "VERB response"

Steps To Reproduce

Use Minimal APIs defined above.

Exceptions (if any)

No response

.NET Version

7.0.100

Anything else?

dotnet --info .NET SDK: Version: 7.0.100 Commit: e12b7af219

Runtime Environment: OS Name: Windows OS Version: 10.0.22000 OS Platform: Windows RID: win10-x64 Base Path: C:\Program Files\dotnet\sdk\7.0.100\

[captainsafia]

Triage: This behavior is a result of the change introduced in this PR.

We introduced an analyzer for this in .NET 8 at https://github.com/dotnet/aspnetcore/pull/44048 that we should consider backporting to .NET 7. @JamesNK Thoughts on backporting this?

The PR above backports a runtime fix that was addressed in https://github.com/dotnet/aspnetcore/issues/39956.

Note: this runtime behavior also exists in .NET 6.

[captainsafia]

Addendum: at least for the OpenAPI-related aspects we are tracking this via https://github.com/dotnet/aspnetcore/issues/44970.

[captainsafia]

@JamesNK Thoughts on a partial revert of https://github.com/dotnet/aspnetcore/pull/42519/files#diff-f8807c470bcc3a077fb176668a46df57b4bb99c992b6b7b375665f8bf3903c94L202-L207?

cc: @eerhardt

[eerhardt]

My understanding of https://github.com/dotnet/aspnetcore/pull/42519/files#diff-f8807c470bcc3a077fb176668a46df57b4bb99c992b6b7b375665f8bf3903c94L202-L207 is that we were doing "trim unsafe" behavior before that change - specifically we would JSON serialize the T value of Task<T>. That's not a warning we can suppress because if the app didn't enable JSON source generation, trimming will change the behavior here.

So in this case, the options I see are:

1. Marking the API as RequiresUnreferencedCode
2. Removing the behavior (what #42519 did)
3. Adding an analyzer that warns specifically for this situation saying that it is not trim safe, and then suppressing the warning in our code.
   • Or resolving the JSON serialization trimming issue in ASP.NET all up, and then suppressing the warning in our code.

[JamesNK]

@JamesNK Thoughts on a partial revert of #42519 (files)?

cc: @eerhardt

I'm against reverting the change. These low-level methods are used by app frameworks, e.g. gRPC, and app frameworks should have a simple way to add endpoints that doesn't create warnings.

I don't have a problem with backporting the analyzer https://github.com/dotnet/aspnetcore/pull/44048

[g-jozsef]

A workaround (at least for .net 7.0.100) is to force the type to be a simple Delegate instead of a RequestDelegate (for functions with a single HttpContext param)

public async Task<....> TestFunction(HttpContext http)

Delegate d = TestFunction;
builder.MapGet("/some-endpoint", d)

Response body is populated correctly with all headers, and it even shows up in the generated openapi docs

[gurustron]

@g-jozsef yes it is either casting to "correct" type or adding a parameter, for example CancelationToken.

[LeonSpors]

@g-jozsef yes it is either casting to "correct" type or adding a parameter, for example CancelationToken.

Thank you for the workaround! I've tested it, and it works perfectly. I appreciate your help. Do you believe it's advisable to incorporate a cancellation token in every asynchronous request across all scenarios?

@captainsafia Any updates on when we can expect a fix for this issue?

[captainsafia]

@g-jozsef yes it is either casting to "correct" type or adding a parameter, for example CancelationToken.

Thank you for the workaround! I've tested it, and it works perfectly. I appreciate your help. Do you believe it's advisable to incorporate a cancellation token in every asynchronous request across all scenarios?

@captainsafia Any updates on when we can expect a fix for this issue?

At the moment, I don't believe that we would do anything to change the overload behavior here. This issue occurs because the compiler favors the RequestDelegate overload of the MapX methods when the provided handler takes a single HttpContext and returns a Task. Adding a cast or an additional parameter changes the signature of the delegate and the compiler will pick the MapX(Delegate handler) overload in those scenarios. I think requiring an explicit cast is sufficient in those scenarios.

[ghord]

Woudn't it be better to partially revert Map as suggested by @captainsafia and introduce new method such as MapTrimSafe for gRPC and other app frameworks? Frameworks can move to more verbose name of the method without impacting users. It is easy to trip over this for beginners...