From ac095412437fc71cb76a6ee83388c7ec31b2cd16 Mon Sep 17 00:00:00 2001 From: Toby Collett Date: Mon, 29 Apr 2013 05:43:39 +0000 Subject: [PATCH] Use shared pointer reference count to track pools By using the shared pointer reference count to track the connections in the connection pool we no longer have to explicitly return them to the pool. This resolves an issue where cursor feature sets were returning their connections before they were done with them, causing a potential race with another connection user. --- include/mapnik/pool.hpp | 80 +++++--------------- plugins/input/postgis/postgis_datasource.cpp | 10 --- 2 files changed, 19 insertions(+), 71 deletions(-) diff --git a/include/mapnik/pool.hpp b/include/mapnik/pool.hpp index fe0572687..ba22cd7fa 100644 --- a/include/mapnik/pool.hpp +++ b/include/mapnik/pool.hpp @@ -42,28 +42,6 @@ namespace mapnik { -template -class PoolGuard -{ -private: - const T& obj_; - PoolT& pool_; -public: - explicit PoolGuard(const T& ptr,PoolT& pool) - : obj_(ptr), - pool_(pool) {} - - ~PoolGuard() - { - pool_->returnObject(obj_); - } - -private: - PoolGuard(); - PoolGuard(const PoolGuard&); - PoolGuard& operator=(const PoolGuard&); -}; - template class Creator> class Pool : private mapnik::noncopyable { @@ -73,8 +51,7 @@ class Pool : private mapnik::noncopyable Creator creator_; unsigned initialSize_; unsigned maxSize_; - ContType usedPool_; - ContType unusedPool_; + ContType pool_; #ifdef MAPNIK_THREADSAFE mutable boost::mutex mutex_; #endif @@ -89,7 +66,7 @@ public: { HolderType conn(creator_()); if (conn->isOK()) - unusedPool_.push_back(conn); + pool_.push_back(conn); } } @@ -98,31 +75,33 @@ public: #ifdef MAPNIK_THREADSAFE mutex::scoped_lock lock(mutex_); #endif - typename ContType::iterator itr=unusedPool_.begin(); - while ( itr!=unusedPool_.end()) - { - MAPNIK_LOG_DEBUG(pool) << "pool: Borrow instance=" << (*itr).get(); - if ((*itr)->isOK()) + typename ContType::iterator itr=pool_.begin(); + while ( itr!=pool_.end()) + { + if (!itr->unique()) { - usedPool_.push_back(*itr); - unusedPool_.erase(itr); - return usedPool_.back(); + ++itr; + } + else if ((*itr)->isOK()) + { + MAPNIK_LOG_DEBUG(pool) << "pool: Borrow instance=" << (*itr).get(); + return *itr; } else { MAPNIK_LOG_DEBUG(pool) << "pool: Bad connection (erase) instance=" << (*itr).get(); - itr=unusedPool_.erase(itr); + itr=pool_.erase(itr); } } // all connection have been taken, check if we allowed to grow pool - if (usedPool_.size() < maxSize_) + if (pool_.size() < maxSize_) { HolderType conn(creator_()); if (conn->isOK()) { - usedPool_.push_back(conn); + pool_.push_back(conn); MAPNIK_LOG_DEBUG(pool) << "pool: Create connection=" << conn.get(); @@ -132,33 +111,12 @@ public: return HolderType(); } - void returnObject(HolderType obj) + unsigned size() const { #ifdef MAPNIK_THREADSAFE mutex::scoped_lock lock(mutex_); #endif - typename ContType::iterator itr=usedPool_.begin(); - while (itr != usedPool_.end()) - { - if (obj.get()==(*itr).get()) - { - MAPNIK_LOG_DEBUG(pool) << "pool: Return instance=" << (*itr).get(); - - unusedPool_.push_back(*itr); - usedPool_.erase(itr); - return; - } - ++itr; - } - } - - std::pair size() const - { -#ifdef MAPNIK_THREADSAFE - mutex::scoped_lock lock(mutex_); -#endif - std::pair size(unusedPool_.size(),usedPool_.size()); - return size; + return pool_.size(); } unsigned max_size() const @@ -193,7 +151,7 @@ public: if (size > initialSize_) { initialSize_ = size; - unsigned total_size = usedPool_.size() + unusedPool_.size(); + unsigned total_size = pool_.size(); // ensure we don't have ghost obj's in the pool. if (total_size < initialSize_) { @@ -203,7 +161,7 @@ public: { HolderType conn(creator_()); if (conn->isOK()) - unusedPool_.push_back(conn); + pool_.push_back(conn); } } } diff --git a/plugins/input/postgis/postgis_datasource.cpp b/plugins/input/postgis/postgis_datasource.cpp index 394a036d1..586a3442d 100644 --- a/plugins/input/postgis/postgis_datasource.cpp +++ b/plugins/input/postgis/postgis_datasource.cpp @@ -53,7 +53,6 @@ const std::string postgis_datasource::GEOMETRY_COLUMNS = "geometry_columns"; const std::string postgis_datasource::SPATIAL_REF_SYS = "spatial_ref_system"; using boost::shared_ptr; -using mapnik::PoolGuard; using mapnik::attribute_descriptor; postgis_datasource::postgis_datasource(parameters const& params) @@ -115,8 +114,6 @@ postgis_datasource::postgis_datasource(parameters const& params) shared_ptr conn = pool->borrowObject(); if (!conn) return; - PoolGuard, - shared_ptr< Pool > > guard(conn, pool); if (conn->isOK()) { @@ -431,7 +428,6 @@ postgis_datasource::~postgis_datasource() if (conn) { conn->close(); - pool->returnObject(conn); } } } @@ -603,8 +599,6 @@ featureset_ptr postgis_datasource::features(const query& q) const shared_ptr conn = pool->borrowObject(); if (conn && conn->isOK()) { - PoolGuard, shared_ptr< Pool > > guard(conn ,pool); - if (geometryColumn_.empty()) { std::ostringstream s_error; @@ -696,7 +690,6 @@ featureset_ptr postgis_datasource::features(const query& q) const if (conn) { err_msg += conn->status(); - pool->returnObject(conn); } else { @@ -719,7 +712,6 @@ featureset_ptr postgis_datasource::features_at_point(coord2d const& pt, double t { shared_ptr conn = pool->borrowObject(); if (!conn) return featureset_ptr(); - PoolGuard, shared_ptr< Pool > > guard(conn, pool); if (conn->isOK()) { @@ -804,7 +796,6 @@ box2d postgis_datasource::envelope() const { shared_ptr conn = pool->borrowObject(); if (!conn) return extent_; - PoolGuard, shared_ptr< Pool > > guard(conn, pool); if (conn->isOK()) { std::ostringstream s; @@ -896,7 +887,6 @@ boost::optional postgis_datasource::get_geometry { shared_ptr conn = pool->borrowObject(); if (!conn) return result; - PoolGuard, shared_ptr< Pool > > guard(conn, pool); if (conn->isOK()) { std::ostringstream s;