diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionExecutor.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionExecutor.cs index e3fc64041f..e24f948f70 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionExecutor.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionExecutor.cs @@ -4,27 +4,13 @@ using System; using System.Collections.Generic; using System.ComponentModel; -using System.Linq; using System.Reflection; -using System.Runtime.ExceptionServices; using System.Threading.Tasks; -using Microsoft.AspNetCore.Mvc.Core; -using Microsoft.Extensions.Internal; namespace Microsoft.AspNetCore.Mvc.Internal { public static class ControllerActionExecutor { - private static readonly MethodInfo _convertOfTMethod = - typeof(ControllerActionExecutor).GetRuntimeMethods().Single(methodInfo => methodInfo.Name == "Convert"); - - // Method called via reflection. - private static Task Convert(object taskAsObject) - { - var task = (Task)taskAsObject; - return CastToObject(task); - } - public static Task ExecuteAsync( ObjectMethodExecutor actionMethodExecutor, object instance, @@ -39,57 +25,7 @@ namespace Microsoft.AspNetCore.Mvc.Internal object instance, object[] orderedActionArguments) { - object invocationResult = actionMethodExecutor.Execute(instance, orderedActionArguments); - return CoerceResultToTaskAsync( - invocationResult, - actionMethodExecutor.MethodInfo.ReturnType, - actionMethodExecutor.MethodInfo.Name, - actionMethodExecutor.MethodInfo.DeclaringType); - } - - // We need to CoerceResult as the object value returned from methodInfo.Invoke has to be cast to a Task. - // This is necessary to enable calling await on the returned task. - // i.e we need to write the following var result = await (Task)mInfo.Invoke. - // Returning Task enables us to await on the result. - // This method is intentionally not using async pattern to keep jit time (on cold start) to a minimum. - private static Task CoerceResultToTaskAsync( - object result, - Type returnType, - string methodName, - Type declaringType) - { - // If it is either a Task or Task - // must coerce the return value to Task - var resultAsTask = result as Task; - if (resultAsTask != null) - { - if (returnType == typeof(Task)) - { - ThrowIfWrappedTaskInstance(resultAsTask.GetType(), methodName, declaringType); - return CastToObject(resultAsTask); - } - - var taskValueType = GetTaskInnerTypeOrNull(returnType); - if (taskValueType != null) - { - // for: public Task Action() - // constructs: return (Task)Convert((Task)result) - var genericMethodInfo = _convertOfTMethod.MakeGenericMethod(taskValueType); - var convertedResult = (Task)genericMethodInfo.Invoke(null, new object[] { result }); - return convertedResult; - } - - // This will be the case for: - // 1. Types which have derived from Task and Task, - // 2. Action methods which use dynamic keyword but return a Task or Task. - throw new InvalidOperationException(Resources.FormatActionExecutor_UnexpectedTaskInstance( - methodName, - declaringType)); - } - else - { - return Task.FromResult(result); - } + return actionMethodExecutor.ExecuteAsync(instance, orderedActionArguments); } public static object[] PrepareArguments( @@ -137,47 +73,5 @@ namespace Microsoft.AspNetCore.Mvc.Internal return arguments; } - - private static void ThrowIfWrappedTaskInstance(Type actualTypeReturned, string methodName, Type declaringType) - { - // Throw if a method declares a return type of Task and returns an instance of Task or Task> - // This most likely indicates that the developer forgot to call Unwrap() somewhere. - if (actualTypeReturned != typeof(Task)) - { - var innerTaskType = GetTaskInnerTypeOrNull(actualTypeReturned); - if (innerTaskType != null && typeof(Task).IsAssignableFrom(innerTaskType)) - { - throw new InvalidOperationException( - Resources.FormatActionExecutor_WrappedTaskInstance( - methodName, - declaringType, - actualTypeReturned.FullName)); - } - } - } - - /// - /// Cast Task to Task of object - /// - private static async Task CastToObject(Task task) - { - await task; - return null; - } - - /// - /// Cast Task of T to Task of object - /// - private static async Task CastToObject(Task task) - { - return (object)await task; - } - - private static Type GetTaskInnerTypeOrNull(Type type) - { - var genericType = ClosedGenericMatcher.ExtractGenericInterface(type, typeof(Task<>)); - - return genericType?.GenericTypeArguments[0]; - } } } \ No newline at end of file diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ObjectMethodExecutor.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ObjectMethodExecutor.cs index b2e98fb717..b9dd6d8d58 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/ObjectMethodExecutor.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/ObjectMethodExecutor.cs @@ -3,15 +3,35 @@ using System; using System.Collections.Generic; +using System.Linq; using System.Linq.Expressions; using System.Reflection; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Mvc.Core; +using Microsoft.Extensions.Internal; namespace Microsoft.AspNetCore.Mvc.Internal { public class ObjectMethodExecutor { + private ActionExecutorAsync _executorAsync; private ActionExecutor _executor; + private static readonly MethodInfo _convertOfTMethod = + typeof(ObjectMethodExecutor).GetRuntimeMethods().Single(methodInfo => methodInfo.Name == nameof(ObjectMethodExecutor.Convert)); + + private static readonly Expression>> _createTaskFromResultExpression = + ((result) => Task.FromResult(result)); + + private static readonly MethodInfo _createTaskFromResultMethod = + ((MethodCallExpression)_createTaskFromResultExpression.Body).Method; + + private static readonly Expression>> _coerceTaskExpression = + ((result, methodName, declaringType) => ObjectMethodExecutor.CoerceTaskType(result, methodName, declaringType)); + + private static readonly MethodInfo _coerceMethod = ((MethodCallExpression)_coerceTaskExpression.Body).Method; + + private ObjectMethodExecutor(MethodInfo methodInfo) { if (methodInfo == null) @@ -21,6 +41,8 @@ namespace Microsoft.AspNetCore.Mvc.Internal MethodInfo = methodInfo; } + private delegate Task ActionExecutorAsync(object target, object[] parameters); + private delegate object ActionExecutor(object target, object[] parameters); private delegate void VoidActionExecutor(object target, object[] parameters); @@ -31,9 +53,15 @@ namespace Microsoft.AspNetCore.Mvc.Internal { var executor = new ObjectMethodExecutor(methodInfo); executor._executor = GetExecutor(methodInfo, targetTypeInfo); + executor._executorAsync = GetExecutorAsync(methodInfo, targetTypeInfo); return executor; } + public Task ExecuteAsync(object target, object[] parameters) + { + return _executorAsync(target, parameters); + } + public object Execute(object target, object[] parameters) { return _executor(target, parameters); @@ -87,5 +115,128 @@ namespace Microsoft.AspNetCore.Mvc.Internal return null; }; } + + private static ActionExecutorAsync GetExecutorAsync(MethodInfo methodInfo, TypeInfo targetTypeInfo) + { + // Parameters to executor + var targetParameter = Expression.Parameter(typeof(object), "target"); + var parametersParameter = Expression.Parameter(typeof(object[]), "parameters"); + + // Build parameter list + var parameters = new List(); + var paramInfos = methodInfo.GetParameters(); + for (int i = 0; i < paramInfos.Length; i++) + { + var paramInfo = paramInfos[i]; + var valueObj = Expression.ArrayIndex(parametersParameter, Expression.Constant(i)); + var valueCast = Expression.Convert(valueObj, paramInfo.ParameterType); + + // valueCast is "(Ti) parameters[i]" + parameters.Add(valueCast); + } + + // Call method + var instanceCast = Expression.Convert(targetParameter, targetTypeInfo.AsType()); + var methodCall = Expression.Call(instanceCast, methodInfo, parameters); + + // methodCall is "((Ttarget) target) method((T0) parameters[0], (T1) parameters[1], ...)" + // Create function + if (methodCall.Type == typeof(void)) + { + var lambda = Expression.Lambda(methodCall, targetParameter, parametersParameter); + var voidExecutor = lambda.Compile(); + return WrapVoidActionAsync(voidExecutor); + } + else + { + // must coerce methodCall to match ActionExecutorAsync signature + var coerceMethodCall = GetCoerceMethodCallExpression(methodCall, methodInfo); + var lambda = Expression.Lambda(coerceMethodCall, targetParameter, parametersParameter); + return lambda.Compile(); + } + } + + // We need to CoerceResult as the object value returned from methodInfo.Invoke has to be cast to a Task. + // This is necessary to enable calling await on the returned task. + // i.e we need to write the following var result = await (Task)mInfo.Invoke. + // Returning Task enables us to await on the result. + private static Expression GetCoerceMethodCallExpression(MethodCallExpression methodCall, MethodInfo methodInfo) + { + var castMethodCall = Expression.Convert(methodCall, typeof(object)); + var returnType = methodCall.Type; + + if (typeof(Task).IsAssignableFrom(returnType)) + { + if (returnType == typeof(Task)) + { + var stringExpression = Expression.Constant(methodInfo.Name); + var typeExpression = Expression.Constant(methodInfo.DeclaringType); + return Expression.Call(null, _coerceMethod, castMethodCall, stringExpression, typeExpression); + } + + var taskValueType = GetTaskInnerTypeOrNull(returnType); + if (taskValueType != null) + { + // for: public Task Action() + // constructs: return (Task)Convert((Task)result) + var genericMethodInfo = _convertOfTMethod.MakeGenericMethod(taskValueType); + var genericMethodCall = Expression.Call(null, genericMethodInfo, castMethodCall); + var convertedResult = Expression.Convert(genericMethodCall, typeof(Task)); + return convertedResult; + } + + // This will be the case for types which have derived from Task and Task + throw new InvalidOperationException(Resources.FormatActionExecutor_UnexpectedTaskInstance( + methodInfo.Name, + methodInfo.DeclaringType)); + } + + return Expression.Call(null, _createTaskFromResultMethod, castMethodCall); + } + + private static ActionExecutorAsync WrapVoidActionAsync(VoidActionExecutor executor) + { + return delegate (object target, object[] parameters) + { + executor(target, parameters); + return Task.FromResult(null); + }; + } + + private static Task CoerceTaskType(object result, string methodName, Type declaringType) + { + var resultAsTask = (Task)result; + return CastToObject(resultAsTask); + } + + /// + /// Cast Task to Task of object + /// + private static async Task CastToObject(Task task) + { + await task; + return null; + } + + /// + /// Cast Task of T to Task of object + /// + private static async Task CastToObject(Task task) + { + return (object)await task; + } + + private static Type GetTaskInnerTypeOrNull(Type type) + { + var genericType = ClosedGenericMatcher.ExtractGenericInterface(type, typeof(Task<>)); + + return genericType?.GenericTypeArguments[0]; + } + + private static Task Convert(object taskAsObject) + { + var task = (Task)taskAsObject; + return CastToObject(task); + } } } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionExecutorTests.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionExecutorTests.cs index bf1d49155a..38b962d130 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionExecutorTests.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ControllerActionExecutorTests.cs @@ -288,8 +288,10 @@ namespace Microsoft.AspNetCore.Mvc.Internal Assert.Equal(expectedException, ex.Message); } + // Async actions that declares a return type of Task and returns an instance of Task or Task> + // are not handled in any special way as they are not supported. They are considered as methods that return Task type. [Fact] - public async Task AsyncAction_ReturningUnwrappedTaskThrows() + public async Task AsyncAction_ReturningUnwrappedTask() { // Arrange var inputParam1 = 1; @@ -298,24 +300,20 @@ namespace Microsoft.AspNetCore.Mvc.Internal var methodWithUnwrappedTask = new MethodWithTaskReturnType(_controller.UnwrappedTask); - var expectedException = string.Format( - CultureInfo.CurrentCulture, - "The method 'UnwrappedTask' on type '{0}' returned an instance of '{1}'. " + - "Make sure to call Unwrap on the returned value to avoid unobserved faulted Task.", - typeof(TestController), - typeof(Task).FullName); + // Act + var result = await ExecuteAction( + methodWithUnwrappedTask, + _controller, + actionParameters); - // Act & Assert - var ex = await Assert.ThrowsAsync( - () => ExecuteAction( - methodWithUnwrappedTask, - _controller, - actionParameters)); - Assert.Equal(expectedException, ex.Message); + // Assert + Assert.Same(null, result); } [Fact] - public async Task AsyncAction_WithDynamicReturnTypeThrows() + // Async actions that has dynamic return type but return Task or Task are not handled in any special way as, + // they are not supported. They are considered as regular methods that return object type. + public async Task AsyncAction_WithDynamicReturnType() { // Arrange var inputParam1 = 1; @@ -323,18 +321,15 @@ namespace Microsoft.AspNetCore.Mvc.Internal var actionParameters = new Dictionary { { "i", inputParam1 }, { "s", inputParam2 } }; var dynamicTaskMethod = new ReturnTaskAsDynamicValue(_controller.ReturnTaskAsDynamicValue); - var expectedException = string.Format( - CultureInfo.CurrentCulture, - "The method 'ReturnTaskAsDynamicValue' on type '{0}' returned a Task instance even though it is not an asynchronous method.", - typeof(TestController)); - // Act & Assert - var ex = await Assert.ThrowsAsync( - () => ExecuteAction( - dynamicTaskMethod, - _controller, - actionParameters)); - Assert.Equal(expectedException, ex.Message); + // Act + var result = await (Task)(await ExecuteAction( + dynamicTaskMethod, + _controller, + actionParameters)); + + // Assert + Assert.Equal(inputParam1, result); } [Fact] @@ -462,12 +457,11 @@ namespace Microsoft.AspNetCore.Mvc.Internal } /// - /// Returns a instead of a . This should throw an - /// . + /// Returns a instead of a . /// public Task UnwrappedTask(int i, string s) { - return Task.Factory.StartNew(async () => await Task.Delay(50)); + return Task.Factory.StartNew(async () => await Task.Factory.StartNew(() => i)); } public string Echo(string input) diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ObjectMethodExecutorTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ObjectMethodExecutorTest.cs index 9c2f5142e0..f82a58d005 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ObjectMethodExecutorTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/ObjectMethodExecutorTest.cs @@ -2,13 +2,9 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; -using System.Collections.Generic; -using System.ComponentModel; using System.Globalization; using System.Reflection; using System.Threading.Tasks; -using Microsoft.AspNetCore.Mvc.Internal; -using Microsoft.AspNetCore.Testing; using Xunit; namespace Microsoft.AspNetCore.Mvc.Internal @@ -72,6 +68,107 @@ namespace Microsoft.AspNetCore.Mvc.Internal new object[] { parameter })); } + [Fact] + public async void ExecuteValueMethodUsingAsyncMethod() + { + var executor = GetExecutorForMethod("ValueMethod"); + var result = await executor.ExecuteAsync( + _targetObject, + new object[] { 10, 20 }); + Assert.Equal(30, (int)result); + } + + [Fact] + public async void ExecuteVoidValueMethodUsingAsyncMethod() + { + var executor = GetExecutorForMethod("VoidValueMethod"); + var result = await executor.ExecuteAsync( + _targetObject, + new object[] { 10 }); + Assert.Same(null, result); + } + + [Fact] + public async void ExecuteValueMethodAsync() + { + var executor = GetExecutorForMethod("ValueMethodAsync"); + var result = await executor.ExecuteAsync( + _targetObject, + new object[] { 10, 20 }); + Assert.Equal(30, (int)result); + } + + [Fact] + public async void ExecuteVoidValueMethodAsync() + { + var executor = GetExecutorForMethod("VoidValueMethodAsync"); + var result = await executor.ExecuteAsync( + _targetObject, + new object[] { 10 }); + Assert.Same(null, result); + } + + [Fact] + public async void ExecuteValueMethodWithReturnTypeAsync() + { + var executor = GetExecutorForMethod("ValueMethodWithReturnTypeAsync"); + var result = await executor.ExecuteAsync( + _targetObject, + new object[] { 10 }); + var resultObject = Assert.IsType(result); + Assert.Equal("Hello", resultObject.value); + } + + [Fact] + public async void ExecuteValueMethodUpdateValueAsync() + { + var executor = GetExecutorForMethod("ValueMethodUpdateValueAsync"); + var parameter = new TestObject(); + var result = await executor.ExecuteAsync( + _targetObject, + new object[] { parameter }); + var resultObject = Assert.IsType(result); + Assert.Equal("HelloWorld", resultObject.value); + } + + [Fact] + public async void ExecuteValueMethodWithReturnTypeThrowsExceptionAsync() + { + var executor = GetExecutorForMethod("ValueMethodWithReturnTypeThrowsExceptionAsync"); + var parameter = new TestObject(); + await Assert.ThrowsAsync( + () => executor.ExecuteAsync( + _targetObject, + new object[] { parameter })); + } + + [Fact] + public void ExecuteMethodOfTaskDerivedTypeReturnTypeThrowsException() + { + var expectedException = string.Format( + CultureInfo.CurrentCulture, + "The method 'TaskActionWithCustomTaskReturnType' on type '{0}' returned a Task instance even though it is not an asynchronous method.", + typeof(TestObject)); + + var ex = Assert.Throws( + () => GetExecutorForMethod("TaskActionWithCustomTaskReturnType")); + Assert.Equal(expectedException, ex.Message); + } + + [Fact] + public void ExecuteMethodOfTaskDerivedTypeOfTReturnTypeThrowsException() + { + var expectedException = string.Format( + CultureInfo.CurrentCulture, + "The method 'TaskActionWithCustomTaskOfTReturnType' on type '{0}' returned a Task instance even though it is not an asynchronous method.", + typeof(TestObject)); + + var ex = Assert.Throws( + () => GetExecutorForMethod("TaskActionWithCustomTaskOfTReturnType")); + + Assert.Equal(expectedException, ex.Message); + } + private ObjectMethodExecutor GetExecutorForMethod(string methodName) { var method = typeof(TestObject).GetMethod(methodName); @@ -106,6 +203,57 @@ namespace Microsoft.AspNetCore.Mvc.Internal parameter.value = "HelloWorld"; return parameter; } + + public Task ValueMethodAsync(int i, int j) + { + return Task.FromResult(i + j); + } + + public async Task VoidValueMethodAsync(int i) + { + await ValueMethodAsync(3, 4); + } + public Task ValueMethodWithReturnTypeAsync(int i) + { + return Task.FromResult(new TestObject() { value = "Hello" }); + } + + public Task ValueMethodWithReturnTypeThrowsExceptionAsync(TestObject i) + { + throw new NotImplementedException("Not Implemented Exception"); + } + + public Task ValueMethodUpdateValueAsync(TestObject parameter) + { + parameter.value = "HelloWorld"; + return Task.FromResult(parameter); + } + + public TaskDerivedType TaskActionWithCustomTaskReturnType(int i, string s) + { + return new TaskDerivedType(); + } + + public TaskOfTDerivedType TaskActionWithCustomTaskOfTReturnType(int i, string s) + { + return new TaskOfTDerivedType(1); + } + + public class TaskDerivedType : Task + { + public TaskDerivedType() + : base(() => Console.WriteLine("In The Constructor")) + { + } + } + + public class TaskOfTDerivedType : Task + { + public TaskOfTDerivedType(T input) + : base(() => input) + { + } + } } } }