Pool WeakReferences in AsyncTimeoutQueue

[verdie-g]

Following https://github.com/aerospike/aerospike-client-csharp/pull/69, now only the WeakReferences remain in gen 2, this change tries to fix that.

With 100 QPS, the WeakReference disappear from the dead objects.

I'm using Microsoft.Extensions.ObjectPool version 2.2.0 which is the same version used by ASP.NET Core 2.2.

One issue with that change is that I set the maximum retained object to 10000 but in my initial dump I found 240K items in the queue. That can be explained by the fact that a single instance of the service does 10K queries per second to Aerospike, so with a 30s socket timeout the queue will be huge.

I'm not sure what do to about it, the maximum retained objects in the pool could be increased but that doesn't sound right. Another solution is to have a max interval in addition to the min interval so that timeouts are checked earlier.

[BrianNichols]

The code still creates a new WeakReference for every ITimeout instance. Where is the call to retrieve a WeakReference from the pool?

Could you explain how this pool works in more detail?

I think a configurable max interval is reasonable. The max retained objects should be configurable as well. Perhaps as static variables?

[verdie-g]

The code still creates a new WeakReference for every ITimeout instance

Fixed. I've messed up the commit.

Could you explain how this pool works in more detail?

You can find its implementation here: DefaultObjectPool. I also added some comments in the code.

I think a configurable max interval is reasonable. The max retained objects should be configurable as well.

I'm uncomfortable exposing implementation details like that to the user. I can add a max interval in a following PR.

[BrianNichols]

Is ObjectPool.Get() thread-safe?

It would be safer to declare queue as "ConcurrentQueue\<ITimeout>" and then get/create the WeakReference in RegisterCommands().

Also, I don't understand the following code:

    if (commandRef.TryGetTarget(out ITimeout command) && command.CheckTimeout())
    {
        list.AddLast(commandRef);
    }

    WeakRefPool.Return(commandRef);

If CheckTimeout() returns true, commandRef will be re-added to the list and also returned to the pool.

[verdie-g]

Is ObjectPool.Get() thread-safe?

Yes.

It would be safer to declare queue as "ConcurrentQueue" and then get/create the WeakReference in RegisterCommands().

That would defeat the purpose of the WeakReference. Objects are kept in the queue for the duration of their timeout which can be up to 30 seconds, reaching gen 2 every time.

I misread the first time and thought you wanted to replace the pool with a ConcurrentQueue and I like the idea because it simplifies the dependencies of the project.

If CheckTimeout() returns true, commandRef will be re-added to the list and also returned to the pool.

That's a bug. It should be fixed in a new commit.

[BrianNichols]

The pull request has been accepted into our stage branch with modifications. Support for MaxWeakRefPoolCount and MaxInterval was added. The stage branch commit is: https://github.com/aerospike/aerospike-client-csharp/commit/4775289210530c303d1207a69612565285131c94

Please review this commit.

[verdie-g]

Ok I've added a few comments.

[BrianNichols]

Based on feedback, I have made the following changes: https://github.com/aerospike/aerospike-client-csharp/commit/04d56cd294a7ac0b789bd6af45c2e98f2a1364f1

If there are no other pull requests, the client can be released within the next few days.

[verdie-g]

No other pull requests planned. Looking forward for the release!

[BrianNichols]

C# client 5.2.1 is released: https://download.aerospike.com/download/client/csharp/notes.html#5.2.1