Skip to content
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.

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

Fix memory leak in MemoryCache #92550

Merged
merged 3 commits into from
Oct 16, 2023
Merged

Fix memory leak in MemoryCache #92550

merged 3 commits into from
Oct 16, 2023

Conversation

verdie-g
Copy link
Contributor

@verdie-g verdie-g commented Sep 24, 2023

WeakReference<T>.TryGetTarget apparently returns false in the target object's finalizer. Here is an example demonstrating that:

CreateTest();
GC.Collect(GC.MaxGeneration);
GC.WaitForPendingFinalizers();

void CreateTest() => _ = new Test();

class Test
{
    private readonly WeakReference<Test> _wr;

    public Test()
    {
        _wr = new WeakReference<Test>(this);
        Console.WriteLine(_wr.TryGetTarget(out _)); // True
    }

    ~Test()
    {
        Console.WriteLine(_wr.TryGetTarget(out _)); // False
    }
}

In MemoryCache that would cause the Stats in the _allStats list to never be cleaned out.

MemoryCache c = new(new MemoryCacheOptions { TrackStatistics = true });
for (int i = 0; i < 100; i += 1)
{
    CreateThread();
}

for (int i = 0; i < 10; i += 1)
{
    Thread.Sleep(100);
    GC.Collect(GC.MaxGeneration);
    GC.WaitForPendingFinalizers();
}

Console.WriteLine(c.GetCurrentStatistics());

void CreateThread()
{
    Thread t = new(() => { c.Get(""); });
    t.Start();
    t.Join();
}

At the end of this program, c._allStats.Count == 100.

`WeakReference<T>.TryGetTarget` apparently returns false in the target object's finalizer. Here is an example demonstrating that:
```cs
CreateTest();
GC.Collect(GC.MaxGeneration);
GC.WaitForPendingFinalizers();

void CreateTest() => _ = new Test();

class Test
{
    private readonly WeakReference<Test> _wr;

    public Test()
    {
        _wr = new WeakReference<Test>(this);
        Console.WriteLine(_wr.TryGetTarget(out _)); // True
    }

    ~Test()
    {
        Console.WriteLine(_wr.TryGetTarget(out _)); // False
    }
}
```

In MemoryCache that would cause the Stats in the _allStats list to never be cleaned out.
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Sep 24, 2023
@ghost
Copy link

ghost commented Sep 24, 2023

Tagging subscribers to this area: @dotnet/area-extensions-caching
See info in area-owners.md if you want to be subscribed.

Issue Details

WeakReference<T>.TryGetTarget apparently returns false in the target object's finalizer. Here is an example demonstrating that:

CreateTest();
GC.Collect(GC.MaxGeneration);
GC.WaitForPendingFinalizers();

void CreateTest() => _ = new Test();

class Test
{
    private readonly WeakReference<Test> _wr;

    public Test()
    {
        _wr = new WeakReference<Test>(this);
        Console.WriteLine(_wr.TryGetTarget(out _)); // True
    }

    ~Test()
    {
        Console.WriteLine(_wr.TryGetTarget(out _)); // False
    }
}

In MemoryCache that would cause the Stats in the _allStats list to never be cleaned out.

Author: verdie-g
Assignees: -
Labels:

area-Extensions-Caching, community-contribution

Milestone: -

Copy link
Member

@jozkee jozkee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you see any downsides to use this aggressive approach on RemoveFromStats?

@verdie-g
Copy link
Contributor Author

Do you see any downsides to use this aggressive approach on RemoveFromStats?

It makes the intention a little less clear imo but I don't see any important downsides.

Copy link
Member

@jozkee jozkee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @verdie-g.

@jozkee jozkee merged commit 9da62fa into dotnet:main Oct 16, 2023
104 of 109 checks passed
@adamsitnik adamsitnik added this to the 9.0.0 milestone Oct 17, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Nov 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Caching community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants