DirectBufferPool: The buffer is not properly freed in the Release function

[jjmSE]

Version Information
Version of Akka.NET? 1.4.25
Which Akka.NET Modules? Akka.IO

Describe the bug
The DirectBufferPool class throws an exception because the buffer is not properly freed in the Release function.
(https://github.com/akkadotnet/akka.net/blob/master/src/core/Akka/IO/Buffers/DirectBufferPool.cs)

Expected behavior
The buffer should be released.

Actual behavior
The buffer is not released and new data is added using the Push function (see screenshots).

Screenshots

Environment
Windows
.NET Framework 4.8

[Arkatufus]

The implementation is correct, its a pool, the released buffer is returned to the pool of buffer by pushing it.
The bug happens somewhere else.

[Aaronontheweb]

@Arkatufus where's the implementation code for Push?

[Arkatufus]

_buffers is a System.Collections.Concurrent.ConcurrentStack instance.

The problem happened precisely because the stack _buffers is empty, and the pool ran out of counters, some socket kept renting more memories without calling Release

[Aaronontheweb]

Got it.

[Arkatufus]

There is a big difference between the C# and JVM implementation of DirectBufferPool.

In JVM, DirectByteBufferPool is a simple object pool intended as a reference holder to a fixed maximum number on ByteBuffer objects. Its purpose is to prevent the JVM from recreating and garbage collecting ByteBuffer instances over and over again to optimize the speed of Java NIO. It does not care if some code tried to rent more memory or tried to release an instance when it is full, it just creates a new instance or let the system GC any overflows.

In C#, we use DirectBufferPool to optimize how memory is allocated and limits the maximum amount of memory of each pool can use. It is actually holding a reference to all the memory it allocated and can be a cause for memory leak if rented buffers are not released, which is what I think happened on this bug report, something is not releasing their byte buffer reference back to the pool correctly.

We could probably fix this by removing the memory optimization part of the buffer and just hands out standalone array of bytes instead of trying to hand manage optimized blocks of memory.

[Aaronontheweb]

We do have a disabled buffer pool that turns this functionality off - the idea is still good, reducing unnecessary allocations, but we need to find where we aren't returning the memory. My guess is that the failure path is where I would look.

[Arkatufus]

When I say turning off the memory optimization, I ment the memory block allocation style; we don't have to hold any reference to the allocated memory block that way. We can still do pooling to minimize the cost of memory allocation and GC, they just wont be in contiguous memory blocks.

[Aaronontheweb]

When I say turning off the memory optimization, I ment the memory block allocation style; we don't have to hold any reference to the allocated memory block that way. We can still do pooling to minimize the cost of memory allocation and GC, they just wont be in contiguous memory blocks.

Got it.

[Arkatufus]

Maybe I should create a new class without the memory optimization, lets call it SimpleBufferPool, for backward binary compatibility?

[Aaronontheweb]

I think this already does that @Arkatufus https://github.com/akkadotnet/akka.net/blob/master/src/core/Akka/IO/Buffers/DisabledBufferPool.cs

[Arkatufus]

The DisabledBufferPool does not do any instance pooling, each ArraySegment is created on the fly on rent and left for the GC to collect on release.

[Aaronontheweb]

Ah I see. I wonder if we can use any of the built-in memory pool types to handle this? I think they ship as separate BCL implementations if you're not using a newer .NET runtime.

[Arkatufus]

I'm reading up on it, that would need some code changes though. I'll try to make another PR for it.

[ismaelhamed]

Would using the DisabledBufferPool by default too in Udp help? Actually, it'd be great to know the recommended use cases for DirectBufferPool, DisabledBufferPool, and now the SimpleBufferPool

[Arkatufus]

@ismaelhamed SimpleBufferPool is still in draft, it might not actually be merged in, its just an idea for a bullet proof simple memory pool.
There's a strong suggestion that memory was not being released properly for DirectBufferPool so it shouldn't be used for now.
I'm making a short term patch to make DisabledBufferPool as the default buffer pool for now.