HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Generic object pool - what should be changed to make it easier to understand?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
genericwhatmakeunderstandshouldpoolobjectchangedeasier

Problem

I have created a simple class that maintains a pool (with maximum size) of objects. It acts like a connection pool (that is where the name came from - initially it was used just to maintain a pool of SMTP connections). When an item is retrieved - if the pool is not empty, a value is retrieved from it; if it is empty, then if the maximum number of objects have not been allocated, a new one is created.

The usage as it currently stands is:

```
///
/// A generic object pool for any data. A pool is automatically created based on the data type.
/// If the pool size is currently exceeded then the task will wait until an object is returned from the pool.
///
public static class ConnectionPool
{
///
/// Sets the maximum pool size. This method is NOT thread-safe and has to be called before the first item is retrieved from the pool.
///
public static void SetPoolSize(int maximumSize)
where TValue : class
{
ConnectionPoolWrapper.SetPoolSize(maximumSize);
}

///
/// Retrieves a new item from the pool. If the pool is empty and the maximum number of items are active, the task will delay
/// until an item is freed.
///
/// Delegate that is used to create a new item if the pool is empty but maximum number of items have not been reached.
public static Task> RetrieveAsync(Func initializer)
where TValue: class
{
return ConnectionPoolWrapper.RetrieveFromPoolAsync(initializer);
}

///
/// Retrieves a new item from the pool. If the pool is empty and the maximum number of items are active, the task will delay
/// until an item is freed.
///
/// The default constructor is used to create new objects when needed.
public static Task> RetrieveAsync()
where TValue : class, new()
{
return ConnectionPoolWrapper.RetrieveFromPoolAsync(() => new TValue());
}
}

///
/// An item in the object pool.
///
public sealed class ConnectionPoolWrapper : IDisposa

Solution

-
I think that syntax like ConnectionPool.RetrieveAsync() is more natural than your syntax. If you switch to that, you can probably eliminate the ConnectionPool/ConnectionPoolWrapper separation when it comes to static methods, which doesn't seem to serve any other purpose.

-
Use usings to make your code shorter, there is no reason to write the full System.Threading.SemaphoreSlim every time you want to use that type.

-
new System.Threading.SemaphoreSlim(5) the 5 here should be a named constant. Or it could be a property with default value, that can be modified. (Though in that case, I'm not sure what the property setter should do if it's invoked after the semaphore has already been created.)

-
I think your initialization of _counter using double-checked locking is not actually thread-safe, the _counter variable would have to be volatile. For some more information, see Jon Skeet's article on implementing singletons.

-
What's the point of the value local variable in the Value getter? If it's trying to be thread-safe, then I don't see a reason for that: the instance of ConnectionPoolWrapper should never be accessed from multiple threads at the same time.

-
Some people recommend always using this. for accessing fields, some people recommend using a prefix like _. But I don't see any reason to use both at the same time.

Context

StackExchange Code Review Q#41986, answer score: 4

Revisions (0)

No revisions yet.