From 9a83fa94c1a3c471c8d530b3f7fe481ad4449348 Mon Sep 17 00:00:00 2001 From: Matt Vance Date: Thu, 3 Oct 2024 15:27:47 -0700 Subject: [PATCH 1/3] Keep parameter values out of IMemoryCache in RelationalCommandCache Store only nullness and array lengths in struct form to prevent parameters memory leaks Fixes #34028 --- .../Query/Internal/RelationalCommandCache.cs | 70 +++++++++++++------ 1 file changed, 48 insertions(+), 22 deletions(-) diff --git a/src/EFCore.Relational/Query/Internal/RelationalCommandCache.cs b/src/EFCore.Relational/Query/Internal/RelationalCommandCache.cs index b8729a4cedf..a6a83044322 100644 --- a/src/EFCore.Relational/Query/Internal/RelationalCommandCache.cs +++ b/src/EFCore.Relational/Query/Internal/RelationalCommandCache.cs @@ -1,7 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Collections; using System.Collections.Concurrent; using System.Runtime.CompilerServices; using Microsoft.Extensions.Caching.Memory; @@ -106,11 +105,21 @@ void IPrintableExpression.Print(ExpressionPrinter expressionPrinter) } } - private readonly struct CommandCacheKey(Expression queryExpression, IReadOnlyDictionary parameterValues) + private readonly struct CommandCacheKey : IEquatable { - private readonly Expression _queryExpression = queryExpression; - private readonly IReadOnlyDictionary _parameterValues = parameterValues; + private readonly Expression _queryExpression; + private readonly ParameterValueInfo[] _parameterValues; + + internal CommandCacheKey(Expression queryExpression, IReadOnlyDictionary parameterValues) { + _queryExpression = queryExpression; + _parameterValues = new ParameterValueInfo[parameterValues.Count]; + var i = 0; + foreach (var (key, value) in parameterValues) + { + _parameterValues[i++] = new ParameterValueInfo(key, value); + } + } public override bool Equals(object? obj) => obj is CommandCacheKey commandCacheKey @@ -124,27 +133,27 @@ public bool Equals(CommandCacheKey commandCacheKey) return false; } - if (_parameterValues.Count > 0) + Check.DebugAssert( + _parameterValues.Length == commandCacheKey._parameterValues.Length, + "Parameter Count mismatch between identical expressions"); + + for (var i = 0; i < _parameterValues.Length; i++) { - foreach (var (key, value) in _parameterValues) - { - if (!commandCacheKey._parameterValues.TryGetValue(key, out var otherValue)) - { - return false; - } + var thisValue = _parameterValues[i]; + var otherValue = commandCacheKey._parameterValues[i]; - // ReSharper disable once ArrangeRedundantParentheses - if ((value == null) != (otherValue == null)) - { - return false; - } + Check.DebugAssert( + thisValue.Key == otherValue.Key, + "Parameter Name mismatch between identical expressions"); - if (value is IEnumerable - && value.GetType() == typeof(object[])) - { - // FromSql parameters must have the same number of elements - return ((object[])value).Length == (otherValue as object[])?.Length; - } + if (thisValue.IsNull != otherValue.IsNull) + { + return false; + } + + if (thisValue.ObjectArrayLength != otherValue.ObjectArrayLength) + { + return false; } } @@ -154,4 +163,21 @@ public bool Equals(CommandCacheKey commandCacheKey) public override int GetHashCode() => RuntimeHelpers.GetHashCode(_queryExpression); } + + // Note that we keep only the nullness of parameters (and array length for FromSql object arrays), and avoid referencing the actual parameter data (see #34028). + private readonly struct ParameterValueInfo + { + public string Key { get; } + + public bool IsNull { get; } + + public int? ObjectArrayLength { get; } + + internal ParameterValueInfo(string key, object? parameterValue) + { + Key = key; + IsNull = parameterValue == null; + ObjectArrayLength = parameterValue is object[] arr ? arr.Length : null; + } + } } From 26a404b70befb5db0bd15846fed45411557f3b77 Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Mon, 7 Oct 2024 20:58:39 +0200 Subject: [PATCH 2/3] Fixes and cleanup --- .../Query/Internal/RelationalCommandCache.cs | 62 +++++++------------ 1 file changed, 22 insertions(+), 40 deletions(-) diff --git a/src/EFCore.Relational/Query/Internal/RelationalCommandCache.cs b/src/EFCore.Relational/Query/Internal/RelationalCommandCache.cs index a6a83044322..dea8032a453 100644 --- a/src/EFCore.Relational/Query/Internal/RelationalCommandCache.cs +++ b/src/EFCore.Relational/Query/Internal/RelationalCommandCache.cs @@ -105,19 +105,23 @@ void IPrintableExpression.Print(ExpressionPrinter expressionPrinter) } } - private readonly struct CommandCacheKey - : IEquatable + private readonly struct CommandCacheKey : IEquatable { private readonly Expression _queryExpression; - private readonly ParameterValueInfo[] _parameterValues; + private readonly Dictionary _parameterInfos; - internal CommandCacheKey(Expression queryExpression, IReadOnlyDictionary parameterValues) { + internal CommandCacheKey(Expression queryExpression, IReadOnlyDictionary parameterValues) + { _queryExpression = queryExpression; - _parameterValues = new ParameterValueInfo[parameterValues.Count]; - var i = 0; + _parameterInfos = new Dictionary(); + foreach (var (key, value) in parameterValues) { - _parameterValues[i++] = new ParameterValueInfo(key, value); + _parameterInfos[key] = new ParameterInfo + { + IsNull = value == null, + ObjectArrayLength = value is object[] arr ? arr.Length : null + }; } } @@ -134,26 +138,17 @@ public bool Equals(CommandCacheKey commandCacheKey) } Check.DebugAssert( - _parameterValues.Length == commandCacheKey._parameterValues.Length, - "Parameter Count mismatch between identical expressions"); + _parameterInfos.Count == commandCacheKey._parameterInfos.Count, + "Parameter Count mismatch between identical queries"); - for (var i = 0; i < _parameterValues.Length; i++) + if (_parameterInfos.Count > 0) { - var thisValue = _parameterValues[i]; - var otherValue = commandCacheKey._parameterValues[i]; - - Check.DebugAssert( - thisValue.Key == otherValue.Key, - "Parameter Name mismatch between identical expressions"); - - if (thisValue.IsNull != otherValue.IsNull) + foreach (var (key, info) in _parameterInfos) { - return false; - } - - if (thisValue.ObjectArrayLength != otherValue.ObjectArrayLength) - { - return false; + if (!commandCacheKey._parameterInfos.TryGetValue(key, out var otherInfo) || info != otherInfo) + { + return false; + } } } @@ -164,20 +159,7 @@ public override int GetHashCode() => RuntimeHelpers.GetHashCode(_queryExpression); } - // Note that we keep only the nullness of parameters (and array length for FromSql object arrays), and avoid referencing the actual parameter data (see #34028). - private readonly struct ParameterValueInfo - { - public string Key { get; } - - public bool IsNull { get; } - - public int? ObjectArrayLength { get; } - - internal ParameterValueInfo(string key, object? parameterValue) - { - Key = key; - IsNull = parameterValue == null; - ObjectArrayLength = parameterValue is object[] arr ? arr.Length : null; - } - } + // Note that we keep only the null-ness of parameters (and array length for FromSql object arrays), + // and avoid referencing the actual parameter data (see #34028). + private readonly record struct ParameterInfo(bool IsNull, int? ObjectArrayLength); } From 829a44329cdb5c619bb656e08089862d8127d613 Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Mon, 7 Oct 2024 21:18:08 +0200 Subject: [PATCH 3/3] More accurate null check --- src/EFCore.Relational/Query/Internal/RelationalCommandCache.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/EFCore.Relational/Query/Internal/RelationalCommandCache.cs b/src/EFCore.Relational/Query/Internal/RelationalCommandCache.cs index dea8032a453..c4edba62a40 100644 --- a/src/EFCore.Relational/Query/Internal/RelationalCommandCache.cs +++ b/src/EFCore.Relational/Query/Internal/RelationalCommandCache.cs @@ -119,7 +119,7 @@ internal CommandCacheKey(Expression queryExpression, IReadOnlyDictionary