Skip to content

fix allocation issue caused by ordering of MVC result executors for Task results #41108

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
May 20, 2022
Merged

fix allocation issue caused by ordering of MVC result executors for Task results #41108

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6efbb19
fix allocation issue caused by ordering of mvc result executors
rafikiassumani-msft Apr 8, 2022
f4f204e
fix spacing issues
rafikiassumani-msft Apr 8, 2022
d3fde25
fix spacing issues
rafikiassumani-msft Apr 8, 2022
773c2b5
add test for TaskOfActionResultExecutor
rafikiassumani-msft Apr 8, 2022
5e37415
Update src/Mvc/Mvc.Core/test/Infrastructure/ActionMethodExecutorTest.cs
rafikiassumani-msft Apr 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Mvc/Mvc.Core/src/Infrastructure/ActionMethodExecutor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ internal abstract class ActionMethodExecutor
new SyncObjectResultExecutor(),

// Executors for async methods
new AwaitableResultExecutor(),
new TaskResultExecutor(),
new AwaitableResultExecutor(),
new TaskOfIActionResultExecutor(),
new TaskOfActionResultExecutor(),
Copy link
Member

@BrennanConroy BrennanConroy Apr 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@rafikiassumani-msft rafikiassumani-msft Apr 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch @BrennanConroy , I think TaskOfIActionResultExecutor had a duplicate test. I adjusted it to account for TaskOfActionResultExecutor

new AwaitableObjectResultExecutor(),
Expand Down
54 changes: 51 additions & 3 deletions src/Mvc/Mvc.Core/test/Infrastructure/ActionMethodExecutorTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
// Licensed to the .NET Foundation under one or more agreements.
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Reflection;
using Microsoft.Extensions.Internal;
using System.Runtime.CompilerServices;

namespace Microsoft.AspNetCore.Mvc.Infrastructure;

Expand All @@ -21,6 +22,7 @@ public void ActionMethodExecutor_ExecutesVoidActions()
var valueTask = actionMethodExecutor.Execute(mapper, objectMethodExecutor, controller, Array.Empty<object>());

// Assert
Assert.Equal("VoidResultExecutor", actionMethodExecutor.GetType().Name);
Copy link
Member

@davidfowl davidfowl Apr 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These types are internal so a type check would work? i.e.

Assert.IsType<VoidResultExecutor>(actionMethodExecutor);

Copy link
Contributor Author

@rafikiassumani-msft rafikiassumani-msft Apr 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Assert.True(controller.Executed);
Assert.IsType<EmptyResult>(valueTask.Result);
}
Expand All @@ -38,6 +40,7 @@ public void ActionMethodExecutor_ExecutesActionsReturningIActionResult()
var valueTask = actionMethodExecutor.Execute(mapper, objectMethodExecutor, controller, Array.Empty<object>());

// Assert
Assert.Equal("SyncActionResultExecutor", actionMethodExecutor.GetType().Name);
Assert.True(valueTask.IsCompleted);
Assert.IsType<ContentResult>(valueTask.Result);
}
Expand All @@ -55,6 +58,7 @@ public void ActionMethodExecutor_ExecutesActionsReturningSubTypeOfActionResult()
var valueTask = actionMethodExecutor.Execute(mapper, objectMethodExecutor, controller, Array.Empty<object>());

// Assert
Assert.Equal("SyncActionResultExecutor", actionMethodExecutor.GetType().Name);
Assert.IsType<ContentResult>(valueTask.Result);
}

Expand All @@ -72,6 +76,8 @@ public void ActionMethodExecutor_ExecutesActionsReturningActionResultOfT()

// Assert
var result = Assert.IsType<ObjectResult>(valueTask.Result);

Assert.Equal("SyncObjectResultExecutor", actionMethodExecutor.GetType().Name);
Assert.NotNull(result.Value);
Assert.IsType<TestModel>(result.Value);
Assert.Equal(typeof(TestModel), result.DeclaredType);
Expand All @@ -91,6 +97,8 @@ public void ActionMethodExecutor_ExecutesActionsReturningModelAsModel()

// Assert
var result = Assert.IsType<ObjectResult>(valueTask.Result);

Assert.Equal("SyncObjectResultExecutor", actionMethodExecutor.GetType().Name);
Assert.NotNull(result.Value);
Assert.IsType<TestModel>(result.Value);
Assert.Equal(typeof(TestModel), result.DeclaredType);
Expand All @@ -110,6 +118,8 @@ public void ActionMethodExecutor_ExecutesActionsReturningModelAsObject()

// Assert
var result = Assert.IsType<ObjectResult>(valueTask.Result);

Assert.Equal("SyncObjectResultExecutor", actionMethodExecutor.GetType().Name);
Assert.NotNull(result.Value);
Assert.IsType<TestModel>(result.Value);
Assert.Equal(typeof(object), result.DeclaredType);
Expand All @@ -128,6 +138,7 @@ public void ActionMethodExecutor_ExecutesActionsReturningActionResultAsObject()
var valueTask = actionMethodExecutor.Execute(mapper, objectMethodExecutor, controller, Array.Empty<object>());

// Assert
Assert.Equal("SyncActionResultExecutor", actionMethodExecutor.GetType().Name);
Assert.IsType<ContentResult>(valueTask.Result);
}

Expand All @@ -144,10 +155,29 @@ public void ActionMethodExecutor_ExecutesActionsReturnTask()
var valueTask = actionMethodExecutor.Execute(mapper, objectMethodExecutor, controller, Array.Empty<object>());

// Assert
Assert.Equal("TaskResultExecutor", actionMethodExecutor.GetType().Name);
Assert.True(controller.Executed);
Assert.IsType<EmptyResult>(valueTask.Result);
}

[Fact]
public void ActionMethodExecutor_ExecutesActionsReturnAwaitable()
{
// Arrange
var mapper = new ActionResultTypeMapper();
var controller = new TestController();
var objectMethodExecutor = GetExecutor(nameof(TestController.ReturnsAwaitable));
var actionMethodExecutor = ActionMethodExecutor.GetExecutor(objectMethodExecutor);

// Act
var awaitableResult = actionMethodExecutor.Execute(mapper, objectMethodExecutor, controller, Array.Empty<object>());

// Assert
Assert.Equal("AwaitableResultExecutor", actionMethodExecutor.GetType().Name);
Assert.True(controller.Executed);
Assert.IsType<EmptyResult>(awaitableResult.Result);
}

[Fact]
public void ActionMethodExecutorExecutesActionsAsynchronouslyReturningIActionResult()
{
Expand All @@ -161,6 +191,7 @@ public void ActionMethodExecutorExecutesActionsAsynchronouslyReturningIActionRes
var valueTask = actionMethodExecutor.Execute(mapper, objectMethodExecutor, controller, Array.Empty<object>());

// Assert
Assert.Equal("TaskOfIActionResultExecutor", actionMethodExecutor.GetType().Name);
Assert.IsType<StatusCodeResult>(valueTask.Result);
}

Expand All @@ -170,17 +201,19 @@ public async Task ActionMethodExecutor_ExecutesActionsAsynchronouslyReturningAct
// Arrange
var mapper = new ActionResultTypeMapper();
var controller = new TestController();
var objectMethodExecutor = GetExecutor(nameof(TestController.ReturnIActionResultAsync));
var objectMethodExecutor = GetExecutor(nameof(TestController.ReturnActionResultAsync));
var actionMethodExecutor = ActionMethodExecutor.GetExecutor(objectMethodExecutor);

// Act
var valueTask = actionMethodExecutor.Execute(mapper, objectMethodExecutor, controller, Array.Empty<object>());

// Assert
await valueTask;
Assert.IsType<StatusCodeResult>(valueTask.Result);
Assert.Equal("TaskOfActionResultExecutor", actionMethodExecutor.GetType().Name);
Assert.IsType<ViewResult>(valueTask.Result);
}


[Fact]
public void ActionMethodExecutor_ExecutesActionsAsynchronouslyReturningModel()
{
Expand All @@ -195,6 +228,8 @@ public void ActionMethodExecutor_ExecutesActionsAsynchronouslyReturningModel()

// Assert
var result = Assert.IsType<ObjectResult>(valueTask.Result);

Assert.Equal("AwaitableObjectResultExecutor", actionMethodExecutor.GetType().Name);
Assert.NotNull(result.Value);
Assert.IsType<TestModel>(result.Value);
Assert.Equal(typeof(TestModel), result.DeclaredType);
Expand All @@ -214,6 +249,8 @@ public void ActionMethodExecutor_ExecutesActionsAsynchronouslyReturningModelAsOb

// Assert
var result = Assert.IsType<ObjectResult>(valueTask.Result);

Assert.Equal("AwaitableObjectResultExecutor", actionMethodExecutor.GetType().Name);
Assert.NotNull(result.Value);
Assert.IsType<TestModel>(result.Value);
Assert.Equal(typeof(object), result.DeclaredType);
Expand All @@ -232,6 +269,7 @@ public void ActionMethodExecutor_ExecutesActionsAsynchronouslyReturningIActionRe
var valueTask = actionMethodExecutor.Execute(mapper, objectMethodExecutor, controller, Array.Empty<object>());

// Assert
Assert.Equal("AwaitableObjectResultExecutor", actionMethodExecutor.GetType().Name);
Assert.IsType<OkResult>(valueTask.Result);
}

Expand All @@ -249,6 +287,8 @@ public void ActionMethodExecutor_ExecutesActionsAsynchronouslyReturningActionRes

// Assert
var result = Assert.IsType<ObjectResult>(valueTask.Result);

Assert.Equal("AwaitableObjectResultExecutor", actionMethodExecutor.GetType().Name);
Assert.NotNull(result.Value);
Assert.IsType<TestModel>(result.Value);
Assert.Equal(typeof(TestModel), result.DeclaredType);
Expand Down Expand Up @@ -304,8 +344,16 @@ public Task ReturnsTask()
return Task.CompletedTask;
}

public YieldAwaitable ReturnsAwaitable()
{
Executed = true;
return Task.Yield();
}

public Task<IActionResult> ReturnIActionResultAsync() => Task.FromResult((IActionResult)new StatusCodeResult(201));

public Task<ViewResult> ReturnActionResultAsync() => Task.FromResult(new ViewResult { StatusCode = 200});

public Task<StatusCodeResult> ReturnsIActionResultSubTypeAsync() => Task.FromResult(new StatusCodeResult(200));

public Task<TestModel> ReturnsModelAsModelAsync() => Task.FromResult(new TestModel());
Expand Down