Skip to content

Conversation

hendriklhf
Copy link

Add missing clearing of pooled arrays

I found several places where arrays rented from pools are not cleared before returning. Any objects in the pools' arrays cannot be collected by the GC. This might keep objects in the returned arrays longer or, in the worst case, forever alive.

  • If the amount of elements written to the array is known, the array is sliced so that only the written portion is cleared, avoiding unnecessary work.
  • If it's a generic array, the array is only cleared if T is a reference type or contains references.
  • There's one case where the file is also compiled for .NET Framework. RuntimeHelpers.IsReferenceOrContainsReferences<T>() is not available there, so typeof(T).IsPrimitive is checked instead.

@Copilot Copilot AI review requested due to automatic review settings September 17, 2025 20:12
@github-actions github-actions bot added the area-blazor Includes: Blazor, Razor Components label Sep 17, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses memory leaks by adding proper clearing of pooled arrays before returning them to array pools. When arrays containing references are returned to pools without clearing, the referenced objects cannot be garbage collected, potentially causing memory leaks.

Key changes:

  • Adds conditional clearing logic that only clears arrays when they contain reference types or references
  • Uses RuntimeHelpers.IsReferenceOrContainsReferences<T>() on .NET and falls back to typeof(T).IsPrimitive check for .NET Framework
  • Optimizes clearing by only clearing the used portion of arrays when the count is known

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/SignalR/common/Shared/JsonUtils.cs Adds clearing logic with .NET Framework compatibility for generic array pool wrapper
src/Shared/ValueStringBuilder/ValueListBuilder.cs Clears used portion of arrays in Dispose and Grow methods
src/Mvc/Mvc.NewtonsoftJson/src/JsonArrayPool.cs Adds clearing for JSON array pool implementation
src/Middleware/OutputCaching/src/RecyclableArrayBufferWriter.cs Clears arrays in Dispose and resize operations
src/Middleware/OutputCaching/src/OutputCacheEntryFormatter.cs Adds explicit clearing for string array when tags exist
src/Components/Endpoints/src/FormMapping/PrefixResolver.cs Clears FormKey array before returning to pool
src/Components/Endpoints/src/FormMapping/Converters/CollectionAdapters/ArrayPoolBufferAdapter.cs Clears arrays in buffer operations and result conversion
src/Components/Components/src/NavigationManager.cs Clears function delegate array after use

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Sep 17, 2025
@JamesNK
Copy link
Member

JamesNK commented Sep 19, 2025

ArrayPool has a return method with an option for clearing arrays. Why not use it instead of clearing before returning?

https://learn.microsoft.com/en-us/dotnet/api/system.buffers.arraypool-1.return?view=net-9.0

@gfoidl
Copy link
Member

gfoidl commented Sep 19, 2025

The array pool method would clear the whole array, regardless of how many items are actually non-zero thus potentially more work than needed.
On overload that specifies the length to clear is what you aim for?

With the approach here just the elements are cleared, that are used.

But instead doing this on every use, maybe create a helper function for this like the one linked above?
If .NET exposes the API public, then it's easy to switch over w/o the need to touch lots of places over the code base.

@hendriklhf
Copy link
Author

Yes, that's why. You generally don't want or need to clear the whole array. 🙂

@hendriklhf
Copy link
Author

Added the Return(T[], int) extension as used in dotnet/runtime 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components 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.

3 participants