[API Proposal]: Introduce IResettable to streamline object pool usage

[geeknoid]

Background and motivation

We use a lot of object pools in our environment and its clumsy to have to implement dedicated policy types when we generally want a single behavior applied to a large variety of object types. We adopted a solution of having all these objects declare themselves as IResettable and then a single policy suffices to deal with these objects

API Proposal

namespace Microsoft.Extensions.ObjectPool;

/// <summary>
/// Defines a method to reset an object to its initial state.
/// </summary>
public interface IResettable
{
    /// <summary>
    /// Reset the object to a neutral state, semantically similar to when the object was first constructed.
    /// </summary>
    /// <remarks>
    /// In general, this method is not expected to be thread-safe.
    /// </remarks>
    void Reset();
}

/// <summary>
/// An object pool policy for resettable objects.
/// </summary>
public sealed class ResettablePooledObjectPolicy<T> : PooledObjectPolicy<T>
    where T : notnull, IResettable, new()
{
    public static ResettablePooledObjectPolicy<T> Instance { get; } = new();

    private ResettablePooledObjectPolicy()
    {
    }

    public override T Create() => new();

    public override bool Return(T obj)
    {
        obj.Reset();
        return true;
    }
}

API Usage

// a resettable object is one that logically can come back to its initial state
internal class MyState : IResettable
{
    private readonly List<int> _data = new();
    public void Add(int value) { _data.Add(value); }
    public void Reset() { _data.Clear(); }
}

var pool = ObjectPool.Create<MyState>(ResettableObjectPoolPolicy<MyState>.Instance);

// or alternatively, there could be a new method on ObjectPool, which requires less typing...
var pool = ObjectPool.CreateResettable<MyState>();

Alternative Designs

No response

Risks

No response

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[joperezr]

cc: @Tratcher

[xiaoyuvax]

I also met similar challenge to use instances returned from the object pool, cos they contain old property values and references to other objects, which may lead to problems, if not intialized.
but the IResettable solution requires implementing Reset() method for each object, it's a lot of labor if u've got a lot of types, cos u have to initialize each member of a type, some of which may be of reference to other types, as is complicated.

So far my solution is an extenstion method which cleans(resets).

 public static T Clean<T>(this T obj)  // where T: IResetable    <-- U can limit it to certain interface
        {
            foreach (var p in obj.GetType().GetProperties().Where(i => i.CanWrite))
               p.SetValue(obj, default);
//In my opinion, references in properties can be simply reset as null, the code reusing these properties is responsible to specify their references, e.g. from ObjectPool. 

            if (obj is IResetable resetable) resetable.Reset();      // <- Reset() can be done here.
            // thus Reset() can only contains fewer necessary code specific to each type, instead of refilling up all properties.
            return obj;
        }

For performance consideration：
1）Above Clean() method is not required on all cases, e.g. when the fetched object would be fully overwritten by subsequent code, while the IResetable solution costs more in such case.
2）DefaultObjectPool should seek lowlevel solution to reset the instance as if the object were created like new(), for neither this Clean() nor IResetable solution is not performance friendly.

Anyway, I adopted the Reset() method, but the ObjectPoolPolicy model is not favorable for my case.

[ghost]

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]

Could the default policy call IResettable.Reset() on return if the object happens to implement the interface? Since this is a new interface, this should be non breaking.

@JamesNK asks if Reset() should return a boolean to indicate success? Like TryReset? Could be useful for things like CancellationTokenSource.

Another option is to keep the ResettablePooledObjectPolicy type, and add a public ResettablePooledObjectPolicy(Func<T, bool> tryReset). This way it could work for types you don't own. We might even be able to replat the StringBuilder policy on it and get rid of the interface.

[xiaoyuvax]

ah, this "default policy call IResettable.Reset() " if detected, is preferable, avoiding introducing new Pool or Policy.

I also suggest Object.Get(bool NoReset=true), as ignores calling the Reset() method, and fetch an object as it is, as to improve performance. If this instance is to be fully overwritten later in the code, it doesn't need a reset.

[geeknoid]

Having the default policy call Reset would require adding a type test in the existing paths, which would slow things down a teeny bit.

Having the Reset function return a boolean is a reasonable idea, although it seems fairly niche. Nevertheless the semantics would be clear. If Reset returns false, then the object is dropped on the floor and not added to the pool.

[Tratcher]

Is the current proposal:

public interface IResettable
{
    bool Reset();
}

And a type check in PooledObjectPolicy.Return?

[geeknoid]

Is the current proposal:

public interface IResettable
{
    bool Reset();
}

And a type check in PooledObjectPolicy.Return?

Yes, I think so.

[halter73]

API Review Notes:

• We're a fan of the bool return value because it allows for objects to enter a non-resettable state.
• Can we get away with only updating the DefaultPooledObjectPolicy<T>? StringBuilderPooledObjectPolicy exists, but StringBuilder is sealed.
• Is there anything analogous for object creation? No.
• Do we expect custom policies to respect this? No. If you are unsure what the PooledPolicy<T> will be, don't rely on this.
• By removing the ResettablePooledObjectPolicy, we lose the generic type constraint. However, given a plain old ObjectPool<T> that you didn't construct, you can never know if it will call IResettable.Reset().
• The alternative is always calling reset yourself before returning. The upside of this proposal is that if you construct the ObjectPool and define the type yourself, you know you won't accidentally not reset. Or have others forget. This is already possible with custom polices, but it's more verbose.

API Approved!

namespace Microsoft.Extensions.ObjectPool;

public interface IResettable
{
    bool TryReset();
}

Edit: The proposal was intended to be "TryReset" rather than "Reset" since IResettable returns a bool.

[Tratcher]

@halter73 that should have been TryReset.

[halter73]

Yeah. We should have discussed that. I figured we were trying to match PooledObjectPolicy<T>.Return(T) which also returns a book for similar reasons without "Try", but I think the "Try" is more clear. It aligns with CancellationTokenSourece.TryReset().

@Tratcher can you send an email? I don't think it will be controversial, but it's good for visibility.

[Tratcher]

We did discuss it, and even agree to it, it just didn't make it into the notes.

[halter73]

I had to recheck the meeting video but @geeknoid did mention that "TryReset" was the new boolean-returning API name while I was distracted trying to copy the "current proposal. I never wrote it out or confirmed that we were all satisfied the name, but that's on me. I'll update the approval and make a note of it in the wrap up email.

[CodeBlanch]

I'm just going to throw this out there. I do agree basic usage of the ObjectPool requires too much ceremony. But I don't think IResettable really helps with that, it is also a lot of ceremony. I had this proposal a while back to introduce a simple helper extension which you can pass a delegate to for doing reset (if needed): #36586

I think it accomplishes the goal with less 🤷

[It got viciously tossed because this public API is not for public usage? Dub-tee-eff? I'm not bitter about that at all 🤣]

[xiaoyuvax]

After experimenting similar resetting mechanism in my project, i found it a big performance disaster to generally reset every instance, especially if a Type owns too many properties, and if most of its properties r to be overwritten later.

Now i've removed all experimental resetting implementations and adopted a more performance-friendly solution:
Use different, dedicated DefaultObjectPools for different object Reuse Cases (in accordance with object life-cycle line) of the same type, so that the same group of properties r always overwritten exactly in reusing code (i.e. avoid messing up reuse-cases), thus to avoid resetting. and wherein u don't have to use Reset() or custom ObjectPoolPolicy while assuring performance.

The only problem here is how to manage different reuse-cases, better with code, as to make it more readable and managable.
i have contemplated some solutions:

1. Group up properties of Reuse Cases by Component Interfaces:
   public class MyObject : IReuseCase1, IReuseCase2

2. allow immediate evaluation(if needed) upon callling a new Get<T,I>(Func<I,I>) method with Component Interfaces, as to make property evaluation more concentratedly visible, as to improve readbility and manageability (if business logic allows).

   /// <summary>
   /// This method's suggested to be added or just extended to DefaultObjectPool<T>,
   /// as to retrieve an instance from the pool with contracted subsets of properties according to multiple lifecycle-lines of the type.
   /// This method is only a helper and demonstration of the 3 steps to use Component Interfaces, but not a must.
   /// </summary>
   /// <typeparam name="T">The type which may has multiple Reuse Cases</typeparam>
   /// <typeparam name="I">One of the component interfaces for T</typeparam>
   /// <param name="case1"></param>
   /// <param name="eval">Delegate to evaluate underlying properties immediately, if u don't want to do it immediately, u can simply use Get<I>() on the ObjectPool</param>
   /// <returns></returns>
public static T Get<T, I>(this DefaultObjectPool<I> pool, Func<I, I> eval = null) 
  where T: I
  where I : class
   {
       I objI = pool.Get();
       objI = eval?.Invoke(objI) ?? objI;   //note: if the delegate returns null, objI would not be changed.
       return objI is T objT ? objT : default;
   }

I made a more complete demo here (we need a DefaultObjectPoolPolicy4ReuseCases<T, I> class) :
ObjectPoolReuseCaseDemo

3. above solution could manage Reuse-Case Consistency already, but i would also expect certain analyzer/compiler help, such as simple warnings, if reuse-case consistency is violated.

Summary (in contrast with IResettable solution) :

1. Isolate Reuse Cases according to object lifecycle lines and create separated Objectpools for the same type can avoid using a dictating Reset() method.
2. Introducing Component Interfaces(sorta contracts) to constraint evalution of an object, and can be codedly equivalent to an optimized partial Reset().

What do u guys think?