Abort pending postgis connections when exception occurs

- patch from @abonnasseau
  - closes #2042
  - refs #2069

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();
             }
         }

plugins/input/postgis/connection.hpp

@@ -46,7 +46,8 @@ class Connection
 public:
     Connection(std::string const& connection_str,boost::optional<std::string> 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;
     }
 };
 

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__":