From f477f7e968b548cfd0590cec9252f42c25397a80 Mon Sep 17 00:00:00 2001 From: Matthew Vance Date: Mon, 14 Oct 2024 13:41:32 -0700 Subject: [PATCH 1/2] Keep parameter values out IMemoryCache in RelationalCommandCache Store only nullness and array lengths in struct form to prevent parameters memory leaks Fixes #34028 Co-authored-by: Shay Rojansky (cherry picked from commit af420cd58113725dcbe36b02348d835cefc898a1) --- .../Query/Internal/RelationalCommandCache.cs | 43 ++++++++++--------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/src/EFCore.Relational/Query/Internal/RelationalCommandCache.cs b/src/EFCore.Relational/Query/Internal/RelationalCommandCache.cs index d0c18bb5afd..d4dbcc3e072 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 Microsoft.Extensions.Caching.Memory; @@ -106,12 +105,21 @@ void IPrintableExpression.Print(ExpressionPrinter expressionPrinter) private readonly struct CommandCacheKey : IEquatable { private readonly Expression _queryExpression; - private readonly IReadOnlyDictionary _parameterValues; + private readonly Dictionary _parameterInfos; - public CommandCacheKey(Expression queryExpression, IReadOnlyDictionary parameterValues) + internal CommandCacheKey(Expression queryExpression, IReadOnlyDictionary parameterValues) { _queryExpression = queryExpression; - _parameterValues = parameterValues; + _parameterInfos = new Dictionary(); + + foreach (var (key, value) in parameterValues) + { + _parameterInfos[key] = new ParameterInfo + { + IsNull = value is null, + ObjectArrayLength = value is object[] arr ? arr.Length : null + }; + } } public override bool Equals(object? obj) @@ -126,27 +134,18 @@ public bool Equals(CommandCacheKey commandCacheKey) return false; } - if (_parameterValues.Count > 0) + Check.DebugAssert( + _parameterInfos.Count == commandCacheKey._parameterInfos.Count, + "Parameter Count mismatch between identical queries"); + + if (_parameterInfos.Count > 0) { - foreach (var (key, value) in _parameterValues) + foreach (var (key, info) in _parameterInfos) { - if (!commandCacheKey._parameterValues.TryGetValue(key, out var otherValue)) + if (!commandCacheKey._parameterInfos.TryGetValue(key, out var otherInfo) || info != otherInfo) { return false; } - - // ReSharper disable once ArrangeRedundantParentheses - if ((value == null) != (otherValue == null)) - { - return false; - } - - 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; - } } } @@ -156,4 +155,8 @@ public bool Equals(CommandCacheKey commandCacheKey) public override int GetHashCode() => 0; } + + // 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 9eaf3fc10d89f84f0ca6c47c69c212b213f7fa2d Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Mon, 14 Oct 2024 23:41:34 +0200 Subject: [PATCH 2/2] Add quirk --- .../Query/Internal/RelationalCommandCache.cs | 58 ++++++++++++++++--- 1 file changed, 49 insertions(+), 9 deletions(-) diff --git a/src/EFCore.Relational/Query/Internal/RelationalCommandCache.cs b/src/EFCore.Relational/Query/Internal/RelationalCommandCache.cs index d4dbcc3e072..2eceda4584d 100644 --- a/src/EFCore.Relational/Query/Internal/RelationalCommandCache.cs +++ b/src/EFCore.Relational/Query/Internal/RelationalCommandCache.cs @@ -1,6 +1,7 @@ // 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 Microsoft.Extensions.Caching.Memory; @@ -104,21 +105,34 @@ void IPrintableExpression.Print(ExpressionPrinter expressionPrinter) private readonly struct CommandCacheKey : IEquatable { + private static readonly bool UseOldBehavior34201 = + AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue34028", out var enabled34028) && enabled34028; + private readonly Expression _queryExpression; private readonly Dictionary _parameterInfos; - internal CommandCacheKey(Expression queryExpression, IReadOnlyDictionary parameterValues) + // Quirk only + private readonly IReadOnlyDictionary? _parameterValues; + + public CommandCacheKey(Expression queryExpression, IReadOnlyDictionary parameterValues) { _queryExpression = queryExpression; _parameterInfos = new Dictionary(); - foreach (var (key, value) in parameterValues) + if (UseOldBehavior34201) { - _parameterInfos[key] = new ParameterInfo + _parameterValues = parameterValues; + } + else + { + foreach (var (key, value) in parameterValues) { - IsNull = value is null, - ObjectArrayLength = value is object[] arr ? arr.Length : null - }; + _parameterInfos[key] = new ParameterInfo + { + IsNull = value is null, + ObjectArrayLength = value is object[] arr ? arr.Length : null + }; + } } } @@ -140,11 +154,37 @@ public bool Equals(CommandCacheKey commandCacheKey) if (_parameterInfos.Count > 0) { - foreach (var (key, info) in _parameterInfos) + if (UseOldBehavior34201) + { + foreach (var (key, value) in _parameterValues!) + { + if (!_parameterValues.TryGetValue(key, out var otherValue)) + { + return false; + } + + // ReSharper disable once ArrangeRedundantParentheses + if ((value == null) != (otherValue == null)) + { + return false; + } + + 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; + } + } + } + else { - if (!commandCacheKey._parameterInfos.TryGetValue(key, out var otherInfo) || info != otherInfo) + foreach (var (key, info) in _parameterInfos) { - return false; + if (!commandCacheKey._parameterInfos.TryGetValue(key, out var otherInfo) || info != otherInfo) + { + return false; + } } } }