Allocate OgrPool singleton on the heap

[strk]

Closes #14176

[strk]

See http://hub.qgis.org/issues/14176 for what this fixes

[wonder-sk]

Hmm I think we should have one way how to implement our singletons - we used to have singletons on heap, even singletons derived from a template class, ended up with static variables... not sure if we should take another round :-)

If I understand correctly, this solves an issue by not calling the destructor... maybe a better approach would be to have QgsApplication::exitQgis() taking care of singleton cleanup while the app environment is alive - singletons that may need some cleanup could register for cleanup, ideally also specifying dependencies on other singletons (if any) to make sure they are destroyed in the correct order.

[strk]

I agree QgsApplication::exitQgis could deal with carefully sorted deinitialization of singletons, but this means to continue using heap, like QgsProviderRegistry::instance and QgsAuthManager::instance do.

Do we agree on that part ?

There are so_many singletons in qgis that my head spins :-/

[strk]

Isn't QgsAuthManager a pretty recent one, btw ? Speaking about the "we ended up using static variables"...

[m-kuhn]

I think having this on the heap is a good thing.

Since it's used in the provider it probably shouldn't be directly called from exitQgis but in cleanupProvider ( qgsogrprovider.cpp:2870) which IIRC is called via provider registry deinitialization.

QGISEXTERN void cleanupProvider()
{
  OGRCleanupAll();
}

The problems we had/have with singletons is that they internally have dependencies. For QGIS 3 we can look into Q_GLOBAL_STATIC and moving some singletons to being children of other singletons instead of being singletons themselves (e.g. qgsmaplayerregistry should always have been a child of qgsproject)

[strk]

+1 on reducing singleton.

For this specific case I see the problem is with OGR and GDAL "providers" both using the same library and both cleaning up.

First GDAL provider calls GDALDestroyDriverManager()
then OGR provider calls OGRCleanupAll(), which ends up calling OGR_DS_Destroy which ends up addressing already-released memory (released by GDALDestroyDriverManager, if what valgrind says is correct).

@rouault does the above sound ?

[wonder-sk]

One more issue with connection pool singletons is that the lazy initialization of the static instance in a function is not guaranteed to be thread safe (according to [1] this is the case with MSVC). Connection pools are meant to be called from multiple threads and IIRC there were rare concurrency issues due to that (with postgres provider).

[1] http://stackoverflow.com/questions/1962880/is-c-static-member-variable-initialization-thread-safe

[strk]

Could we have a global initializer to par with "cleanupProvider" ? So to ensure provider initialization happens from main thread, and can initialize its pools ?

[rouault]

It is clearly illegal to call OGR_DS_Destroy() after GDALDestroyDriverManager() as done in stack trace http://hub.qgis.org/issues/14176#note-2 . GDALDestroyDriverManager() takes care of force closing still opened datasets (among other things). So calling OGR_DS_Destroy() afterwards will result in operating on a freed pointer. The fact that some people sees an issue and others not must come from people using GDAL 1.X (in which the GDAL and OGR worlds are separated) and others GDAL 2.X (where GDAL and OGR drivers are unified, so GDALDestroyDriverManager() also affects OGR drivers & datasources).

Calling OGRCleanupAll() after GDALDestroyDriverManager() should be OK though (but in the stack trace OGRCleanupAll() isn't called). In GDAL 2.X, OGRCleanupAll() is just an alias of GDALDestroyDriverManager(), and it is safe to call it twice.

I'd note that relying on GDALDestroyDriverManager() / OGRCleanupAll() to clause remaining opened datasets is a tiny bit risky, and it would certainly be better to explicitly close them before.

[m-kuhn]

Q_GLOBAL_STATIC should resolve the threading issues and allows to check if it has already been freed.

Maybe it's possible to port this code to our 4.8 codebase and we can remove it again once Qt 5.1 is the minimum version?

[strk]

So to summarize:

1. Calling GDALDestroyDriverManager() is not acceptable unless we can guarantee the OGRConnectionPool was destroyed
2. OGRConnectionPool is in a different provider form the GDAL one

It occurs to me that we need a way to control order in which provider's "cleanup" functions are invoked.

[m-kuhn]

On 02/02/2016 12:50 PM, Sandro Santilli wrote:

So to summarize:

1. Calling GDALDestroyDriverManager() is not acceptable unless we can
   guarantee the OGRConnectionPool was destroyed
2. OGRConnectionPool is in a /different/ provider form the GDAL one

It occurs to me that we need a way to control order in which
provider's "cleanup" functions are invoked.

Or that gdal initialization and de-initialization should happen from the
main app instead of the providers.

Matthias Kuhn
OPENGIS.ch - https://www.opengis.ch
Spatial • (Q)GIS • PostGIS • Open Source

[strk]

In any case it must be heap-allocation or we can't control much, so that step is needed anyway.

I've pushed a rebased version with added commit to: (1) cleanup pool in OGR cleanupProvider and (2) disable call to GDALDestroyDriverManager

Valgrind doesn't show leaks from GDAL, at least while running qgis_analyzertest

[rouault]

Or that gdal initialization and de-initialization should happen from the main app instead of the providers.

+1

[strk]

Good point about having GDAL/OGR deinitialization happen at app time, given it already takes care of calling GDALAllRegister() and OGRRegisterAll() ! Will commit that shortly

[strk]

we could maybe also take a step further and drop any call to RegisterAll (ogr/gdal) from any other place (there are currently many occurrences of that call)

[rouault]

we could maybe also take a step further and drop any call to RegisterAll (ogr/gdal) from any other place (there are currently many occurrences of that call)

Perhaps they might still be useful for code not using QgsApplication ?

[wonder-sk]

Could we have a global initializer to par with "cleanupProvider" ? So to ensure provider initialization happens from main thread, and can initialize its pools ?

Sounds good to me.

Also the GDAL/OGR init/cleanup in QgsApplication looks safer.

Would be great if the cleanup of singleton would also set the pointer to zero, so we can detect use of a dangling pointer after destruction of the singleton.

[strk]

Pointer set to zero with the last commit

[strk]

Squash-pushed as single commit 0512538