Skip to content

Addressing memory leak in ModelContainer #1462

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Addressing memory leak in ModelContainer #1462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8d6c020
Removing model container and maintaining only one Edm model structure…
AlanWong-MS Jun 5, 2018
2427071
Dummy commit to kick off tests
AlanWong-MS Jun 5, 2018
8c9087f
Dummy commit to kick off test
AlanWong-MS Jun 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions build.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ if ($args.Count -eq 0)
$TestType = 'Nightly'
$Configuration = 'Release'
}
elseif ($args[0] -match 'quick' -or ($args[0] -match '-q'))
elseif ($args[0] -match 'quick' -or ($args[0] -match '-q'))
{
$TestType = "Quick"
}
Expand All @@ -43,7 +43,7 @@ elseif ($args[0] -match 'EnableSkipStrongName')
{
$TestType = "EnableSkipStrongName"
}
else
else
{
Error("Unknown input ""$args"". It can be empty or ""quick|DisableSkipStrongName|EnableSkipStrongName"".")
exit
Expand Down
1 change: 0 additions & 1 deletion src/Microsoft.AspNet.OData.Shared/Microsoft.AspNet.OData.Shared.projitems
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,6 @@
<Compile Include="$(MSBuildThisFileDirectory)Query\Expressions\PropertyContainer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Query\Expressions\SelectExpandBinder.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Query\Expressions\SelectExpandWrapper.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Query\Expressions\ModelContainer.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Query\Expressions\SelectExpandWrapperConverter.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Query\ITruncatedCollection.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Query\TruncatedCollectionOfT.cs" />
Expand Down
32 changes: 0 additions & 32 deletions src/Microsoft.AspNet.OData.Shared/Query/Expressions/ModelContainer.cs
View file
Open in desktop

This file was deleted.

9 changes: 3 additions & 6 deletions src/Microsoft.AspNet.OData.Shared/Query/Expressions/SelectExpandBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ internal class SelectExpandBinder
private ODataQueryContext _context;
private IEdmModel _model;
private ODataQuerySettings _settings;
private string _modelID;

public SelectExpandBinder(ODataQuerySettings settings, SelectExpandQueryOption selectExpandQuery)
{
Expand All @@ -42,7 +41,6 @@ public SelectExpandBinder(ODataQuerySettings settings, SelectExpandQueryOption s
_selectExpandQuery = selectExpandQuery;
_context = selectExpandQuery.Context;
_model = _context.Model;
_modelID = ModelContainer.GetModelID(_model);
_settings = settings;
}

Expand Down Expand Up @@ -313,10 +311,9 @@ private Expression ProjectElement(Expression source, SelectExpandClause selectEx
bool isTypeNamePropertySet = false;
bool isContainerPropertySet = false;

// Initialize property 'ModelID' on the wrapper class.
// source = new Wrapper { ModelID = 'some-guid-id' }
wrapperProperty = wrapperType.GetProperty("ModelID");
wrapperTypeMemberAssignments.Add(Expression.Bind(wrapperProperty, Expression.Constant(_modelID)));
// Initialize property 'Model' on the wrapper class
wrapperProperty = wrapperType.GetProperty("Model");
wrapperTypeMemberAssignments.Add(Expression.Bind(wrapperProperty, Expression.Constant(_model)));

// Initialize property 'Instance' on the wrapper class
// source => new Wrapper { Instance = element }
Expand Down
17 changes: 5 additions & 12 deletions src/Microsoft.AspNet.OData.Shared/Query/Expressions/SelectExpandWrapper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,9 @@ internal abstract class SelectExpandWrapper : IEdmEntityObject, ISelectExpandWra
public PropertyContainer Container { get; set; }

/// <summary>
/// An ID to uniquely identify the model in the <see cref="ModelContainer"/>.
/// Edm model corresponding to request.
/// </summary>
public string ModelID { get; set; }
public IEdmModel Model { get; set; }

/// <inheritdoc />
public object UntypedInstance { get; set; }
Expand All @@ -41,7 +41,7 @@ internal abstract class SelectExpandWrapper : IEdmEntityObject, ISelectExpandWra
/// <inheritdoc />
public IEdmTypeReference GetEdmType()
{
IEdmModel model = GetModel();
IEdmModel model = Model;
Type elementType = GetElementType();
return model.GetEdmTypeReference(elementType);
}
Expand All @@ -64,7 +64,7 @@ public bool TryGetPropertyValue(string propertyName, out object value)
if (UseInstanceForProperties && UntypedInstance != null)
{
_typedEdmEntityObject = _typedEdmEntityObject ??
new TypedEdmEntityObject(UntypedInstance, GetEdmType() as IEdmEntityTypeReference, GetModel());
new TypedEdmEntityObject(UntypedInstance, GetEdmType() as IEdmEntityTypeReference, Model);

return _typedEdmEntityObject.TryGetPropertyValue(propertyName, out value);
}
Expand All @@ -88,7 +88,7 @@ public IDictionary<string, object> ToDictionary(Func<IEdmModel, IEdmStructuredTy
Dictionary<string, object> dictionary = new Dictionary<string, object>();
IEdmStructuredType type = GetEdmType().AsStructured().StructuredDefinition();

IPropertyMapper mapper = mapperProvider(GetModel(), type);
IPropertyMapper mapper = mapperProvider(Model, type);
if (mapper == null)
{
throw Error.InvalidOperation(SRResources.InvalidPropertyMapper, typeof(IPropertyMapper).FullName,
Expand Down Expand Up @@ -123,12 +123,5 @@ public IDictionary<string, object> ToDictionary(Func<IEdmModel, IEdmStructuredTy
}

protected abstract Type GetElementType();

private IEdmModel GetModel()
{
Contract.Assert(ModelID != null);

return ModelContainer.GetModel(ModelID);
}
}
}
1 change: 0 additions & 1 deletion .../UnitTest/Microsoft.AspNet.OData.Test.Shared/Microsoft.AspNet.OData.Test.Shared.projitems
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,6 @@
<Compile Include="$(MSBuildThisFileDirectory)Query\Expressions\FilterBinderTests.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Query\Expressions\Linq2ObjectsComparisonMethodsTest.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Query\Expressions\LinqParameterContainerTest.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Query\Expressions\ModelContainerTest.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Query\Expressions\PropertyContainerTest.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Query\Expressions\SelectExpandBinderTest.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Query\Expressions\SelectExpandWrapperTest.cs" />
Expand Down
38 changes: 0 additions & 38 deletions test/UnitTest/Microsoft.AspNet.OData.Test.Shared/Query/Expressions/ModelContainerTest.cs
View file
Open in desktop

This file was deleted.

4 changes: 2 additions & 2 deletions test/UnitTest/Microsoft.AspNet.OData.Test.Shared/Query/Expressions/SelectExpandBinderTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,8 +299,8 @@ public void ProjectAsWrapper_Element_ProjectedValueContainsModelID()

// Assert
SelectExpandWrapper<Customer> customerWrapper = Expression.Lambda(projection).Compile().DynamicInvoke() as SelectExpandWrapper<Customer>;
Assert.NotNull(customerWrapper.ModelID);
Assert.Same(_model.Model, ModelContainer.GetModel(customerWrapper.ModelID));
Assert.NotNull(customerWrapper.Model);
Assert.Same(_model.Model, customerWrapper.Model);
}

[Theory]
Expand Down
40 changes: 19 additions & 21 deletions .../UnitTest/Microsoft.AspNet.OData.Test.Shared/Query/Expressions/SelectExpandWrapperTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,10 @@ namespace Microsoft.AspNet.OData.Test.Query.Expressions
public class SelectExpandWrapperTest
{
private CustomersModelWithInheritance _model;
private string _modelID;

public SelectExpandWrapperTest()
{
_model = new CustomersModelWithInheritance();
_modelID = ModelContainer.GetModelID(_model.Model);
}

[Fact]
Expand All @@ -45,7 +43,7 @@ public void GetEdmType_Returns_InstanceType()
{
_model.Model.SetAnnotationValue(_model.Customer, new ClrTypeAnnotation(typeof(TestEntity)));
_model.Model.SetAnnotationValue(_model.SpecialCustomer, new ClrTypeAnnotation(typeof(DerivedEntity)));
SelectExpandWrapper<TestEntity> wrapper = new SelectExpandWrapper<TestEntity> { ModelID = _modelID };
SelectExpandWrapper<TestEntity> wrapper = new SelectExpandWrapper<TestEntity> { Model = _model.Model };
wrapper.Instance = new DerivedEntity();

IEdmTypeReference edmType = wrapper.GetEdmType();
Expand All @@ -58,7 +56,7 @@ public void GetEdmType_Returns_ElementTypeIfInstanceIsNull()
{
_model.Model.SetAnnotationValue(_model.Customer, new ClrTypeAnnotation(typeof(TestEntity)));
_model.Model.SetAnnotationValue(_model.SpecialCustomer, new ClrTypeAnnotation(typeof(DerivedEntity)));
SelectExpandWrapper<TestEntity> wrapper = new SelectExpandWrapper<TestEntity> { ModelID = _modelID };
SelectExpandWrapper<TestEntity> wrapper = new SelectExpandWrapper<TestEntity> { Model = _model.Model };

IEdmTypeReference edmType = wrapper.GetEdmType();

Expand All @@ -72,10 +70,10 @@ public void TryGetValue_ReturnsValueFromPropertyContainer_IfPresent()
MockPropertyContainer container = new MockPropertyContainer();
container.Properties.Add("SampleProperty", expectedPropertyValue);
SelectExpandWrapper<TestEntity> wrapper = new SelectExpandWrapper<TestEntity>
{
ModelID = _modelID,
Container = container
};
{
Model = _model.Model,
Container = container
};
wrapper.Instance = new TestEntity();

object value;
Expand All @@ -91,10 +89,10 @@ public void TryGetValue_ReturnsValueFromInstance_IfNotPresentInContainer()
object expectedPropertyValue = new object();
MockPropertyContainer container = new MockPropertyContainer();
SelectExpandWrapper<TestEntity> wrapper = new SelectExpandWrapper<TestEntity>
{
ModelID = _modelID,
Container = container
};
{
Model = _model.Model,
Container = container
};
wrapper.Instance = new TestEntity { SampleProperty = expectedPropertyValue };
wrapper.UseInstanceForProperties = true;

Expand All @@ -109,7 +107,7 @@ public void TryGetValue_ReturnsValueFromInstance_IfNotPresentInContainer()
public void TryGetValue_ReturnsValueFromInstance_IfContainerIsNull()
{
object expectedPropertyValue = new object();
SelectExpandWrapper<TestEntity> wrapper = new SelectExpandWrapper<TestEntity> { ModelID = _modelID };
SelectExpandWrapper<TestEntity> wrapper = new SelectExpandWrapper<TestEntity> { Model = _model.Model };
wrapper.Instance = new TestEntity { SampleProperty = expectedPropertyValue };
wrapper.UseInstanceForProperties = true;

Expand All @@ -129,7 +127,7 @@ public void TryGetValue_PropertyAliased_IfAnnotationSet()
_model.CustomerName,
new ClrPropertyInfoAnnotation(typeof(TestEntityWithAlias).GetProperty("SampleProperty")));
object expectedPropertyValue = new object();
SelectExpandWrapper<TestEntityWithAlias> wrapper = new SelectExpandWrapper<TestEntityWithAlias> { ModelID = _modelID };
SelectExpandWrapper<TestEntityWithAlias> wrapper = new SelectExpandWrapper<TestEntityWithAlias> { Model = _model.Model };
wrapper.Instance = new TestEntityWithAlias { SampleProperty = expectedPropertyValue };
wrapper.UseInstanceForProperties = true;

Expand All @@ -145,7 +143,7 @@ public void TryGetValue_PropertyAliased_IfAnnotationSet()
[Fact]
public void TryGetValue_ReturnsFalse_IfContainerAndInstanceAreNull()
{
SelectExpandWrapper<TestEntity> wrapper = new SelectExpandWrapper<TestEntity> { ModelID = _modelID };
SelectExpandWrapper<TestEntity> wrapper = new SelectExpandWrapper<TestEntity> { Model = _model.Model };

object value;
bool result = wrapper.TryGetPropertyValue("SampleProperty", out value);
Expand All @@ -156,7 +154,7 @@ public void TryGetValue_ReturnsFalse_IfContainerAndInstanceAreNull()
[Fact]
public void TryGetValue_ReturnsFalse_IfPropertyNotPresentInElement()
{
SelectExpandWrapper<TestEntity> wrapper = new SelectExpandWrapper<TestEntity> { ModelID = _modelID };
SelectExpandWrapper<TestEntity> wrapper = new SelectExpandWrapper<TestEntity> { Model = _model.Model };

object value;
bool result = wrapper.TryGetPropertyValue("SampleNotPresentProperty", out value);
Expand All @@ -177,7 +175,7 @@ public void ToDictionary_ContainsAllStructuralProperties_IfInstanceIsNotNull()
SelectExpandWrapper<TestEntity> testWrapper = new SelectExpandWrapper<TestEntity>
{
Instance = new TestEntity { SampleProperty = 42 },
ModelID = ModelContainer.GetModelID(model),
Model = _model.Model,
UseInstanceForProperties = true,
};

Expand All @@ -202,7 +200,7 @@ public void ToDictionary_ContainsAllProperties_FromContainer()
SelectExpandWrapper<TestEntity> wrapper = new SelectExpandWrapper<TestEntity>
{
Container = container,
ModelID = ModelContainer.GetModelID(model)
Model = _model.Model
};

// Act
Expand Down Expand Up @@ -237,7 +235,7 @@ public void ToDictionary_Throws_IfMapperProvider_ReturnsNullPropertyMapper()
SelectExpandWrapper<TestEntity> wrapper = new SelectExpandWrapper<TestEntity>
{
Instance = new TestEntity { SampleProperty = 42 },
ModelID = ModelContainer.GetModelID(model)
Model = model
};

Func<IEdmModel, IEdmStructuredType, IPropertyMapper> mapperProvider =
Expand Down Expand Up @@ -266,7 +264,7 @@ public void ToDictionary_Throws_IfMappingIsNullOrEmpty_ForAGivenProperty(string
SelectExpandWrapper<TestEntity> testWrapper = new SelectExpandWrapper<TestEntity>
{
Instance = new TestEntity { SampleProperty = 42 },
ModelID = ModelContainer.GetModelID(model),
Model = _model.Model,
UseInstanceForProperties = true,
};

Expand Down Expand Up @@ -296,7 +294,7 @@ public void ToDictionary_AppliesMappingToAllProperties_IfInstanceIsNotNull()
SelectExpandWrapper<TestEntity> testWrapper = new SelectExpandWrapper<TestEntity>
{
Instance = new TestEntity { SampleProperty = 42 },
ModelID = ModelContainer.GetModelID(model),
Model = _model.Model,
UseInstanceForProperties = true,
};

Expand Down