Don't crash providers when destrucing with fake open connection

m-kuhn · m-kuhn · commit 3dc832a32b05 · 2015-12-22T09:12:32.000+01:00
diff --git a/src/core/qgsfeatureiterator.cpp b/src/core/qgsfeatureiterator.cpp
@@ -120,6 +120,7 @@ class QgsExpressionSorter
 QgsAbstractFeatureIterator::QgsAbstractFeatureIterator( const QgsFeatureRequest& request )
     : mRequest( request )
     , mClosed( false )
+    , mZombie( false )
     , refs( 0 )
     , mFetchedCount( 0 )
     , mGeometrySimplifier( nullptr )
@@ -152,8 +153,8 @@ bool QgsAbstractFeatureIterator::nextFeature( QgsFeature& f )
     else
     {
       dataOk = false;
-      // don't call close, the provider connection has already been closed
-      mClosed = true;
+      // even the zombie dies at this point...
+      mZombie = false;
     }
   }
   else
@@ -289,7 +290,8 @@ void QgsAbstractFeatureIterator::setupOrderBy( const QList<QgsFeatureRequest::Or
 
     mFeatureIterator = mCachedFeatures.constBegin();
     mUseCachedFeatures = true;
-    mClosed = false;
+    // The real iterator is closed, we are only serving cached features
+    mZombie = true;
   }
 }
 
diff --git a/src/core/qgsfeatureiterator.h b/src/core/qgsfeatureiterator.h
@@ -89,7 +89,17 @@ class CORE_EXPORT QgsAbstractFeatureIterator
     /** Set to true, as soon as the iterator is closed. */
     bool mClosed;
 
+    /**
+     * A feature iterator may be closed already but still be serving features from the cache.
+     * This is done when we serve features which have been pre-fetched and the order by has
+     * been locally sorted.
+     * In such a scenario, all resources have been released (mClosed is true) but the deads
+     * are still alive.
+     */
+    bool mZombie;
+
     //! reference counting (to allow seamless copying of QgsFeatureIterator instances)
+    //! TODO QGIS3: make this private
     int refs;
     void ref(); //!< add reference
     void deref(); //!< remove reference, delete if refs == 0
@@ -247,7 +257,7 @@ inline bool QgsFeatureIterator::close()
 
 inline bool QgsFeatureIterator::isClosed() const
 {
-  return mIter ? mIter->mClosed : true;
+  return mIter ? mIter->mClosed && !mIter->mZombie : true;
 }
 
 inline bool operator== ( const QgsFeatureIterator &fi1, const QgsFeatureIterator &fi2 )
diff --git a/src/providers/ogr/qgsogrfeatureiterator.cpp b/src/providers/ogr/qgsogrfeatureiterator.cpp
@@ -243,7 +243,7 @@ bool QgsOgrFeatureIterator::rewind()
 
 bool QgsOgrFeatureIterator::close()
 {
-  if ( mClosed )
+  if ( !mConn )
     return false;
 
   iteratorClosed();
@@ -253,7 +253,9 @@ bool QgsOgrFeatureIterator::close()
     OGR_DS_ReleaseResultSet( mConn->ds, ogrLayer );
   }
 
-  QgsOgrConnPool::instance()->releaseConnection( mConn );
+  if ( mConn )
+    QgsOgrConnPool::instance()->releaseConnection( mConn );
+
   mConn = nullptr;
 
   mClosed = true;
diff --git a/src/providers/postgres/qgspostgresfeatureiterator.cpp b/src/providers/postgres/qgspostgresfeatureiterator.cpp
@@ -313,7 +313,7 @@ bool QgsPostgresFeatureIterator::rewind()
 
 bool QgsPostgresFeatureIterator::close()
 {
-  if ( mClosed )
+  if ( !mConn )
     return false;
 
   mConn->closeCursor( mCursorName );
diff --git a/src/providers/spatialite/qgsspatialitefeatureiterator.cpp b/src/providers/spatialite/qgsspatialitefeatureiterator.cpp
@@ -235,7 +235,7 @@ bool QgsSpatiaLiteFeatureIterator::rewind()
 
 bool QgsSpatiaLiteFeatureIterator::close()
 {
-  if ( mClosed )
+  if ( !mHandle )
     return false;
 
   iteratorClosed();

Original file line number	Diff line number	Diff line change
`@@ -120,6 +120,7 @@ class QgsExpressionSorter`
`120`	`120`	`QgsAbstractFeatureIterator::QgsAbstractFeatureIterator( const QgsFeatureRequest& request )`
`121`	`121`	`: mRequest( request )`
`122`	`122`	`, mClosed( false )`
	`123`	`+ , mZombie( false )`
`123`	`124`	`, refs( 0 )`
`124`	`125`	`, mFetchedCount( 0 )`
`125`	`126`	`, mGeometrySimplifier( nullptr )`
`@@ -152,8 +153,8 @@ bool QgsAbstractFeatureIterator::nextFeature( QgsFeature& f )`
`152`	`153`	`else`
`153`	`154`	`{`
`154`	`155`	`dataOk = false;`
`155`		`- // don't call close, the provider connection has already been closed`
`156`		`- mClosed = true;`
	`156`	`+ // even the zombie dies at this point...`
	`157`	`+ mZombie = false;`
`157`	`158`	`}`
`158`	`159`	`}`
`159`	`160`	`else`
`@@ -289,7 +290,8 @@ void QgsAbstractFeatureIterator::setupOrderBy( const QList<QgsFeatureRequest::Or`
`289`	`290`
`290`	`291`	`mFeatureIterator = mCachedFeatures.constBegin();`
`291`	`292`	`mUseCachedFeatures = true;`
`292`		`- mClosed = false;`
	`293`	`+ // The real iterator is closed, we are only serving cached features`
	`294`	`+ mZombie = true;`
`293`	`295`	`}`
`294`	`296`	`}`
`295`	`297`
Original file line number	Diff line number	Diff line change
`@@ -313,7 +313,7 @@ bool QgsPostgresFeatureIterator::rewind()`
`313`	`313`
`314`	`314`	`bool QgsPostgresFeatureIterator::close()`
`315`	`315`	`{`
`316`		`- if ( mClosed )`
	`316`	`+ if ( !mConn )`
`317`	`317`	`return false;`
`318`	`318`
`319`	`319`	`mConn->closeCursor( mCursorName );`
Original file line number	Diff line number	Diff line change
`@@ -235,7 +235,7 @@ bool QgsSpatiaLiteFeatureIterator::rewind()`
`235`	`235`
`236`	`236`	`bool QgsSpatiaLiteFeatureIterator::close()`
`237`	`237`	`{`
`238`		`- if ( mClosed )`
	`238`	`+ if ( !mHandle )`
`239`	`239`	`return false;`
`240`	`240`
`241`	`241`	`iteratorClosed();`