Skip to content

Commit

Permalink
[release/6.0] Stop storing model references in the ValueGeneratorCache (
Browse files Browse the repository at this point in the history
#31867)

* [release/6.0] Stop storing model references in the ValueGeneratorCache

Fixes #31539

### Description

Storing references to the model resulted in a customer reported memory leak in multi-tenant application.

### Customer impact

Customer-reported large memory leak.

### How found

Customer reported.

### Regression

No.

### Testing

Existing tests cover that there is no regression with the cache key changed. Manual testing that the model is no longer referenced.

### Risk

Low. Also quirked.

* Quirk

* Make model ID API pubternal
  • Loading branch information
ajcvickers committed Oct 4, 2023
1 parent 7fb644d commit b285a6e
Show file tree
Hide file tree
Showing 6 changed files with 70 additions and 12 deletions.
1 change: 1 addition & 0 deletions src/EFCore/Metadata/Conventions/RuntimeModelConvention.cs
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ public virtual IModel ProcessModelFinalized(IModel model)
protected virtual RuntimeModel Create(IModel model)
{
var runtimeModel = new RuntimeModel();
runtimeModel.ModelId = model.ModelId;
runtimeModel.SetSkipDetectChanges(((IRuntimeModel)model).SkipDetectChanges);
((IModel)runtimeModel).ModelDependencies = model.ModelDependencies!;

Expand Down
9 changes: 9 additions & 0 deletions src/EFCore/Metadata/IReadOnlyModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,15 @@ private static int GetDerivedLevel(Type? derivedType, Dictionary<Type, int> deri
return level;
}

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
/// the same compatibility standards as public APIs. It may be changed or removed without notice in
/// any release. You should only use it directly in your code with extreme caution and knowing that
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
[EntityFrameworkInternal]
public Guid ModelId { get; }

/// <summary>
/// <para>
/// Creates a human-readable representation of the given metadata.
Expand Down
8 changes: 8 additions & 0 deletions src/EFCore/Metadata/Internal/Model.cs
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,14 @@ public virtual IEnumerable<EntityType> FindEntityTypes(Type type)
: result;
}

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
/// the same compatibility standards as public APIs. It may be changed or removed without notice in
/// any release. You should only use it directly in your code with extreme caution and knowing that
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public virtual Guid ModelId { get; } = Guid.NewGuid();

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
/// the same compatibility standards as public APIs. It may be changed or removed without notice in
Expand Down
13 changes: 13 additions & 0 deletions src/EFCore/Metadata/RuntimeModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,15 @@ private IEnumerable<RuntimeEntityType> FindEntityTypes(Type type)
: result;
}

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
/// the same compatibility standards as public APIs. It may be changed or removed without notice in
/// any release. You should only use it directly in your code with extreme caution and knowing that
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
[EntityFrameworkInternal]
public virtual Guid ModelId { get; set; }

/// <summary>
/// Adds configuration for a scalar type.
/// </summary>
Expand Down Expand Up @@ -302,5 +311,9 @@ IEnumerable<ITypeMappingConfiguration> IModel.GetTypeMappingConfigurations()
=> _typeConfigurations.Count == 0
? null
: _typeConfigurations.GetValueOrDefault(propertyType);

/// <inheritdoc />
Guid IReadOnlyModel.ModelId
=> ModelId;
}
}
42 changes: 35 additions & 7 deletions src/EFCore/ValueGeneration/ValueGeneratorCache.cs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ namespace Microsoft.EntityFrameworkCore.ValueGeneration
/// </remarks>
public class ValueGeneratorCache : IValueGeneratorCache
{
private static readonly bool _useOldBehavior31539 =
AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue31539", out var enabled31539) && enabled31539;

/// <summary>
/// Initializes a new instance of the <see cref="ValueGeneratorCache" /> class.
/// </summary>
Expand All @@ -49,24 +52,48 @@ public ValueGeneratorCache(ValueGeneratorCacheDependencies dependencies)

private readonly struct CacheKey : IEquatable<CacheKey>
{
private readonly Guid _modelId;
private readonly string? _property;
private readonly string? _entityType;

public CacheKey(IProperty property, IEntityType entityType)
{
Property = property;
EntityType = entityType;
if (_useOldBehavior31539)
{
_modelId = default;
_property = null;
_entityType = null;
Property = property;
EntityType = entityType;
}
else
{
_modelId = entityType.Model.ModelId;
_property = property.Name;
_entityType = entityType.Name;
Property = null;
EntityType = null;
}
}

public IProperty Property { get; }
public IProperty? Property { get; }

public IEntityType EntityType { get; }
public IEntityType? EntityType { get; }

public bool Equals(CacheKey other)
=> Property.Equals(other.Property) && EntityType.Equals(other.EntityType);
=> _useOldBehavior31539
? Property!.Equals(other.Property) && EntityType!.Equals(other.EntityType)
: (_property!.Equals(other._property, StringComparison.Ordinal)
&& _entityType!.Equals(other._entityType, StringComparison.Ordinal)
&& _modelId.Equals(other._modelId));

public override bool Equals(object? obj)
=> obj is CacheKey cacheKey && Equals(cacheKey);

public override int GetHashCode()
=> HashCode.Combine(Property, EntityType);
=> _useOldBehavior31539
? HashCode.Combine(Property!, EntityType!)
: HashCode.Combine(_property!, _entityType!, _modelId);
}

/// <summary>
Expand All @@ -88,7 +115,8 @@ public override int GetHashCode()
Check.NotNull(property, nameof(property));
Check.NotNull(factory, nameof(factory));

return _cache.GetOrAdd(new CacheKey(property, entityType), static (ck, f) => f(ck.Property, ck.EntityType), factory);
return _cache.GetOrAdd(
new CacheKey(property, entityType), static (ck, p) => p.factory(p.property, p.entityType), (factory, entityType, property));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -169,18 +169,17 @@ public void Identifying_foreign_key_value_is_generated_if_principal_key_not_set(
[InlineData(true, true)]
public void Identifying_foreign_key_value_is_propagated_if_principal_key_is_generated(bool generateTemporary, bool async)
{
var model = BuildModel(generateTemporary);

var principal = new Product();
var dependent = new ProductDetail { Product = principal };

var contextServices = CreateContextServices(model);
var contextServices = CreateContextServices(BuildModel(generateTemporary));
var stateManager = contextServices.GetRequiredService<IStateManager>();
var principalEntry = stateManager.GetOrCreateEntry(principal);
principalEntry.SetEntityState(EntityState.Added);
var dependentEntry = stateManager.GetOrCreateEntry(dependent);
var principalProperty = model.FindEntityType(typeof(Product)).FindProperty(nameof(Product.Id));
var dependentProperty = model.FindEntityType(typeof(ProductDetail)).FindProperty(nameof(ProductDetail.Id));
var runtimeModel = contextServices.GetRequiredService<IModel>();
var principalProperty = runtimeModel.FindEntityType(typeof(Product))!.FindProperty(nameof(Product.Id))!;
var dependentProperty = runtimeModel.FindEntityType(typeof(ProductDetail))!.FindProperty(nameof(ProductDetail.Id))!;
var keyPropagator = contextServices.GetRequiredService<IKeyPropagator>();

PropagateValue(keyPropagator, dependentEntry, dependentProperty, async);
Expand Down

0 comments on commit b285a6e

Please sign in to comment.