Surface the _numItems as a public property on DefaultObjectPool

[Shazwazza]

Background and Motivation

It would be nice to be able to emit a metric anytime the DefaultObjectPool limit is breached. Currently this is not possible because the bool ReturnCore is marked private protected so it cannot be overridden to get the bool response from this method. If the _numbItems was encapsulated as a Property {get}, than it would be possible to determine if the pool is full.

Proposed API

public class DefaultObjectPool<T> : ObjectPool<T> where T : class
{
    private readonly Func<T> _createFunc;
    private readonly Func<T, bool> _returnFunc;
    private readonly int _maxCapacity;
    private int _numItems;

    private protected readonly ConcurrentQueue<T> _items = new();
    private protected T? _fastItem;
    
+   public int ItemCount => _numItems;

Usage Examples

// return item to the pool:
pool.Return(item);

// is it full?
if (pool.ItemCount == _maximumRetained)
{
   // emit metric for tracking
}

Alternative Designs

• Remove private from the private protected bool ReturnCore so it can be overridden, call the base class to get the boolean response and emit the metric there.
• Change ObjectPool<T>.void Return(T obj) to ObjectPool<T>.bool Return(T obj)

Risks

The least risk is to expose the int property so that the underlying APIs to need to change. I don't foresee any risks by exposing the ItemCount property.

[dotnet-policy-service]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[halter73]

I like the alternative designs, because it's more reliable than checking the ItemCount after calling Return. With just ItemCount, I don't think it's possible to reliably determine how many items got disposed. I supposed you could implement a custom IPooledObjectPolicy that tracks the calls to Create and Return, but you could do this already. That doesn't mean we shouldn't make this easier though.

Unfortunately, we cannot make any breaking changes. We would need to add a new version of Return that returns a bool, and we'd need to come up with a name other than Return for this, since you cannot overload on return type in C#. Maybe "TryReturn" would be a good name for the bool-returning version of return even though Return is already best-effort.

Also, are you sure you're okay with not making changes to the ObjectPool abstract base class? It's a lot easier if we don't need to change the base class because we won't have to worry about the default virtual implementation or capability-testing. But if you're not using the abstraction, I'm not sure how much value ObjectPool really adds over managing the ConcurrentQueue yourself which would give you far more flexibility and introspection capabilities.

[Shazwazza]

I agree, ideally we can modify the ObjectPool abstract class but that means breaking changes which we can't do. Adding the TryReturn to DefaultObjectPool wouldn't be a breaking change and would certainly be more robust. It would essentially just do the same thing as public override void Return(T obj) but have a return type:

public bool TryReturn(T obj) => ReturnCore(obj);

It would mean that for folks to use this, they'd not be referencing the abstraction but DefaultObjectPool specifically. Maybe in another major dotnet version, the ObjectPool class's signature would change to:

public abstract bool TryReturn(T obj);

And on a side note, is there any plans to move the ObjectPool* code to not exist within the aspnetcore repo?