ArrayMemoryPool.ArrayMemoryPoolBuffer implementation does not dispose if the ref count reaches 0

[davidfowl]

Seems like when we ported this from corefxlab we removed a bunch of functionality. Originally the implementation called Dispose on the reference count = 0:

https://github.com/dotnet/corefxlab/blob/c81145ba362ca6649dcf97ae662869f7005061ee/src/System.Buffers.Primitives/System/Buffers/Pooling/ArrayMemoryPool.cs#L91-L97

Now it does nothing.

https://github.com/dotnet/corefx/blob/master/src/System.Memory/src/System/Buffers/ArrayMemoryPool.ArrayMemoryPoolBuffer.cs#L88

This is making pipelines basically leak memory like crazy when using this pool implementation (which is now the default). Can we clarify on the semantics here ASAP?

/cc @stephentoub @KrzysztofCwalina @ahsonkhan @atsushikan

[ghost]

Seems to me the refcount is about tracking the lifetime of the pinned MemoryHandle and catching cases where you dispose while still using the pinned MemoryHandle, while Dispose is about tracking the lifetime of the underlying storage. If you rent, you're already expected to Dispose or use the using construct whether you create a pinned MemoryHandle or whether you just use it as an array. Saying "don't Dispose" in one case and "do Dispose" in the other doesn't make sense to me.

[davidfowl]

@atsushikan did that change during the port? As you can see that wasn't originally the case but we need to clarify the contract because pipelines is relying on these semantics for correctness. Also our ASP.NET Core SlabMemoryPool implementation does this today https://github.com/aspnet/Common/blob/9d58f3473c558ea67306b0cb09b67c1ccdedf25e/shared/Microsoft.Extensions.Buffers.Sources/MemoryPoolBlock.cs#L102-L112.

[ghost]

Yes, it did change during the port.

[ghost]

Right, let's clarify the contract. I've put in my two cents, but let's see what the api guys think.

[KrzysztofCwalina]

Yeah, ArrayMemoryPoolBuffer is supposed to auto-dispose when refcount drops to zero.

[davidfowl]

I think it's worth having some MemoryPool<T> tests in place that serve as a specification for various implementations. If the semantics are wrong here, there are dire consequences https://github.com/dotnet/corefx/issues/27543#issuecomment-369132611

[KrzysztofCwalina]

@GrabYourPitchforks, once we decide what semantics we want, we will decide who does the work.

[KrzysztofCwalina]

actually, I noticed this is fixed. So just make sure the semantics are what we want.