From e7e67d63e59d27d8c795cec973bf89ee905fbafc Mon Sep 17 00:00:00 2001 From: Arthur Vickers Date: Thu, 22 Sep 2022 12:54:11 +0100 Subject: [PATCH 1/2] [release/7.0] Reset LocalView when returning context to the pool Fixes #29164 This change treats the LocalView as one of the resettable services and resets it rather than severing when the context is returned to the pool. This fixes a memory leak because the view was previously recreated and re-registered with _the same StateManager_ every time the context was re-used, these registrations were never cleared. But since the StateManager is reused, the local views can also be reused, meaning that the registration is retained rather than repeated across uses of the context instance. --- src/EFCore/ChangeTracking/LocalView.cs | 34 ++++++++++++++++++- src/EFCore/Internal/InternalDbSet.cs | 13 ++++--- .../DbContextPoolingTest.cs | 11 ++++++ 3 files changed, 50 insertions(+), 8 deletions(-) diff --git a/src/EFCore/ChangeTracking/LocalView.cs b/src/EFCore/ChangeTracking/LocalView.cs index ffaa0bb46ae..2f7ec5b652a 100644 --- a/src/EFCore/ChangeTracking/LocalView.cs +++ b/src/EFCore/ChangeTracking/LocalView.cs @@ -51,7 +51,8 @@ public class LocalView<[DynamicallyAccessedMembers(IEntityType.DynamicallyAccess INotifyCollectionChanged, INotifyPropertyChanged, INotifyPropertyChanging, - IListSource + IListSource, + IResettableService where TEntity : class { private ObservableBackedBindingList? _bindingList; @@ -499,4 +500,35 @@ IList IListSource.GetList() /// bool IListSource.ContainsListCollection => false; + + /// + /// 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. + /// + [EntityFrameworkInternal] + void IResettableService.ResetState() + { + _bindingList = null; + _observable = null; + _countChanges = 0; + _count = 0; + _triggeringStateManagerChange = false; + _triggeringObservableChange = false; + _triggeringLocalViewChange = false; + } + + /// + /// 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. + /// + [EntityFrameworkInternal] + Task IResettableService.ResetStateAsync(CancellationToken cancellationToken) + { + ((IResettableService)this).ResetState(); + return Task.CompletedTask; + } } diff --git a/src/EFCore/Internal/InternalDbSet.cs b/src/EFCore/Internal/InternalDbSet.cs index c5b954f07de..8e22892dd7e 100644 --- a/src/EFCore/Internal/InternalDbSet.cs +++ b/src/EFCore/Internal/InternalDbSet.cs @@ -520,7 +520,7 @@ IServiceProvider IInfrastructure.Instance /// doing so can result in application failures when updating to a new Entity Framework Core release. /// void IResettableService.ResetState() - => _localView = null; + => ((IResettableService?)_localView)?.ResetState(); /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to @@ -528,13 +528,12 @@ void IResettableService.ResetState() /// 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. /// - /// A to observe while waiting for the task to complete. - /// If the is canceled. - Task IResettableService.ResetStateAsync(CancellationToken cancellationToken) + async Task IResettableService.ResetStateAsync(CancellationToken cancellationToken) { - ((IResettableService)this).ResetState(); - - return Task.CompletedTask; + if (_localView != null) + { + await ((IResettableService)_localView).ResetStateAsync(cancellationToken).ConfigureAwait(false); + } } private EntityEntry EntryWithoutDetectChanges(TEntity entity) diff --git a/test/EFCore.SqlServer.FunctionalTests/DbContextPoolingTest.cs b/test/EFCore.SqlServer.FunctionalTests/DbContextPoolingTest.cs index 0d97fbe7d00..d06c757eb4b 100644 --- a/test/EFCore.SqlServer.FunctionalTests/DbContextPoolingTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/DbContextPoolingTest.cs @@ -768,6 +768,9 @@ public async Task Context_configuration_is_reset(bool useInterface, bool async) Assert.Null(context1!.Database.GetCommandTimeout()); + var set = context1.Customers; + var localView = set.Local; + context1.ChangeTracker.AutoDetectChangesEnabled = true; context1.ChangeTracker.LazyLoadingEnabled = true; context1.ChangeTracker.QueryTrackingBehavior = QueryTrackingBehavior.NoTracking; @@ -826,6 +829,9 @@ public async Task Context_configuration_is_reset(bool useInterface, bool async) Assert.False(_context_OnSavedChanges); Assert.False(_context_OnSavingChanges); Assert.False(_context_OnSaveChangesFailed); + + Assert.Same(set, context2!.Customers); + Assert.Same(localView, context2!.Customers.Local); } [ConditionalTheory] @@ -865,6 +871,8 @@ public async Task Context_configuration_is_reset_with_factory(bool async, bool w var factory = BuildFactory(withDependencyInjection); var context1 = async ? await factory.CreateDbContextAsync() : factory.CreateDbContext(); + var set = context1.Customers; + var localView = set.Local; context1.ChangeTracker.AutoDetectChangesEnabled = true; context1.ChangeTracker.LazyLoadingEnabled = true; @@ -909,6 +917,9 @@ public async Task Context_configuration_is_reset_with_factory(bool async, bool w Assert.False(_context_OnSavedChanges); Assert.False(_context_OnSavingChanges); Assert.False(_context_OnSaveChangesFailed); + + Assert.Same(set, context2!.Customers); + Assert.Same(localView, context2!.Customers.Local); } [ConditionalFact] From 93e7221c355562d18f320c5670b8906bfc04bc9f Mon Sep 17 00:00:00 2001 From: Arthur Vickers Date: Thu, 22 Sep 2022 16:42:32 +0100 Subject: [PATCH 2/2] Updated. --- src/EFCore/ChangeTracking/LocalView.cs | 29 ++----- src/EFCore/Internal/InternalDbSet.cs | 11 ++- .../DbContextPoolingTest.cs | 87 +++++++++++++++++-- 3 files changed, 94 insertions(+), 33 deletions(-) diff --git a/src/EFCore/ChangeTracking/LocalView.cs b/src/EFCore/ChangeTracking/LocalView.cs index 2f7ec5b652a..bd0e4f22d0d 100644 --- a/src/EFCore/ChangeTracking/LocalView.cs +++ b/src/EFCore/ChangeTracking/LocalView.cs @@ -51,8 +51,7 @@ public class LocalView<[DynamicallyAccessedMembers(IEntityType.DynamicallyAccess INotifyCollectionChanged, INotifyPropertyChanged, INotifyPropertyChanging, - IListSource, - IResettableService + IListSource where TEntity : class { private ObservableBackedBindingList? _bindingList; @@ -502,13 +501,11 @@ bool IListSource.ContainsListCollection => false; /// - /// 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. + /// Resets this view, clearing any created with and + /// any created with , and clearing any + /// events registered on , , or . /// - [EntityFrameworkInternal] - void IResettableService.ResetState() + public virtual void Reset() { _bindingList = null; _observable = null; @@ -517,18 +514,8 @@ void IResettableService.ResetState() _triggeringStateManagerChange = false; _triggeringObservableChange = false; _triggeringLocalViewChange = false; - } - - /// - /// 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. - /// - [EntityFrameworkInternal] - Task IResettableService.ResetStateAsync(CancellationToken cancellationToken) - { - ((IResettableService)this).ResetState(); - return Task.CompletedTask; + PropertyChanged = null; + PropertyChanging = null; + CollectionChanged = null; } } diff --git a/src/EFCore/Internal/InternalDbSet.cs b/src/EFCore/Internal/InternalDbSet.cs index 8e22892dd7e..0516c793aa8 100644 --- a/src/EFCore/Internal/InternalDbSet.cs +++ b/src/EFCore/Internal/InternalDbSet.cs @@ -520,7 +520,7 @@ IServiceProvider IInfrastructure.Instance /// doing so can result in application failures when updating to a new Entity Framework Core release. /// void IResettableService.ResetState() - => ((IResettableService?)_localView)?.ResetState(); + => _localView?.Reset(); /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to @@ -528,12 +528,11 @@ void IResettableService.ResetState() /// 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. /// - async Task IResettableService.ResetStateAsync(CancellationToken cancellationToken) + Task IResettableService.ResetStateAsync(CancellationToken cancellationToken) { - if (_localView != null) - { - await ((IResettableService)_localView).ResetStateAsync(cancellationToken).ConfigureAwait(false); - } + ((IResettableService)this).ResetState(); + + return Task.CompletedTask; } private EntityEntry EntryWithoutDetectChanges(TEntity entity) diff --git a/test/EFCore.SqlServer.FunctionalTests/DbContextPoolingTest.cs b/test/EFCore.SqlServer.FunctionalTests/DbContextPoolingTest.cs index d06c757eb4b..c4e3449b4a2 100644 --- a/test/EFCore.SqlServer.FunctionalTests/DbContextPoolingTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/DbContextPoolingTest.cs @@ -1,6 +1,9 @@ // 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.ObjectModel; +using System.Collections.Specialized; +using System.ComponentModel; using System.Data; using Microsoft.Data.SqlClient; using Microsoft.EntityFrameworkCore.Internal; @@ -164,6 +167,12 @@ public class Customer { public string CustomerId { get; set; } public string CompanyName { get; set; } + public ObservableCollection Orders { get; } = new(); + } + + public class Order + { + public string OrderId { get; set; } } private interface ISecondContext @@ -770,6 +779,17 @@ public async Task Context_configuration_is_reset(bool useInterface, bool async) var set = context1.Customers; var localView = set.Local; + localView.PropertyChanged += LocalView_OnPropertyChanged; + localView.PropertyChanging += LocalView_OnPropertyChanging; + localView.CollectionChanged += LocalView_OnCollectionChanged; + var customer1 = new Customer { CustomerId = "C" }; + context1.Customers.Attach(customer1); + Assert.Equal(1, localView.Count); + Assert.Same(customer1, localView.ToBindingList().Single()); + Assert.Same(customer1, localView.ToObservableCollection().Single()); + Assert.True(_localView_OnPropertyChanging); + Assert.True(_localView_OnPropertyChanged); + Assert.True(_localView_OnCollectionChanged); context1.ChangeTracker.AutoDetectChangesEnabled = true; context1.ChangeTracker.LazyLoadingEnabled = true; @@ -791,6 +811,10 @@ public async Task Context_configuration_is_reset(bool useInterface, bool async) context1.SavedChanges += Context_OnSavedChanges; context1.SaveChangesFailed += Context_OnSaveChangesFailed; + _localView_OnPropertyChanging = false; + _localView_OnPropertyChanged = false; + _localView_OnCollectionChanged = false; + await Dispose(serviceScope, async); serviceScope = serviceProvider.CreateScope(); @@ -811,9 +835,13 @@ public async Task Context_configuration_is_reset(bool useInterface, bool async) Assert.False(context2.Database.AutoSavepointsEnabled); Assert.Null(context1.Database.GetCommandTimeout()); - var customer = new Customer { CustomerId = "C" }; - context2.Customers.Attach(customer).State = EntityState.Modified; - context2.Customers.Attach(customer).State = EntityState.Unchanged; + Assert.Empty(localView); + Assert.Empty(localView.ToBindingList()); + Assert.Empty(localView.ToObservableCollection()); + + var customer2 = new Customer { CustomerId = "C" }; + context2.Customers.Attach(customer2).State = EntityState.Modified; + context2.Customers.Attach(customer2).State = EntityState.Unchanged; Assert.False(_changeTracker_OnTracking); Assert.False(_changeTracker_OnTracked); @@ -832,6 +860,12 @@ public async Task Context_configuration_is_reset(bool useInterface, bool async) Assert.Same(set, context2!.Customers); Assert.Same(localView, context2!.Customers.Local); + Assert.Equal(1, localView.Count); + Assert.Same(customer2, localView.ToBindingList().Single()); + Assert.Same(customer2, localView.ToObservableCollection().Single()); + Assert.False(_localView_OnPropertyChanging); + Assert.False(_localView_OnPropertyChanged); + Assert.False(_localView_OnCollectionChanged); } [ConditionalTheory] @@ -872,7 +906,19 @@ public async Task Context_configuration_is_reset_with_factory(bool async, bool w var context1 = async ? await factory.CreateDbContextAsync() : factory.CreateDbContext(); var set = context1.Customers; + var localView = set.Local; + localView.PropertyChanged += LocalView_OnPropertyChanged; + localView.PropertyChanging += LocalView_OnPropertyChanging; + localView.CollectionChanged += LocalView_OnCollectionChanged; + var customer1 = new Customer { CustomerId = "C" }; + context1.Customers.Attach(customer1); + Assert.Equal(1, localView.Count); + Assert.Same(customer1, localView.ToBindingList().Single()); + Assert.Same(customer1, localView.ToObservableCollection().Single()); + Assert.True(_localView_OnPropertyChanging); + Assert.True(_localView_OnPropertyChanged); + Assert.True(_localView_OnCollectionChanged); context1.ChangeTracker.AutoDetectChangesEnabled = true; context1.ChangeTracker.LazyLoadingEnabled = true; @@ -893,15 +939,23 @@ public async Task Context_configuration_is_reset_with_factory(bool async, bool w context1.SavedChanges += Context_OnSavedChanges; context1.SaveChangesFailed += Context_OnSaveChangesFailed; + _localView_OnPropertyChanging = false; + _localView_OnPropertyChanged = false; + _localView_OnCollectionChanged = false; + await Dispose(context1, async); var context2 = async ? await factory.CreateDbContextAsync() : factory.CreateDbContext(); Assert.Same(context1, context2); - var customer = new Customer { CustomerId = "C" }; - context2.Customers.Attach(customer).State = EntityState.Modified; - context2.Customers.Attach(customer).State = EntityState.Unchanged; + Assert.Empty(localView); + Assert.Empty(localView.ToBindingList()); + Assert.Empty(localView.ToObservableCollection()); + + var customer2 = new Customer { CustomerId = "C" }; + context2.Customers.Attach(customer2).State = EntityState.Modified; + context2.Customers.Attach(customer2).State = EntityState.Unchanged; Assert.False(_changeTracker_OnTracking); Assert.False(_changeTracker_OnTracked); @@ -920,6 +974,12 @@ public async Task Context_configuration_is_reset_with_factory(bool async, bool w Assert.Same(set, context2!.Customers); Assert.Same(localView, context2!.Customers.Local); + Assert.Equal(1, localView.Count); + Assert.Same(customer2, localView.ToBindingList().Single()); + Assert.Same(customer2, localView.ToObservableCollection().Single()); + Assert.False(_localView_OnPropertyChanging); + Assert.False(_localView_OnPropertyChanged); + Assert.False(_localView_OnCollectionChanged); } [ConditionalFact] @@ -1006,6 +1066,21 @@ private void Context_OnSaveChangesFailed(object sender, SaveChangesFailedEventAr private bool _context_OnSaveChangesFailed; + private bool _localView_OnPropertyChanged; + + private void LocalView_OnPropertyChanged(object sender, PropertyChangedEventArgs e) + => _localView_OnPropertyChanged = true; + + private bool _localView_OnPropertyChanging; + + private void LocalView_OnPropertyChanging(object sender, PropertyChangingEventArgs e) + => _localView_OnPropertyChanging = true; + + private bool _localView_OnCollectionChanged; + + private void LocalView_OnCollectionChanged(object sender, NotifyCollectionChangedEventArgs e) + => _localView_OnCollectionChanged = true; + private bool _changeTracker_OnTracking; private void ChangeTracker_OnTracking(object sender, EntityTrackingEventArgs e)