fix allocation issue caused by ordering of MVC result executors for Task results #41108
Conversation
ghost
added
the
area-mvc
| new TaskResultExecutor(), | ||
| new AwaitableResultExecutor(), | ||
| new TaskOfIActionResultExecutor(), | ||
| new TaskOfActionResultExecutor(), |
There was a problem hiding this comment.
I think we should make sure every executor on this list is called by a test, for example I don't see an added check for "TaskOfActionResultExecutor", does this mean we are missing test coverage for it? Or is it shadowed by TaskOfIActionResultExecutor?
There was a problem hiding this comment.
Good catch @BrennanConroy , I think TaskOfIActionResultExecutor had a duplicate test. I adjusted it to account for TaskOfActionResultExecutor
src/Mvc/Mvc.Core/test/Infrastructure/ActionMethodExecutorTest.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Brennan <[email protected]>
| var valueTask = actionMethodExecutor.Execute(mapper, objectMethodExecutor, controller, Array.Empty<object>()); | ||
|
|
||
| // Assert | ||
| Assert.Equal("VoidResultExecutor", actionMethodExecutor.GetType().Name); |
There was a problem hiding this comment.
These types are internal so a type check would work? i.e.
Assert.IsType<VoidResultExecutor>(actionMethodExecutor);There was a problem hiding this comment.
@davidfowl All the executors are private classes. The only internal class is the ActionMethodExecutor. I don't think there is enough value in making these classes internal just for testing purposes.
halter73
left a comment
halter73
left a comment
There was a problem hiding this comment.
We could make the private nested classes internal nested classes instead to allow for IsType checks in our tests, but I don't think that's very important.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
4 participants
Fix allocation issue caused by ordering of MVC result executors for Task results
This PR changes the order of the ActionMethod executors for Task result types and adds tests to ensure the right executor gets chosen based on different action result scenarios.
Description
This PR changes the order of the ActionMethod executors so that for the case of
Taskwhere the result type is known at compile time, we can invoke theExecutesync method rather than theExecuteAsyncto allow us to "await" directly the result value via type cast. TheExecuteAyncmethod returns a custom awaitable, which can incur extra head allocations.Fixes #40364