Runtime: Simpler pattern for array rent/return

[fgreinacher]

Currently it seems like the safe way to use an array from an ArrayPool<T> is something like this:

var pool = ArrayPool<byte>.Shared;
var buffer = pool.Rent(4096);
try
{
    Use(buffer);
}
finally
{
    pool.Return(buffer);
}

Why not make it a bit easier and offer some convenience layer on top of the current API?

var pool = ArrayPool<byte>.Shared;
using (pool.Rent(4096, out var buffer)) 
{
    Use(buffer);
}

If the thing returned by this new Rent method is a struct I don't think it'll have any relevant impact on performance/allocations.

Is it worth writing an formal API proposal for this?

[Clockwork-Muse]

...I'd be extremely cautious about an API like that: the actual item being disposed isn't the real resource being released, which is an independent reference. I suspect you'd get _more_ leaks that way, not less. You'd have to get the item from whatever was returned from Rent (and probably pass that down, instead of the raw array).

This would prevent swapping out buffers as well, since changing the instance referenced in the using is impossible - if the buffer needs to grow you're out of luck (or have to switch back to the old way of doing things).

[fgreinacher]

Understood - but is the situation really different when using Rent/Return directly?

[Clockwork-Muse]

Sure - with the normal ’Rent’/’Return’ you have one item to manage. The current proposal has two, one of which is _implicitly_ (not obviously) disposed. That's a recipe for disaster at some point. You'd be better off with a combined object instead.

The "switch out buffer" bit is only partially an issue - it's probably not a good idea normally, just because you might lose track of a buffer somewhere. So that may be a point in favor of this. But things like cloning-and-growing the buffer gets a bit difficult.

[jnm2]

I heartily disagree with @Clockwork-Muse. I use this pattern all the time for reference counting.

With the Resolve/Release typed factory pattern, I'll have:

public interface IFactory<T>
{
    T Resolve();
    void Release(T instance);
}

public static class FactoryExtensions
{
    // It's important that release is not called twice.
    // A struct does not provide such a guarantee and there's no point in wrapping 
    // an allocated class instance with a struct.
    public static IDisposable Lease<T>(this IFactory<T> factory, out T instance)
    {
        if (factory == null) throw new ArgumentNullException(nameof(factory));
        instance = factory.Resolve();
        return new FactoryLease<T>(factory, instance);
    }

    private sealed class FactoryLease<T> : IDisposable
    {
        private IFactory<T> factory;
        private readonly T instance;

        public FactoryLease(IFactory<T> factory, T instance)
        {
            this.factory = factory;
            this.instance = instance;
        }

        public void Dispose() => Interlocked.Exchange(ref factory, null)?.Release(instance);
    }
}

Usage:

public void DoThing(IFactory<DbContext> dbContextFactory)
{
    using (dbContextFactory.Lease(out var dbContext))
    {
        // Operation 1
    }

    using (dbContextFactory.Lease(out var dbContext))
    {
        // Operation 2
    }
}

[jnm2]

As for passing a lease around, rather than the tuple (IDisposable, T[]), it's easy enough to make FactoryLease<T> public and expose an .Instance property.

[fgreinacher]

My primary reason for not using the returned IDisposable to expose the array is overload resolution. If we returned a lease from the extension method, we couldn't call it Rent because the instance T[] Rent(int) would always win.

What do you think about something like this:

public static class PooledArray
{
    public static PooledArray<T> RentFrom(ArrayPool<T> pool, int minimumLength)
    {
        var data = pool.Rent(minimumLength);
        return new PooledArray(data, pool);
   }
}

public class PooledArray<T> : IDisposable
{
    public T[] Data { get; }
    public ArrayPool<T> Pool { get; }

    public PooledArray(T[] data, ArrayPool<T> pool)
    {  
        Data = data;
        Pool = pool;
    }

   public void Return() => Pool.Return(Data);

   public void Dispose() => Release();
}

Usage:

var pool = ArrayPool<byte>.Shared;
using (var pooledArray = PooledArray.RentFrom(pool, 4096)) 
{
    Use(pooledArray.Data);
}

[safern]

I don't think ArrayPool should be Disposable as the intention of these API is something similar to C malloc/free functions where you rent a space of memory and you return it yourself when you're done using it, it is an Unsafe API where you need to rent the piece of memory that you need and return it when you're done specifically to avoid allocations and the GC handling those objects for you. So I don't think making it Disposable "would make it easier to use", but it would change the whole purpose of this API.

cc: @jkotas

[fgreinacher]

Sorry, but I don’t get how IDisposable has anything to do with the GC. In this case it is just meant as a more conventient (and partially more safe) way to ensure that your arrays are returned to the pool when you’re done with them.

[safern]

Sorry, I missed use the concept of the GC. My point here is that using the IDisposable pattern is to free unmanaged resources, i.e native handles before the GC destructs the object which will just free managed resources. ArrayPool is a pure concept of Rent/Return an array from a shared instance pool, it doesn't really involve freeing native handles or disposing other objects within its class. What happen if a developer doesn't return the rented array? The GC will collect it afterwards, it would be better to return it yourself, yes, but it will not leave dangling handles if you don't do it. So I think IDisposable is not a good fit here.

Also the way that this is implemented doesn't allow us to implement this pattern since the idea is that you can Rent multiple different buffers at the same time from the same Pool or SharedInstance, when you rent it we just return that array and move our index within the pool up and we loose track of what buffer was rented from whom. Imagine you have multiple threads renting different buffers, with the disposable pattern, when Dispose is called by the runtime once you exit your using statement how would we know which is the buffer from our pool that you're done using? That is why the Return parameter receives a T[] so that we actually get the array that is returned from our pool and store it again to be able to use by another process/instance.

Also from this proposal what I understand is like you want would be a new Class/Struct that implements IDisposable and stores the array instance and when dispose is called just returns it? Something like a wrapper around ArrayPool, i.e DisposableArrayPool<T> : ArrayPool<T>, IDisposable?

[jkotas]

There may different kind of wrappers one can build around arrays returned by ArrayPool. The recently introduced MemoryPool is the reference counted one that you can use like this:

using (OwnedMemory<byte> memory = MemoryPool<byte>.Shared.Rent(size))
{
   ...
}

Other kind of wrappers are being discussed in https://github.com/dotnet/csharplang/issues/1344 or https://github.com/dotnet/corefx/issues/27985. I think the one wrapper that we do need is a wrapper that abstracts away allocation of a local scratch buffer that allocates on stack if the size is small and allocates using array pool when the size is large.

[fgreinacher]

Yeah, the API shape of MemoryPool.Rent is what I was looking for in this proposal. So, I’ll just have to wait for those APIs and migrate my code to use them :) Thanks everyone!

[safern]

Thanks for your interest in the product and proposal @fgreinacher