Added missing clearing of pooled arrays

[hendriklhf]

I found a few places where arrays were returned to the pool but not cleared, which would undesirably keep remaining objects in the array alive.
The check for typeof(T).IsPrimitive is of course not 100% correct, but, if I'm not mistaken, T is never a non-primitive unmanaged type in all current usages.

[Copilot]

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

[Copilot]

The fallback check using !typeof(T).IsPrimitive may not accurately determine if T contains references for value types. Please add a clarifying comment that explains this assumption, given that in current usages T is always a primitive unmanaged type.

[Copilot]

Similar to the Dispose method, the Grow method uses the !typeof(T).IsPrimitive check as a fallback. Consider adding a comment here to clarify that this condition is acceptable because T is ensured to be a primitive unmanaged type in all current contexts.

[hendriklhf]

@dotnet-policy-service agree

[stephentoub]

@tarekgh, the comment on this method says:
"Private helper to copy IEnumerable to array. We have it to avoid adding dependencies on System.Linq"
Why don't we want to just use LINQ's ToArray here? We're concerned about loading System.Linq.dll?

[tarekgh]

Yes, IIRC long ago some partners were developing micro services and worked hard to reduce any new dependencies. We initially added this dependency on Linq and then later removed it because of that.

Cc @noahfalk

[stephentoub]

Do we know if that's still relevant? If you're in this part of the code, you're dealing with an IEnumerable that's not a collection. I'd be a bit surprised if such consuming code was part of an application that wasn't already using System.Linq.dll somewhere. (It's also not a separate dependency requiring distribution with the app, as it's part of any .NET that would be targeted.)

[tarekgh]

We can try to change it and look if we see any complaints again.

@noahfalk let us know if you have more context or concern depending on Linq.

[tannergooding]

Should we rather be ensuring that ArrayPool itself clears the returned array for any RuntimeHelpers.IsReferenceOrContainsReferences<T>(), much like we do for List<T> and other scenarios?

This seems more like a general issue with potentially keeping objects alive and not something that should be handled per callsite prior to the return.

[stephentoub]

Should we rather be ensuring that ArrayPool itself clears the returned array

It doesn't know how much to clear; it would have to clear the whole thing, which is what it does in Return when the clearArray parameter is true.

[tannergooding]

It doesn't know how much to clear; it would have to clear the whole thing

Right, but it seems like that's better/cheaper than potentially holding onto other references for an unknown amount of time.

It really seems like there should be an analyzer and/or some overload of Return that makes this significantly more convenient and less problematic.

For example, some void Return(T[] array, int lengthToClear) + an analyzer for a non unmanaged type T could help catch issues like this PR is fixing while reducing code duplication.

[tarekgh]

It seems to me @tannergooding suggestion is a better solution here to avoid the callers of the ArrayPool clearing the list before returning it. @hendriklhf can you try it? and we can open tracking issue for having an analyzer for that.

[tarekgh]

Additionally, @hendriklhf if you interested to apply the change having System.Diagnostics.DiagnosticSource take dependency on Linq and then remove the code of Measurement<T>.ToArray() replacing the calling sites with the Linq.ToArray? Please let me know if you can't do so I can help with it.

CC @noahfalk

[hendriklhf]

Additionally, @hendriklhf if you interested to apply the change having System.Diagnostics.DiagnosticSource take dependency on Linq and then remove the code of Measurement<T>.ToArray() replacing the calling sites with the Linq.ToArray?

Done 👍

It seems to me @tannergooding suggestion is a better solution here to avoid the callers of the ArrayPool clearing the list before returning it. @hendriklhf can you try it? and we can open tracking issue for having an analyzer for that.

Yes, added the Return(T[], int) overload on ArrayPool as an internal for that.

[tarekgh]

LGTM. Thanks!

[hendriklhf]

Now that I think about it, it might make more sense to swap which Return calls which, so there is no new place introduced where the array is cleared and is also only cleared if it can be returned. Would just require overriding the new Return in the two inheriting types and moving the return logic to it. But I can also leave it as it is, whatever you prefer.
But if it will become public with an analyzer, couldn't be abstract, as it would be breaking, right? So might have to stay as it is right now. 🤔

public override void Return(T[] array, bool clearArray = false)
{
    ArgumentNullException.ThrowIfNull(array);
    Return(array, clearArray ? array.Length : 0);
}

internal override void Return(T[] array, int lengthToClear)
{
    ...
}

[tarekgh]

@hendriklhf Instead of introducing a new internal method internal override void Return(T[] array, int lengthToClear) within the ArrayPool class, I'm considering implementing it as an extension helper method instead. This approach would enable its use in libraries targeting down-level platforms (such as .NET Standard 2.0 and .NET Framework). Eventually we may consider a public method in ArrayPool.

[tarekgh]

I am merging this PR. We can consider moving Return(T[] array, int lengthToClear) to an extension method if we see a need to use it in the down-level targeting code.