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
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.
  • Loading branch information
ajcvickers committed Sep 26, 2023
1 parent d616f39 commit e812d35
Show file tree
Hide file tree
Showing 7 changed files with 36 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ private IReadOnlyList<MigrationOperation> RewriteOperations(
createTableOperation.PrimaryKey = AddPrimaryKeyOperation.CreateFrom(primaryKey);
}

foreach (var column in table.Columns.Where(c => c.Order.HasValue).OrderBy(c => c.Order.Value)
foreach (var column in table.Columns.Where(c => c.Order.HasValue).OrderBy(c => c.Order!.Value)
.Concat(table.Columns.Where(c => !c.Order.HasValue)))
{
if (!column.TryGetDefaultValue(out var defaultValue))
Expand Down
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
5 changes: 5 additions & 0 deletions src/EFCore/Metadata/IReadOnlyModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,11 @@ private static int GetDerivedLevel(Type? derivedType, Dictionary<Type, int> deri
return level;
}

/// <summary>
/// A unique identifier for the <see cref="Model"/> instance and the <see cref="IRuntimeModel"/> created from it.
/// </summary>
public Guid ModelId { get; }

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

/// <inheritdoc />
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
9 changes: 9 additions & 0 deletions src/EFCore/Metadata/RuntimeModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,11 @@ private IEnumerable<RuntimeEntityType> FindEntityTypes(Type type)
: result;
}

/// <summary>
/// A unique identifier for the <see cref="Model"/> instance and the <see cref="IRuntimeModel"/> created from it.
/// </summary>
public virtual Guid ModelId { get; set; }

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

/// <inheritdoc />
Guid IReadOnlyModel.ModelId
=> ModelId;
}
}
22 changes: 13 additions & 9 deletions src/EFCore/ValueGeneration/ValueGeneratorCache.cs
Original file line number Diff line number Diff line change
Expand Up @@ -49,24 +49,27 @@ 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;
_modelId = entityType.Model.ModelId;
_property = property.Name;
_entityType = entityType.Name;
}

public IProperty Property { get; }

public IEntityType EntityType { get; }

public bool Equals(CacheKey other)
=> 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);
=> HashCode.Combine(_property, _entityType, _modelId);
}

/// <summary>
Expand All @@ -88,7 +91,8 @@ public virtual ValueGenerator GetOrAdd(
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 e812d35

Please sign in to comment.