Get EmptyHttpResult in RDF via reflection #45878

captainsafia · 2023-01-04T19:17:38Z

Addresses #45063

captainsafia · 2023-01-06T02:17:15Z

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

+    // Due to https://github.com/dotnet/aspnetcore/issues/41330 we cannot reference the EmptyHttpResult type
+    // but users still need to assert on it as in https://github.com/dotnet/aspnetcore/issues/45063
+    // so we temporarily work around this here by using reflection to get the actual type.
+    private static readonly NewExpression EmptyHttpResultValueTaskExpr = Type.GetType("Microsoft.AspNetCore.Http.Results, Microsoft.AspNetCore.Http.Results") is {} resultsType


This conditional primarily exists to support scenarios where RDF is instantiated in scenarios where the Results assembly isn't referenced (SignalR tests). This shouldn't impact scenarios where users are running this from the framework.

Instead of using Expression.New(typeof(ValueTask<object>)) which I think would result in a null being serialized to the response, can we make this throw lazily if we ever actually try building an expression tree with this and we cannot find the resultsType? Or maybe just continue using the private version of EmptyHttpResult?

I know this should only affect tests, but it's probably better to either make it throw or make it work to make debugging our tests easier in the future. I think throwing is probably the best option. We can add dependencies to any tests that need to use this NewExpression which will make them more real-world anyway.

brunolins16 · 2023-01-06T19:46:40Z

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

@@ -2154,32 +2159,34 @@ static async Task ExecuteAwaited(ValueTask task)

    private static ValueTask<object?> ExecuteTaskWithEmptyResult(Task task)
    {
+        Debug.Assert(EmptyHttpResultInstance is not null);


Is not possible to it be null in the SignalR scenario? Or, maybe the SignalR scenario is edge case that should never happen in real world?

I assume this would be null in our SignalR tests currently since they don't reference the Results assembly, but I don't think the signalr tests are hitting this code path.

That said, we run our tests in release on the CI, so it might be better to reference a property that's basically just => EmptyHttpResultInstance ?? throw new UnreachableException("The EmptyHttpResult type could not be found."). instead of using EmptyHttpResultInstance directly.

Yep, the problem in the SignalR tests was the resolution that was happening for the assembly in the static properties. I cleaned this up to account for Stephen's feedback and reused the same EmptyHttpResult.Instance variable when constructing the property.

halter73 · 2023-01-06T20:09:11Z

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

+    // Due to https://github.com/dotnet/aspnetcore/issues/41330 we cannot reference the EmptyHttpResult type
+    // but users still need to assert on it as in https://github.com/dotnet/aspnetcore/issues/45063
+    // so we temporarily work around this here by using reflection to get the actual type.
+    private static readonly NewExpression EmptyHttpResultValueTaskExpr = Type.GetType("Microsoft.AspNetCore.Http.Results, Microsoft.AspNetCore.Http.Results") is {} resultsType


Instead of using Expression.New(typeof(ValueTask<object>)) which I think would result in a null being serialized to the response, can we make this throw lazily if we ever actually try building an expression tree with this and we cannot find the resultsType? Or maybe just continue using the private version of EmptyHttpResult?

I know this should only affect tests, but it's probably better to either make it throw or make it work to make debugging our tests easier in the future. I think throwing is probably the best option. We can add dependencies to any tests that need to use this NewExpression which will make them more real-world anyway.

halter73 · 2023-01-06T20:10:47Z

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

@@ -2154,32 +2159,34 @@ static async Task ExecuteAwaited(ValueTask task)

    private static ValueTask<object?> ExecuteTaskWithEmptyResult(Task task)
    {
+        Debug.Assert(EmptyHttpResultInstance is not null);


I assume this would be null in our SignalR tests currently since they don't reference the Results assembly, but I don't think the signalr tests are hitting this code path.

That said, we run our tests in release on the CI, so it might be better to reference a property that's basically just => EmptyHttpResultInstance ?? throw new UnreachableException("The EmptyHttpResult type could not be found."). instead of using EmptyHttpResultInstance directly.

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

halter73

I would still prefer if this threw in release builds if EmptyHttpResultInstance is null.

captainsafia · 2023-01-11T21:39:42Z

I would still prefer if this threw in release builds if EmptyHttpResultInstance is null.

Ah, I misunderstood what you're previous feedback was. I thought you meant throw exceptions in all builds (which would impact our local test scenario). I can add an if-def to throw on RELEASE builds.

captainsafia · 2023-01-12T02:14:43Z

/backport to release/7.0

github-actions · 2023-01-12T02:15:00Z

Started backporting to release/7.0: https://github.com/dotnet/aspnetcore/actions/runs/3898500659

github-actions · 2023-01-12T02:15:34Z

@captainsafia backporting to release/7.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Get EmptyHttpResult in RDF via reflection
Using index info to reconstruct a base tree...
M	src/Http/Http.Extensions/src/RequestDelegateFactory.cs
M	src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs
CONFLICT (content): Merge conflict in src/Http/Http.Extensions/test/RequestDelegateFactoryTests.cs
Auto-merging src/Http/Http.Extensions/src/RequestDelegateFactory.cs
CONFLICT (content): Merge conflict in src/Http/Http.Extensions/src/RequestDelegateFactory.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Get EmptyHttpResult in RDF via reflection
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2023-01-12T02:15:35Z

@captainsafia an error occurred while backporting to release/7.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

* Get EmptyHttpResult in RDF via reflection * Always use instance property for checks * Favor Debug.Assert instead of early exception * Throw exception in RELEASE builds * Fix ifdef

* Get EmptyHttpResult in RDF via reflection (#45878) * Get EmptyHttpResult in RDF via reflection * Always use instance property for checks * Favor Debug.Assert instead of early exception * Throw exception in RELEASE builds * Fix ifdef * Add HttpResults using

ghost added the area-runtime label Jan 4, 2023

build-analysis bot mentioned this pull request Jan 4, 2023

Request failed "500 Internal Server Error" dotnet/arcade#11737

Open

5 tasks

Get EmptyHttpResult in RDF via reflection

9519064

captainsafia force-pushed the safia/empty-result branch from 01e1a9c to 9519064 Compare January 6, 2023 00:04

captainsafia marked this pull request as ready for review January 6, 2023 02:16

captainsafia requested review from Tratcher, BrennanConroy and halter73 as code owners January 6, 2023 02:16

captainsafia requested a review from a team January 6, 2023 02:16

captainsafia commented Jan 6, 2023

View reviewed changes

brunolins16 reviewed Jan 6, 2023

View reviewed changes

halter73 reviewed Jan 6, 2023

View reviewed changes

captainsafia added 2 commits January 10, 2023 01:00

Always use instance property for checks

6e82110

Favor Debug.Assert instead of early exception

7ddb934

davidfowl mentioned this pull request Jan 11, 2023

[AOT] Add expression free request filter pipeline for RequestDelegate #46020

Merged

halter73 approved these changes Jan 11, 2023

View reviewed changes

captainsafia added 2 commits January 11, 2023 22:06

Throw exception in RELEASE builds

442a0d9

Fix ifdef

58b8757

captainsafia merged commit fdd35b0 into main Jan 12, 2023

captainsafia deleted the safia/empty-result branch January 12, 2023 02:14

ghost added this to the 8.0-preview1 milestone Jan 12, 2023

captainsafia mentioned this pull request Jan 12, 2023

Get EmptyHttpResult in RDF via reflection (#45878) #46048

Merged

10 tasks

amcasey added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Aug 25, 2023

amcasey removed the area-runtime label Aug 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get EmptyHttpResult in RDF via reflection #45878

Get EmptyHttpResult in RDF via reflection #45878

captainsafia commented Jan 4, 2023

captainsafia Jan 6, 2023

halter73 Jan 6, 2023

brunolins16 Jan 6, 2023 •

edited

Loading

halter73 Jan 6, 2023

captainsafia Jan 10, 2023

halter73 Jan 6, 2023

halter73 Jan 6, 2023

halter73 left a comment

captainsafia commented Jan 11, 2023

captainsafia commented Jan 12, 2023

github-actions bot commented Jan 12, 2023

github-actions bot commented Jan 12, 2023

github-actions bot commented Jan 12, 2023

Get EmptyHttpResult in RDF via reflection #45878

Get EmptyHttpResult in RDF via reflection #45878

Conversation

captainsafia commented Jan 4, 2023

captainsafia Jan 6, 2023

Choose a reason for hiding this comment

halter73 Jan 6, 2023

Choose a reason for hiding this comment

brunolins16 Jan 6, 2023 • edited Loading

Choose a reason for hiding this comment

halter73 Jan 6, 2023

Choose a reason for hiding this comment

captainsafia Jan 10, 2023

Choose a reason for hiding this comment

halter73 Jan 6, 2023

Choose a reason for hiding this comment

halter73 Jan 6, 2023

Choose a reason for hiding this comment

halter73 left a comment

Choose a reason for hiding this comment

captainsafia commented Jan 11, 2023

captainsafia commented Jan 12, 2023

github-actions bot commented Jan 12, 2023

github-actions bot commented Jan 12, 2023

github-actions bot commented Jan 12, 2023

brunolins16 Jan 6, 2023 •

edited

Loading