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

ArrayPool clearing not working with many tasks #111090

Closed
ssambi opened this issue Jan 4, 2025 · 2 comments
Closed

ArrayPool clearing not working with many tasks #111090

ssambi opened this issue Jan 4, 2025 · 2 comments
Labels
needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners

Comments

@ssambi
Copy link

ssambi commented Jan 4, 2025

Description

With a high number of tasks, ArrayPool.Shared.Return(), with the clearArray argument set to true, fails to clear the array

Reproduction Steps

Unit test:

 [Fact]
 public async Task Test()
 {
     const int TasksCount = 100000;
     const int ArrayLength = 10000;
     List<Task> tasks = new List<Task>();
     for (int i = 0; i < TasksCount; i++)
     {
         tasks.Add(Task.Run(() =>
         {
             int[] a = ArrayPool<int>.Shared.Rent(ArrayLength);
             Assert.True(a.Take(ArrayLength).All(e => e == 0));

             for (int j = 0; j < ArrayLength; j++)
             {
                 a[j] = 1;
             }
             Assert.True(a.Take(ArrayLength).All(e => e == 1));

             ArrayPool<int>.Shared.Return(a, true);
         }));
     }
     await Task.WhenAll(tasks);
 }

Expected behavior

It's expected that the following assert, that checks that the rented array is cleared, always succeeds:

Assert.True(a.Take(ArrayLength).All(e => e == 0));

Actual behavior

The assert sometimes fails

Regression?

No response

Known Workarounds

No response

Configuration

SDK .net 9.0.101
Windows 10 22H2

Other information

Maybe the cause is that ArrayPool.Shared is thread-safe, since the shared implementation stores the array instances in a ThreadStatic structure. But it's not "await/task safe" as a task could switch the thread it's executing on.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jan 4, 2025
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jan 4, 2025
@huoyaoyuan
Copy link
Member

The shared array pool is used by a lot of components, including LINQ. Other components can return uncleared array into the pool and get rented by your code.

Code shouldn't depend on existing content of rented array. In the proposed stress debug mode, the pool will always fill the array with garbage.

@stephentoub
Copy link
Member

The shared array pool is used by a lot of components, including LINQ. Other components can return uncleared array into the pool and get rented by your code.

Code shouldn't depend on existing content of rented array. In the proposed stress debug mode, the pool will always fill the array with garbage.

It also currently uses GC.AllocateUninitializedArray when a new array is needed:

buffer = GC.AllocateUninitializedArray<T>(minimumLength);

@stephentoub stephentoub closed this as not planned Won't fix, can't repro, duplicate, stale Jan 4, 2025
@dotnet-policy-service dotnet-policy-service bot removed the untriaged New issue has not been triaged by the area owner label Jan 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners
Projects
None yet
Development

No branches or pull requests

3 participants