#3019: ObjectPool wait on borrow condition fix

Foundation/include/Poco/ObjectPool.h

@@ -47,7 +47,7 @@ public:
 	{
 		return new C;
 	}
-	
+
 	bool validateObject(P pObject)
 		/// Checks whether the object is still valid
 		/// and can be reused.
@@ -60,14 +60,14 @@ public:
 	{
 		return true;
 	}
-	
+
 	void activateObject(P pObject)
 		/// Called before an object is handed out by the pool.
 		/// Also called for newly created objects, before
 		/// they are given out for the first time.
 	{
 	}
-	
+
 	void deactivateObject(P pObject)
 		/// Called after an object has been given back to the
 		/// pool and the object is still valid (a prior call
@@ -77,7 +77,7 @@ public:
 		/// must not throw an exception.
 	{
 	}
-	
+
 	void destroyObject(P pObject)
 		/// Destroy an object.
 		///
@@ -97,20 +97,20 @@ public:
 	{
 		return new C;
 	}
-	
+
 	bool validateObject(Poco::AutoPtr<C> pObject)
 	{
 		return true;
 	}
-	
+
 	void activateObject(Poco::AutoPtr<C> pObject)
 	{
 	}
-	
+
 	void deactivateObject(Poco::AutoPtr<C> pObject)
 	{
 	}
-	
+
 	void destroyObject(Poco::AutoPtr<C> pObject)
 	{
 	}
@@ -125,20 +125,20 @@ public:
 	{
 		return new C;
 	}
-	
+
 	bool validateObject(Poco::SharedPtr<C> pObject)
 	{
 		return true;
 	}
-	
+
 	void activateObject(Poco::SharedPtr<C> pObject)
 	{
 	}
-	
+
 	void deactivateObject(Poco::SharedPtr<C> pObject)
 	{
 	}
-	
+
 	void destroyObject(Poco::SharedPtr<C> pObject)
 	{
 	}
@@ -177,7 +177,7 @@ public:
 	{
 		poco_assert (capacity <= peakCapacity);
 	}
-	
+
 	ObjectPool(const F& factory, std::size_t capacity, std::size_t peakCapacity):
 		/// Creates a new ObjectPool with the given PoolableObjectFactory,
 		/// capacity and peak capacity. The PoolableObjectFactory must have
@@ -189,7 +189,7 @@ public:
 	{
 		poco_assert (capacity <= peakCapacity);
 	}
-	
+
 	~ObjectPool()
 		/// Destroys the ObjectPool.
 	{
@@ -205,7 +205,7 @@ public:
 			poco_unexpected();
 		}
 	}
-		
+
 	P borrowObject(long timeoutMilliseconds = 0)
 		/// Obtains an object from the pool, or creates a new object if
 		/// possible.
@@ -217,22 +217,15 @@ public:
 	{
 		Poco::FastMutex::ScopedLock lock(_mutex);
 
-		if (!_pool.empty())
-		{
-			P pObject = _pool.back();
-			_pool.pop_back();
-			return activateObject(pObject);
-		}
-
-		if (_size >= _peakCapacity)
+		if (_size >= _peakCapacity && _pool.empty())
 		{
 			if (timeoutMilliseconds == 0)
 			{
 				return 0;
 			}
-			while (_size >= _peakCapacity)
+			while (_size >= _peakCapacity && _pool.empty())
 			{
-				if ( !_availableCondition.tryWait(_mutex, timeoutMilliseconds))
+				if (!_availableCondition.tryWait(_mutex, timeoutMilliseconds))
 				{
 					// timeout
 					return 0;
@@ -240,10 +233,19 @@ public:
 			}
 		}
 
+		if (!_pool.empty())
+		{
+			P pObject = _pool.back();
+			_pool.pop_back();
+
+			return activateObject(pObject);
+		}
+
 		// _size < _peakCapacity
 		P pObject = _factory.createObject();
 		activateObject(pObject);
 		_size++;
+
 		return pObject;
 	}
 
@@ -260,6 +262,7 @@ public:
 				try
 				{
 					_pool.push_back(pObject);
+					_availableCondition.signal();
 					return;
 				}
 				catch (...)
@@ -276,19 +279,19 @@ public:
 	{
 		return _capacity;
 	}
-	
+
 	std::size_t peakCapacity() const
 	{
 		return _peakCapacity;
 	}
-	
+
 	std::size_t size() const
 	{
 		Poco::FastMutex::ScopedLock lock(_mutex);
-		
+
 		return _size;
 	}
-	
+
 	std::size_t available() const
 	{
 		Poco::FastMutex::ScopedLock lock(_mutex);
@@ -310,12 +313,12 @@ protected:
 		}
 		return pObject;
 	}
-	
+
 private:
 	ObjectPool();
 	ObjectPool(const ObjectPool&);
 	ObjectPool& operator = (const ObjectPool&);
-	
+
 	F _factory;
 	std::size_t _capacity;
 	std::size_t _peakCapacity;

Foundation/testsuite/src/ObjectPoolTest.cpp

@@ -13,6 +13,7 @@
 #include "CppUnit/TestSuite.h"
 #include "Poco/ObjectPool.h"
 #include "Poco/Exception.h"
+#include "Poco/Thread.h"
 
 
 using Poco::ObjectPool;
@@ -30,18 +31,18 @@ ObjectPoolTest::~ObjectPoolTest()
 
 void ObjectPoolTest::testObjectPool()
 {
-	ObjectPool<std::string, Poco::SharedPtr<std::string> > pool(3, 4);
-	
+	ObjectPool<std::string, Poco::SharedPtr<std::string>> pool(3, 4);
+
 	assertTrue (pool.capacity() == 3);
 	assertTrue (pool.peakCapacity() == 4);
 	assertTrue (pool.size() == 0);
 	assertTrue (pool.available() == 4);
-	
+
 	Poco::SharedPtr<std::string> pStr1 = pool.borrowObject();
 	pStr1->assign("first");
 	assertTrue (pool.size() == 1);
 	assertTrue (pool.available() == 3);
-	
+
 	Poco::SharedPtr<std::string> pStr2 = pool.borrowObject();
 	pStr2->assign("second");
 	assertTrue (pool.size() == 2);
@@ -51,19 +52,19 @@ void ObjectPoolTest::testObjectPool()
 	pStr3->assign("third");
 	assertTrue (pool.size() == 3);
 	assertTrue (pool.available() == 1);
-	
+
 	Poco::SharedPtr<std::string> pStr4 = pool.borrowObject();
 	pStr4->assign("fourth");
 	assertTrue (pool.size() == 4);
 	assertTrue (pool.available() == 0);
-	
+
 	Poco::SharedPtr<std::string> pStr5 = pool.borrowObject();
 	assertTrue (pStr5.isNull());
-	
+
 	pool.returnObject(pStr4);
 	assertTrue (pool.size() == 4);
 	assertTrue (pool.available() == 1);
-	
+
 	pool.returnObject(pStr3);
 	assertTrue (pool.size() == 4);
 	assertTrue (pool.available() == 2);
@@ -75,10 +76,10 @@ void ObjectPoolTest::testObjectPool()
 	pool.returnObject(pStr3);
 	pool.returnObject(pStr2);
 	pool.returnObject(pStr1);
-	
+
 	assertTrue (pool.size() == 3);
 	assertTrue (pool.available() == 4);
-	
+
 	pStr1 = pool.borrowObject();
 	assertTrue (*pStr1 == "second");
 	assertTrue (pool.available() == 3);
@@ -88,6 +89,27 @@ void ObjectPoolTest::testObjectPool()
 }
 
 
+void ObjectPoolTest::testObjectPoolWaitOnBorrowObject()
+{
+	ObjectPool<std::string, Poco::SharedPtr<std::string>> pool(1, 1);
+
+	Poco::SharedPtr<std::string> objectToReturnDuringBorrow = pool.borrowObject();
+
+	Poco::Thread threadToReturnObject;
+	threadToReturnObject.startFunc(
+		[&pool, &objectToReturnDuringBorrow]()
+		{
+			pool.returnObject(objectToReturnDuringBorrow);
+		}
+	);
+
+	Poco::SharedPtr<std::string> object = pool.borrowObject(1000);
+
+	threadToReturnObject.join();
+	assertFalse(object.isNull());
+}
+
+
 void ObjectPoolTest::setUp()
 {
 }
@@ -103,6 +125,7 @@ CppUnit::Test* ObjectPoolTest::suite()
 	CppUnit::TestSuite* pSuite = new CppUnit::TestSuite("ObjectPoolTest");
 
 	CppUnit_addTest(pSuite, ObjectPoolTest, testObjectPool);
+	CppUnit_addTest(pSuite, ObjectPoolTest, testObjectPoolWaitOnBorrowObject);
 
 	return pSuite;
 }

Foundation/testsuite/src/ObjectPoolTest.h

@@ -25,6 +25,7 @@ public:
 	~ObjectPoolTest();
 
 	void testObjectPool();
+	void testObjectPoolWaitOnBorrowObject();
 
 	void setUp();
 	void tearDown();