[API Proposal]: ObjectPool.Create without new() constraint

[ladeak]

Background and motivation

Today the existing ObjectPool.Create has new() constraint on the generic type parameter, because DefaultPooledObjectPolicy requires it.

Suggestion is a non-nullable IPooledObjectPolicy overload, so with a custom pooled object policy, we can create object pools for T does not have a default ctor (that would be the benefit of creating a custom IPooledObjectPolicy)

public static ObjectPool<T> Create<T>(IPooledObjectPolicy<T> policy) where T : class

API Proposal

namespace Microsoft.Extensions.ObjectPool;

public static class ObjectPool
{
    public static ObjectPool<T> Create<T>(IPooledObjectPolicy<T> policy) where T : class
}

API Usage

ObjectPool<MyType> pool = ObjectPool.Create<MyType>(new MyTypePolicy());

class MyType(int value) { }

class MyTypePolicy : IPooledObjectPolicy<MyType>
{
    public MyType Create() => new MyType(0);
    public bool Return(MyType obj)
    {
        return true;
    }
}

Alternative Designs

Instead of an overload (which might require changing and removing the parameter of the existing method), have a separate method name.

(I am happy to address the issue if there are no objections during the discussion.)

[ladeak]

Is this something I can take a look at? Any code owner/maintainer could agree/disagree?

[WeihanLi]

generally, the new api proposal should get approved before implementation

[ladeak]

@WeihanLi , strictly speaking not a new api, just separating an existing one by the optional parameter. I am looking forward to a feedback from the .NET team.

[halter73]

I think it would require a new method name to be non-breaking since the current method currently allows a null policy and the new() constraint is required in that case. We could use ActivatorUtilities and throw a runtime exception, but I don't think it's worth it. It's not quite as discoverable, but I think using the DefaultObjectPoolProvider provider directly should give you what you want. And I figure that's about as discoverable as a new method.

var poolProvider = new DefaultObjectPoolProvider();
ObjectPool<MyType> pool = poolProvider.Create(new MyTypePolicy());

class MyType(int value) { }

class MyTypePolicy : IPooledObjectPolicy<MyType>
{
    public MyType Create() => new MyType(0);
    public bool Return(MyType obj)
    {
        return true;
    }
}

If we we're to do it all over again, we'd probably have to overloads, ObjectPool.Create<T>() where T : new() and ObjectPool.Create<T>(IPooledObjectPolicy<T> policy) without constraints, but this would break code that explicitly passed in null or didn't recompile if we changed it now.

[ladeak]

The workaround is present, although less discoverable/documented, otherwise I would not have created this issue. If an overload is added (resulting one method with a required parameter and one with no parameters) it would be a breaking change for cases when null is explicitly passed in - not sure if anyone should have passed null intentionally.
The goal of the proposal is to "adjust" the API as if it was newly developed while taking the hit of the breaking change (I felt that was still reasonable).
I am happy with closing the issue if you suggest that this not reasonable anymore. As far as I understand it is the case.

[halter73]

@ladeak I admit that the discoverability of DefaultObjectPoolProvider isn't great. I'm trying to think of a change that would be safe enough to try for .NET 11.

public static class ObjectPool
{
-    public static ObjectPool<T> Create<T>(IPooledObjectPolicy<T>? policy = null) where T : class, new()
+    public static ObjectPool<T> Create<T>() where T : class, new()
+    public static ObjectPool<T> Create<T>(IPooledObjectPolicy<T> policy) where T : class
}

This would at least force you to provide a type with a default constructor if you compile against the non-policy overload.

The problem I have with this is that any code that doesn't provide a policy that's compiled against the old version of object ObjectPool.Create<MyType>() will still call the IPooledObjectPolicy<T> overload. We'll no longer be able to call new T() for this overload because we no longer have the generic constraint. se'd be forced to fall back to ActivatorUtilities in this case rather than simply throwing a clean ArgumentNullException like we would if it was a new API.

I'm not sure it's worth it, but we can discuss it in API review.

[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.