NET8: DefaultObjectPool<T>.Get() method returns null under multi-threads, seems not thread-safe with NET8 and throws exception, while no problem with NET7

[xiaoyuvax]

Description

In case of multi-threads, ObjectPool.Get() returns null, seems not thread-safe under NET8 (8.0.0) and throws exception, but not under NET7.

Reproduction Steps

Checkout below demo to reproduce, watch for debugger break in the logger codes:

Symptom:

ObjectPool.Get() in the log object's WriteInternal() method, which uses a DefaultObjectPool, throws exception.
While the ObjectPoolCase1 and ObjectPoolCase2 in Main() seems not causing this problem, nor eitherway under NET7.

https://github.com/xiaoyuvax/ObjectPoolReuseCaseDemo

Expected behavior

DefaultObjectPool.Get() should be thread-safe, and always return an object instead of null.

Actual behavior

DefaultObjectPool.Get() returns null randomly.

Regression?

No response

Known Workarounds

Check if returns null and use new keyword to create new instance

Configuration

No response

Other information

No response

[jkotas]

DefaultObjectPool lives in dotnet/aspnetcore: https://github.com/dotnet/aspnetcore/blob/main/src/ObjectPool/src/DefaultObjectPool.cs

[gfoidl]

Didn't dig through the latest code, but related PR is #45251

[amcasey]

cc @geeknoid @BrennanConroy

[geeknoid]

Hi @xiaoyuvax I'm not able to reproduce the problem.

The repo you pointed to above doesn't build as the logging project isn't provided. So I took your test code, stripped out all the logging, added an explicit check for null coming from the Get function, and then ran it for several minutes without any issues being raised.

The code I tested is below. This was compiled for net8.0 using the 8.0 version of Microsoft.Extensions.ObjectPool.

using System;
using System.Diagnostics;
using System.Threading.Tasks;
using Microsoft.Extensions.ObjectPool;

namespace ObjectPoolResuseCases;

public interface IReuseCase1
{
    public string Property1 { get; set; }
    public string Property2 { get; set; }
};

public interface IReuseCase2
{
    public string Property3 { get; set; }
    public string Property4 { get; set; }
};

public class DefaultReuseCasePolicy<T, I> : IPooledObjectPolicy<I>
    where T : I, new()
    where I : class
{
    public I Create() => new T();

    public bool Return(I obj) => true;
}

public class MyObject : IReuseCase1, IReuseCase2
{
    public string Property1 { get; set; }
    public string Property2 { get; set; }
    public string Property3 { get; set; }
    public string Property4 { get; set; }
}

internal static class ObjectPoolDemo
{
    private static readonly DefaultObjectPool<IReuseCase1> ObjectPoolCase1 = new(new DefaultReuseCasePolicy<MyObject, IReuseCase1>());

    private static readonly DefaultObjectPool<IReuseCase2> ObjectPoolCase2 = new(new DefaultReuseCasePolicy<MyObject, IReuseCase2>());

    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();
        if (objI == null)
        {
            throw new InvalidOperationException("Uh ho");
        }

        objI = eval?.Invoke(objI) ?? objI;   //note: if the delegate returns null, objI would not be changed.
        return objI is T objT ? objT : default;
    }

    [STAThread]
    private static void Main()
    {
        int counter = 0;

        Console.WriteLine("Loop started!");
        while (true)
        {
            _ = Task.Run(() =>
            {
                try
                {
                    var objCase1 = ObjectPoolCase1.Get<MyObject, IReuseCase1>(t =>
                    {
                        t.Property1 = $"A{counter}";
                        t.Property2 = $"B{counter}";

                        return t;
                    });

                    ObjectPoolCase1.Return(objCase1);

                    var objCase2 = ObjectPoolCase2.Get<MyObject, IReuseCase2>(t =>
                    {
                        t.Property3 = $"C{counter}";
                        t.Property4 = $"D{counter}";
                        return t;
                    });

                    ObjectPoolCase2.Return(objCase2);

                    var objCase3 = ObjectPoolCase1.Get<MyObject, IReuseCase1>(t =>
                    {
                        t.Property1 = $"E{counter}";
                        t.Property2 = $"F{counter}";

                        return t;
                    });

                    counter++;
                }
                catch (Exception ex)
                {
                    Debugger.Break();
                }
            });

            Console.SetCursorPosition(0, Console.CursorTop);
            Console.Write("Looping:" + (counter % 4) switch { 0 => "-", 1 => @"\", 2 => "|", _ => "/" });
        }
    }

Did I delete something critical?

Perhaps you can try to reproduce the problem with the explicit "if" check I added right after pool.Get to confirm that this is actually where null is coming from?

[xiaoyuvax]

@geeknoid the key is in in the log code (do pull the entire demo pls) , as i've commented in the code.
Actually is the StringBuilderPool in the log codes (in WriteInternal() method) throws exception.
The entire example(together with the log codes) displays two different cases of behaviors of DefaultObjectPool: one that in the main program is ok, but the other in the log codes is problematic, u can try to compare to see what's wrong there.

Please read the following again:

Symptom:
ObjectPool.Get() in the log object's WriteInternal() method, which uses a DefaultObjectPool, throws exception.
While the ObjectPoolCase1 and ObjectPoolCase2 in Main() seems not causing this problem, nor eitherway under NET7.

[geeknoid]

Ah sorry, when I pulled your repo I didn't notice that it used submodules, so it wouldn't built as-is. Once I pulled the submodules, I was able to build and run the code.

Turns out the issue is that your code is calling the object pool's Return method and passing a null reference in. This leads to the null reference being returned back to you the next time you query the pool. So the Get behavior is correct (this is a case of GIGO).

I'll open a PR to add null checks in the Return API to make diagnosing this situation easier in the future.

[geeknoid]

#52935

[xiaoyuvax]

thx, bingo, then i'd better add explicity check so far...
NET7's taken care of such null references smoothly, as i've assumed with NET8, it's should be a good tradition to pass on.