Skip to content

Surface the _numItems as a public property on DefaultObjectPool #61029

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Shazwazza opened this issue Mar 19, 2025 · 2 comments
Open

Surface the _numItems as a public property on DefaultObjectPool #61029

Shazwazza opened this issue Mar 19, 2025 · 2 comments
Labels
api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically

Comments

@Shazwazza
Copy link
Contributor

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.

@Shazwazza Shazwazza added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Mar 19, 2025
@ghost ghost added the needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically label Mar 19, 2025
@sebastienros sebastienros added api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels May 30, 2025
Copy link
Contributor

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically
Projects
None yet
Development

No branches or pull requests

3 participants