From adeab1be93458cbe4b1d070ff3013fec8d382b4c Mon Sep 17 00:00:00 2001 From: Smit Patel Date: Wed, 25 Sep 2019 13:36:27 -0700 Subject: [PATCH] Improve exception message for memory leak warning Resolves #17623 --- src/EFCore/Properties/CoreStrings.Designer.cs | 24 +++++++++ src/EFCore/Properties/CoreStrings.resx | 9 ++++ .../ShapedQueryCompilingExpressionVisitor.cs | 54 ++++++++++++++++--- .../Query/SimpleQueryTestBase.cs | 24 ++++++--- 4 files changed, 99 insertions(+), 12 deletions(-) diff --git a/src/EFCore/Properties/CoreStrings.Designer.cs b/src/EFCore/Properties/CoreStrings.Designer.cs index 5dba85f2e26..c1c3c350391 100644 --- a/src/EFCore/Properties/CoreStrings.Designer.cs +++ b/src/EFCore/Properties/CoreStrings.Designer.cs @@ -2206,6 +2206,30 @@ public static string ClashingNonOwnedDerivedEntityType([CanBeNull] object entity GetString("ClashingNonOwnedDerivedEntityType", nameof(entityType), nameof(derivedType)), entityType, derivedType); + /// + /// Client projection contains reference to constant expression of '{constantType}' which is being passed as argument to method '{methodName}'. This could potentially cause memory leak. Consider assigning this constant to local variable and using the variable in the query instead. See https://go.microsoft.com/fwlink/?linkid=2103067 for more information. + /// + public static string ClientProjectionCapturingConstantInMethodArgument([CanBeNull] object constantType, [CanBeNull] object methodName) + => string.Format( + GetString("ClientProjectionCapturingConstantInMethodArgument", nameof(constantType), nameof(methodName)), + constantType, methodName); + + /// + /// Client projection contains reference to constant expression of '{constantType}' through instance method '{methodName}'. This could potentially cause memory leak. Consider making the method static so that it does not capture constant in the instance. See https://go.microsoft.com/fwlink/?linkid=2103067 for more information. + /// + public static string ClientProjectionCapturingConstantInMethodInstance([CanBeNull] object constantType, [CanBeNull] object methodName) + => string.Format( + GetString("ClientProjectionCapturingConstantInMethodInstance", nameof(constantType), nameof(methodName)), + constantType, methodName); + + /// + /// Client projection contains reference to constant expression of '{constantType}'. This could potentially cause memory leak. Consider assigning this constant to local variable and using the variable in the query instead. See https://go.microsoft.com/fwlink/?linkid=2103067 for more information. + /// + public static string ClientProjectionCapturingConstantInTree([CanBeNull] object constantType) + => string.Format( + GetString("ClientProjectionCapturingConstantInTree", nameof(constantType)), + constantType); + private static string GetString(string name, params string[] formatterNames) { var value = _resourceManager.GetString(name); diff --git a/src/EFCore/Properties/CoreStrings.resx b/src/EFCore/Properties/CoreStrings.resx index c3b86d2eb36..1ecba898b67 100644 --- a/src/EFCore/Properties/CoreStrings.resx +++ b/src/EFCore/Properties/CoreStrings.resx @@ -1222,4 +1222,13 @@ The type '{entityType}' cannot be marked as owned because the derived entity type - '{derivedType}' has been configured as non-owned. + + Client projection contains reference to constant expression of '{constantType}' which is being passed as argument to method '{methodName}'. This could potentially cause memory leak. Consider assigning this constant to local variable and using the variable in the query instead. See https://go.microsoft.com/fwlink/?linkid=2103067 for more information. + + + Client projection contains reference to constant expression of '{constantType}' through instance method '{methodName}'. This could potentially cause memory leak. Consider making the method static so that it does not capture constant in the instance. See https://go.microsoft.com/fwlink/?linkid=2103067 for more information. + + + Client projection contains reference to constant expression of '{constantType}'. This could potentially cause memory leak. Consider assigning this constant to local variable and using the variable in the query instead. See https://go.microsoft.com/fwlink/?linkid=2103067 for more information. + \ No newline at end of file diff --git a/src/EFCore/Query/ShapedQueryCompilingExpressionVisitor.cs b/src/EFCore/Query/ShapedQueryCompilingExpressionVisitor.cs index 8904b2a6aba..fffb4c2e1a4 100644 --- a/src/EFCore/Query/ShapedQueryCompilingExpressionVisitor.cs +++ b/src/EFCore/Query/ShapedQueryCompilingExpressionVisitor.cs @@ -172,17 +172,47 @@ public ConstantVerifyingExpressionVisitor(ITypeMappingSource typeMappingSource) _typeMappingSource = typeMappingSource; } + private bool ValidConstant(ConstantExpression constantExpression) + { + return constantExpression.Value == null + || _typeMappingSource.FindMapping(constantExpression.Type) != null; + } + protected override Expression VisitConstant(ConstantExpression constantExpression) { - if (constantExpression.Value == null - || _typeMappingSource.FindMapping(constantExpression.Type) != null) + if (!ValidConstant(constantExpression)) + { + throw new InvalidOperationException( + CoreStrings.ClientProjectionCapturingConstantInTree(constantExpression.Type.DisplayName())); + } + + return constantExpression; + } + + protected override Expression VisitMethodCall(MethodCallExpression methodCallExpression) + { + if (RemoveConvert(methodCallExpression.Object) is ConstantExpression constantInstance + && !ValidConstant(constantInstance)) + { + throw new InvalidOperationException( + CoreStrings.ClientProjectionCapturingConstantInMethodInstance( + constantInstance.Type.DisplayName(), + methodCallExpression.Method.Name)); + } + + foreach (var argument in methodCallExpression.Arguments) { - return constantExpression; + if (RemoveConvert(argument) is ConstantExpression constantArgument + && !ValidConstant(constantArgument)) + { + throw new InvalidOperationException( + CoreStrings.ClientProjectionCapturingConstantInMethodArgument( + constantArgument.Type.DisplayName(), + methodCallExpression.Method.Name)); + } } - throw new InvalidOperationException( - $"Client projection contains reference to constant expression of type: {constantExpression.Type.DisplayName()}. " + - "This could potentially cause memory leak."); + return base.VisitMethodCall(methodCallExpression); } protected override Expression VisitExtension(Expression extensionExpression) @@ -192,6 +222,18 @@ protected override Expression VisitExtension(Expression extensionExpression) ? extensionExpression : base.VisitExtension(extensionExpression); } + + private static Expression RemoveConvert(Expression expression) + { + while (expression != null + && (expression.NodeType == ExpressionType.Convert + || expression.NodeType == ExpressionType.ConvertChecked)) + { + expression = RemoveConvert(((UnaryExpression)expression).Operand); + } + + return expression; + } } private class EntityMaterializerInjectingExpressionVisitor : ExpressionVisitor diff --git a/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.cs index 6ded1fed86e..230590c3a35 100644 --- a/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/SimpleQueryTestBase.cs @@ -9,6 +9,7 @@ using System.Threading; using System.Threading.Tasks; using Microsoft.EntityFrameworkCore.Diagnostics; +using Microsoft.EntityFrameworkCore.Internal; using Microsoft.EntityFrameworkCore.TestModels.Northwind; using Microsoft.EntityFrameworkCore.TestUtilities; using Xunit; @@ -5617,8 +5618,12 @@ public virtual void Client_code_using_instance_method_throws() { using (var context = CreateContext()) { - Assert.Throws( - () => context.Customers.Select(c => InstanceMethod(c)).ToList()); + Assert.Equal( + CoreStrings.ClientProjectionCapturingConstantInMethodInstance( + GetType().DisplayName(), + nameof(InstanceMethod)), + Assert.Throws( + () => context.Customers.Select(c => InstanceMethod(c)).ToList()).Message); } } @@ -5629,8 +5634,12 @@ public virtual void Client_code_using_instance_in_static_method() { using (var context = CreateContext()) { - Assert.Throws( - () => context.Customers.Select(c => StaticMethod(this, c)).ToList()); + Assert.Equal( + CoreStrings.ClientProjectionCapturingConstantInMethodArgument( + GetType().DisplayName(), + nameof(StaticMethod)), + Assert.Throws( + () => context.Customers.Select(c => StaticMethod(this, c)).ToList()).Message); } } @@ -5641,8 +5650,11 @@ public virtual void Client_code_using_instance_in_anonymous_type() { using (var context = CreateContext()) { - Assert.Throws( - () => context.Customers.Select(c => new { A = this }).ToList()); + Assert.Equal( + CoreStrings.ClientProjectionCapturingConstantInTree( + GetType().DisplayName()), + Assert.Throws( + () => context.Customers.Select(c => new { A = this }).ToList()).Message); } }