From 6844863a89a3afc7a244c1c328fc7775216fcbce Mon Sep 17 00:00:00 2001 From: Dane Springmeyer Date: Tue, 12 Nov 2013 18:38:39 -0800 Subject: [PATCH] Abort pending postgis connections when exception occurs - patch from @abonnasseau - closes #2042 - refs #2069 --- plugins/input/postgis/asyncresultset.hpp | 16 +++++++ plugins/input/postgis/connection.hpp | 20 +++++++-- tests/python_tests/postgis_test.py | 57 ++++++++++++++++++++++++ 3 files changed, 89 insertions(+), 4 deletions(-) diff --git a/plugins/input/postgis/asyncresultset.hpp b/plugins/input/postgis/asyncresultset.hpp index a5f838ad8..c9396897a 100644 --- a/plugins/input/postgis/asyncresultset.hpp +++ b/plugins/input/postgis/asyncresultset.hpp @@ -54,6 +54,18 @@ public: close(); } + + void abort() + { + if(conn_ && conn_->isPending() ) + { + MAPNIK_LOG_DEBUG(postgis) << "AsyncResultSet: aborting pending connection - " << conn_.get(); + // there is no easy way to abort a pending connection, so we close it : this will ensure that + // the connection will be recycled in the pool + conn_->close(); + } + } + virtual void close() { if (!is_closed_) @@ -62,6 +74,10 @@ public: is_closed_ = true; if (conn_) { + if(conn_->isPending()) + { + abort(); + } conn_.reset(); } } diff --git a/plugins/input/postgis/connection.hpp b/plugins/input/postgis/connection.hpp index 8c764225f..b67272926 100644 --- a/plugins/input/postgis/connection.hpp +++ b/plugins/input/postgis/connection.hpp @@ -46,7 +46,8 @@ class Connection public: Connection(std::string const& connection_str,boost::optional const& password) : cursorId(0), - closed_(false) + closed_(false), + pending_(false) { std::string connect_with_pass = connection_str; if (password && !password->empty()) @@ -54,6 +55,7 @@ public: connect_with_pass += " password=" + *password; } conn_ = PQconnectdb(connect_with_pass.c_str()); + MAPNIK_LOG_DEBUG(postgis) << "postgis_connection: postgresql connection create - " << this; if (PQstatus(conn_) != CONNECTION_OK) { std::string err_msg = "Postgis Plugin: "; @@ -61,6 +63,8 @@ public: err_msg += "\nConnection string: '"; err_msg += connection_str; err_msg += "'\n"; + MAPNIK_LOG_DEBUG(postgis) << "postgis_connection: creation failed, closing connection - " << this; + close(); throw mapnik::datasource_exception(err_msg); } } @@ -70,7 +74,7 @@ public: if (! closed_) { PQfinish(conn_); - MAPNIK_LOG_DEBUG(postgis) << "postgis_connection: postgresql connection closed - " << conn_; + MAPNIK_LOG_DEBUG(postgis) << "postgis_connection: postgresql connection closed - " << this; closed_ = true; } } @@ -156,6 +160,7 @@ public: close(); throw mapnik::datasource_exception(err_msg); } + pending_ = true; return result; } @@ -204,12 +209,17 @@ public: return (!closed_) && (PQstatus(conn_) != CONNECTION_BAD); } + bool isPending() const + { + return pending_; + } + void close() { if (! closed_) { PQfinish(conn_); - MAPNIK_LOG_DEBUG(postgis) << "postgis_connection: datasource closed, also closing connection - " << conn_; + MAPNIK_LOG_DEBUG(postgis) << "postgis_connection: closing connection (close)- " << this; closed_ = true; } } @@ -225,8 +235,9 @@ private: PGconn *conn_; int cursorId; bool closed_; + bool pending_; - void clearAsyncResult(PGresult *result) const + void clearAsyncResult(PGresult *result) { // Clear all pending results while(result) @@ -234,6 +245,7 @@ private: PQclear(result); result = PQgetResult(conn_); } + pending_ = false; } }; diff --git a/tests/python_tests/postgis_test.py b/tests/python_tests/postgis_test.py index 05852f650..256801c3e 100644 --- a/tests/python_tests/postgis_test.py +++ b/tests/python_tests/postgis_test.py @@ -701,6 +701,63 @@ if 'postgis' in mapnik.DatasourceCache.plugin_names() \ count += 1 eq_(count,8) + + def test_psql_error_should_give_back_connections_opened_for_lower_layers_to_the_pool(): + map1 = mapnik.Map(600,300) + s = mapnik.Style() + r = mapnik.Rule() + r.symbols.append(mapnik.PolygonSymbolizer(mapnik.Color('#f2eff9'))) + s.rules.append(r) + map1.append_style('style',s) + + # This layer will fail after a while + buggy_s = mapnik.Style() + buggy_r = mapnik.Rule() + buggy_r.symbols.append(mapnik.PolygonSymbolizer(mapnik.Color('#ff0000'))) + buggy_r.filter = mapnik.Filter("[fips] = 'FR'") + buggy_s.rules.append(buggy_r) + map1.append_style('style for buggy layer',buggy_s) + buggy_layer = mapnik.Layer('this layer is buggy at runtime') + # We ensure the query wille be long enough + buggy_layer.datasource = mapnik.PostGIS(dbname=MAPNIK_TEST_DBNAME,table='(SELECT geom as geom, pg_sleep(0.1), fips::int from world_merc) as failure_tabl', + max_async_connection=2, max_size=2,asynchronous_request = True, geometry_field='geom') + buggy_layer.styles.append('style for buggy layer') + + # The query for this layer will be sent, then the previous layer will raise an exception before results are read + forced_canceled_layer = mapnik.Layer('this layer will be canceled when an exception stops map rendering') + forced_canceled_layer.datasource = mapnik.PostGIS(dbname=MAPNIK_TEST_DBNAME,table='world_merc', + max_async_connection=2, max_size=2, asynchronous_request = True, geometry_field='geom') + forced_canceled_layer.styles.append('style') + + map1.layers.append(buggy_layer) + map1.layers.append(forced_canceled_layer) + map1.zoom_all() + map2 = mapnik.Map(600,300) + map2.background = mapnik.Color('steelblue') + s = mapnik.Style() + r = mapnik.Rule() + r.symbols.append(mapnik.LineSymbolizer(mapnik.Color('rgb(50%,50%,50%)'),0.1)) + r.symbols.append(mapnik.LineSymbolizer(mapnik.Color('rgb(50%,50%,50%)'),0.1)) + s.rules.append(r) + map2.append_style('style',s) + layer1 = mapnik.Layer('layer1') + layer1.datasource = mapnik.PostGIS(dbname=MAPNIK_TEST_DBNAME,table='world_merc', + max_async_connection=2, max_size=2, asynchronous_request = True, geometry_field='geom') + layer1.styles.append('style') + map2.layers.append(layer1) + map2.zoom_all() + + # We expect this to trigger a PSQL error + try: + mapnik.render_to_file(map1,'world.png', 'png') + # Test must fail if error was not raised just above + eq_(False,True) + except RuntimeError: + pass + # This used to raise an exception before correction of issue 2042 + mapnik.render_to_file(map2,'world2.png', 'png') + + atexit.register(postgis_takedown) if __name__ == "__main__":