Runtime: Add an option to return finalizable versions of ArrayMemoryPoolBuffer from MemoryPool

[KrzysztofCwalina]

[stephentoub]

Ouch 😦 So every rental is not only an allocation, but an allocation of a finalizable object?

[davidfowl]

Better rent big buffers 😬

[jkotas]

@KrzysztofCwalina Could you please explain motivation for this? What is the problem that it is trying to solve?

[davidfowl]

It's the oops, we have a bug in our code, please don't leak the memory. We do the same in kestrel today with our pool. The blocks have a finalizer (we pool them though).

[jkotas]

These are managed arrays. The memory won't leak. They will be collected by GC.

You have to do that in kestrel because of you are using unmanaged memory.

[davidfowl]

These are managed arrays. The memory won't leak. They will be collected by GC.

You never have to return arrays to the array pool? What happens if you a bunch of components do that?

You have to do that in kestrel because of you are using unmanaged memory.

Kestrel is using managed memory, it's just pinned.

[jkotas]

You never have to return arrays to the array pool? What happens if you a bunch of components do that?

The pool will be allocating a fresh arrays some of the time. If everybody does that, then the system will behave as if there was no array pool.

Kestrel is using managed memory, it's just pinned.

Pinning turns it into unmanaged memory for the purpose of this discussion.

[KrzysztofCwalina]

I was talking to a customer and they asked if we finalize the buffers. I told them that yes (as I though that's what we did when the code was in corfxlab), but then I noticed we don't in corefx.

I am pretty sure the motivation for this is to make sure pool is actually a pool (and not a glorified new T[]) in the light of bugs.

But I totally agree there is a trade off here. If we think the cost is not worth it, we should close this issue.

Too bad we don't have an easy and cheap way to make it into a debug only feature.

[jkotas]

Too bad we don't have an easy and cheap way to make it into a debug only feature.

ArrayMemoryPoolBuffer is internal, so you can create an internal variant of it with finalizers that will be used when you turn on debugging diagnostic. But I do not think that this debugging diagnostic should be on by default.

We do have tracing for GC and ArrayPool. If you get a problem with leaking pooled arrays in production, you should be able to pinpoint it using those. The question is whether we need more diagnostic aids for the manual memory management than that, and what they should look like.

[KrzysztofCwalina]

Ah, good point, we could just have a different subclass of the buffer. Changing title of the issue :-)

Can you point me to a writeup about the ArrayPool diagnostics?

[jkotas]

Can you point me to a writeup about the ArrayPool diagnostics?

It was added by dotnet/corefx#5231 . That's all I know about it.

[ghost]

@KrzysztofCwalina - can you be more specific about this option? How would it be specified (as a new overload of Rent on MemoryPool? An options parameter passed to Rent?) What would the finalizer do? (If it disposes the owned memory, it's not a "diagnostic mode" - it's a failsafe. The app that specified this option would be saying "I'm a buggy app. Please clean up after me. I understand this will make the pool more resource hungry.")

[ghost]

Past the cut off for api changes (especially ones that haven't even been concretely proposed yet.)

[stephentoub]

A potentially much bigger problem than the cost are the issues @GrabYourPitchforks has previously highlighted, e.g.
C# Span<byte> s = MemoryPool<byte>.Shared.Rent(42); Use(s);
At this point, if the IMemoryOwner<byte> returned from rent were finalizable to return the instance back to the pool, it could end up getting returned while we're still using the span, since the span is only keeping the underlying array alive, not the finalizable object wrapping the array.

Those issues have led to guidelines that we should basically never have finalizers on such types. Given that, I'm going to close this. Please feel free to re-open if there's something else this issue is tracking.