Skip to content

Commit

Permalink
Fix for Memory Leak (#660)
Browse files Browse the repository at this point in the history
* Fix for Memory Leak

#659

* Update MemoryLeakTests.cs

* Remove ReadOnlyCollectionPooled

Use ReadOnlyDisposableCollection
  • Loading branch information
ChrisPulman authored May 11, 2024
1 parent efe8b22 commit 3834d5e
Show file tree
Hide file tree
Showing 10 changed files with 102 additions and 168 deletions.
1 change: 0 additions & 1 deletion samples/Directory.build.props
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
<PropertyGroup>
<Nullable>enable</Nullable>
<AvaloniaVersion>11.0.10</AvaloniaVersion>
<CodeAnalysisRuleSet>$(MSBuildThisFileDirectory)..\src\analyzers.ruleset</CodeAnalysisRuleSet>
<WarningsAsErrors>CS8600;CS8602;CS8603;CS8604;CS8605;CS8606;CS8607;CS8608;CS8609;CS8610;CS8611;CS8612;CS8613;CS8614;CS8615;CS8616;CS8617;CS8618;CS8619;CS8620;CS8621;CS8622;CS8623;CS8624;CS8625</WarningsAsErrors>
</PropertyGroup>
<ItemGroup>
Expand Down
26 changes: 13 additions & 13 deletions src/Directory.build.props
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
<LangVersion>preview</LangVersion>
<EnableNETAnalyzers>True</EnableNETAnalyzers>
<AnalysisLevel>latest</AnalysisLevel>
<NoWarn>$(NoWarn);IDE1006</NoWarn>
</PropertyGroup>

<ItemGroup>
Expand All @@ -35,17 +36,16 @@
<None Include="$(MSBuildThisFileDirectory)..\README.md" Pack="true" PackagePath="\" />
</ItemGroup>

<ItemGroup Condition="'$(IsTestProject)' != 'true'">
<PackageReference Include="Microsoft.SourceLink.GitHub" Version="8.0.0" PrivateAssets="All" />
</ItemGroup>
<ItemGroup>
<PackageReference Include="Nerdbank.GitVersioning" Version="3.6.133" PrivateAssets="all" />
<PackageReference Include="stylecop.analyzers" Version="1.2.0-beta.556" PrivateAssets="all" />
<PackageReference Include="Roslynator.Analyzers" Version="4.12.2" PrivateAssets="All" />
</ItemGroup>
<ItemGroup>
<AdditionalFiles Include="$(MSBuildThisFileDirectory)stylecop.json" Link="stylecop.json" />
</ItemGroup>
<ItemGroup Condition="'$(IsTestProject)' != 'true'">
<PackageReference Include="Microsoft.SourceLink.GitHub" Version="8.0.0" PrivateAssets="All" />
</ItemGroup>
<ItemGroup>
<PackageReference Include="Nerdbank.GitVersioning" Version="3.6.133" PrivateAssets="all" />
<PackageReference Include="stylecop.analyzers" Version="1.2.0-beta.556" PrivateAssets="all" />
<PackageReference Include="Roslynator.Analyzers" Version="4.12.2" PrivateAssets="All" />
</ItemGroup>
<ItemGroup>
<AdditionalFiles Include="$(MSBuildThisFileDirectory)stylecop.json" Link="stylecop.json" />
</ItemGroup>

</Project>

</Project>
18 changes: 5 additions & 13 deletions src/ReactiveUI.Validation.Tests/MemoryLeakTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -37,28 +37,20 @@ public void Instance_Released_IsGarbageCollected()
new Action(
() =>
{
var sut = new TestClassMemory();
var memTest = new TestClassMemory();

reference = new WeakReference(sut, true);
sut.Dispose();
reference = new WeakReference(memTest, true);
memTest.Dispose();
})();

// Sut should have gone out of scope about now, so the garbage collector can clean it up
// memTest should have gone out of scope about now, so the garbage collector can clean it up
dotMemory.Check(
memory => memory.GetObjects(
where => where.Type.Is<TestClassMemory>()).ObjectsCount.Should().Be(0, "it is out of scope"));

GC.Collect();
GC.WaitForPendingFinalizers();

if (reference.Target is TestClassMemory sut)
{
// ReactiveObject does not inherit from IDisposable, so we need to check ValidationContext
sut.ValidationContext.Should().BeNull("it is garbage collected");
}
else
{
reference.Target.Should().BeNull("it is garbage collected");
}
reference.IsAlive.Should().BeFalse("it is garbage collected");
}
}
2 changes: 1 addition & 1 deletion src/ReactiveUI.Validation.Tests/Models/TestClassMemory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public TestClassMemory()
.DisposeWith(_disposable);

// commenting out the following statement makes the test green
ValidationContext.ValidationStatusChange
ValidationContext?.ValidationStatusChange
.Subscribe(/* you do something here, but this does not matter for now. */)
.DisposeWith(_disposable);
}
Expand Down
5 changes: 3 additions & 2 deletions src/ReactiveUI.Validation.Tests/ValidationContextTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -125,14 +125,15 @@ public void IsValidShouldNotifyOfValidityChange()
viewModel.ValidationContext.Add(nameValidation);

var latestValidity = false;
viewModel.IsValid().Subscribe(isValid => latestValidity = isValid);
var d = viewModel.IsValid().Subscribe(isValid => latestValidity = isValid);
Assert.False(latestValidity);

viewModel.Name = "Jonathan";
Assert.True(latestValidity);

viewModel.Name = string.Empty;
Assert.False(latestValidity);
d.Dispose();
}

/// <summary>
Expand Down Expand Up @@ -220,4 +221,4 @@ public void ShouldClearAttachedValidationRulesForTheGivenProperty()
Assert.NotEmpty(viewModel.ValidationContext.Text);
Assert.Equal(name2ErrorMessage, viewModel.ValidationContext.Text.ToSingleLine());
}
}
}
112 changes: 0 additions & 112 deletions src/ReactiveUI.Validation/Collections/ReadOnlyCollectionPooled.cs

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
// Copyright (c) 2024 .NET Foundation and Contributors. All rights reserved.
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for full license information.

using System;
using System.Collections;
using System.Collections.Generic;
using System.Collections.Immutable;

namespace ReactiveUI.Validation.Collections;

internal sealed class ReadOnlyDisposableCollection<T>(IEnumerable<T> items) : IReadOnlyCollection<T>, IDisposable
{
private readonly ImmutableList<T> _immutableList = ImmutableList.CreateRange(items);
private bool _disposedValue;

/// <summary>
/// Gets the number of elements in the collection.
/// </summary>
public int Count => _immutableList.Count;

/// <summary>
/// Returns an enumerator that iterates through the collection.
/// </summary>
/// <returns>
/// An enumerator that can be used to iterate through the collection.
/// </returns>
public IEnumerator<T> GetEnumerator() => _immutableList.GetEnumerator();

/// <summary>
/// Returns an enumerator that iterates through a collection.
/// </summary>
/// <returns>
/// An <see cref="T:System.Collections.IEnumerator"></see> object that can be used to iterate through the collection.
/// </returns>
IEnumerator IEnumerable.GetEnumerator() => _immutableList.GetEnumerator();

/// <summary>
/// Performs application-defined tasks associated with freeing, releasing, or resetting unmanaged resources.
/// </summary>
public void Dispose()
{
Dispose(disposing: true);
GC.SuppressFinalize(this);
}

private void Dispose(bool disposing)
{
if (!_disposedValue)
{
if (disposing)
{
_immutableList.Clear();
}

_disposedValue = true;
}
}
}
27 changes: 10 additions & 17 deletions src/ReactiveUI.Validation/Contexts/ValidationContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
using System;
using System.Buffers;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Reactive.Concurrency;
using System.Reactive.Disposables;
Expand All @@ -31,15 +30,16 @@ namespace ReactiveUI.Validation.Contexts;
/// </remarks>
public class ValidationContext : ReactiveObject, IValidationContext
{
private readonly CompositeDisposable _disposables = [];

private readonly ReplaySubject<IValidationState> _validationStatusChange = new(1);
private readonly ReplaySubject<bool> _validSubject = new(1);

private readonly IConnectableObservable<bool> _validationConnectable;
private readonly IObservable<bool> _validationObservable;
private readonly ObservableAsPropertyHelper<IValidationText> _validationText;
private readonly ObservableAsPropertyHelper<bool> _isValid;

private readonly CompositeDisposable _disposables = [];
private SourceList<IValidationComponent> _validationSource = new();
private readonly SourceList<IValidationComponent> _validationSource = new();
private bool _isActive;

/// <summary>
Expand All @@ -52,15 +52,14 @@ public ValidationContext(IScheduler? scheduler = null)
var changeSets = _validationSource.Connect().ObserveOn(scheduler);
Validations = changeSets.AsObservableList();

_validationConnectable = changeSets
_validationObservable = changeSets
.StartWithEmpty()
.AutoRefreshOnObservable(x => x.ValidationStatusChange)
.QueryWhenChanged(static x =>
{
using ReadOnlyCollectionPooled<IValidationComponent> validationComponents = new(x);
using ReadOnlyDisposableCollection<IValidationComponent> validationComponents = new(x);
return validationComponents.Count is 0 || validationComponents.All(v => v.IsValid);
})
.Multicast(_validSubject);
});

_isValid = _validSubject
.StartWith(true)
Expand Down Expand Up @@ -95,7 +94,7 @@ public IObservable<bool> Valid
/// <summary>
/// Gets get the list of validations.
/// </summary>
public IObservableList<IValidationComponent> Validations { get; private set; }
public IObservableList<IValidationComponent> Validations { get; }

/// <inheritdoc/>
public bool IsValid
Expand Down Expand Up @@ -162,10 +161,7 @@ public IValidationText Text
/// <inheritdoc/>
public void Dispose()
{
// Dispose of unmanaged resources.
Dispose(true);

// Suppress finalization.
GC.SuppressFinalize(this);
}

Expand All @@ -183,11 +179,8 @@ protected virtual void Dispose(bool disposing)
_validationStatusChange.Dispose();
_validSubject.Dispose();
_validationSource.Clear();
Validations.Dispose();
_validationSource.Dispose();

Validations = null!;
_validationSource = null!;
Validations.Dispose();
}
}

Expand All @@ -199,7 +192,7 @@ private void Activate()
}

_isActive = true;
_disposables.Add(_validationConnectable.Connect());
_disposables.Add(_validationObservable.Subscribe(_validSubject));
}

/// <summary>
Expand Down
Loading

0 comments on commit 3834d5e

Please sign in to comment.