Better cleanup in gdaldrivermanager.cpp:GDALDestroyDriverManager()

[Kurt Schwehr]

I have an non-public case where two threads end up calling GDALDestroyDriverManager() and the world comes apart. A partial fix would be to set the driver manager pointer to be a nullptr, but there would still be a race condition. I believe that by adding a 2nd mutex and setting the pointer to nullptr, that code has a better chance of surviving in a sane state (where it will likely be reporting about a failed test or other bad assumption). If there is another active thread that has a pointer to the driver manager, then things will still come unhinged.

This change will be for after the GDAL 2.1 branch is created.

Thoughts on this patch?

gdaldrivermanager.cpp

@@ -52 +53 @@
 
 static volatile GDALDriverManager        *poDM = NULL;
 static CPLMutex *hDMMutex = NULL;
+static CPLMutex *hDestroyDriverManagerMutex = NULL;
 
 CPLMutex** GDALGetphDMMutex() { return &hDMMutex; }
 
@@ -81 +83 @@
 
         if( poDM == NULL )
             poDM = new GDALDriverManager();
+
+        if( hDestroyDriverManagerMutex == NULL )
+        {
+            // Create the mutex as a side effect of holding it.
+            CPLMutexHolder oHolder( &hDestroyDriverManagerMutex, 0.1,
+                                    __FILE__, __LINE__ );
+        }
     }
 
     CPLAssert( NULL != poDM );
@@ -842 +851 @@
     // needs to be reacquired within the destructor during driver
     // deregistration.
     if( poDM != NULL )
+    {
+        CPLMutexHolderD( &hDestroyDriverManagerMutex );
         delete poDM;
+        poDM = NULL;
+    }
 }

[Even Rouault]

One issue is that the hDestroyDriverManagerMutex would not be destroyed, and that it cannot be destroyed since the destructor of GDALDriverManager is responsible to destroy the mutexes... A static mutex would be ideal for that (and not only here), but it doesn't look like it is (easily) doable on Windows

[Kurt Schwehr]

Since this is mostly working for people, what about doing a solution just for >= C++11 with lock_guard? I have not tried this yet.

​http://en.cppreference.com/w/cpp/thread/lock_guard

#if __cplusplus >= 201103L
#include <mutex>

static std::mutex oDeleteMutex;
#endif

...

void CPL_STDCALL GDALDestroyDriverManager( void )

{
    // THREADSAFETY: We would like to lock the mutex here, but it
    // needs to be reacquired within the destructor during driver
    // deregistration.

#if __cplusplus >= 201103L
    std::lock_guard<std::mutex> oLock(oDeleteMutex);
#endif

    if( poDM != NULL )
    {
        delete poDM;
        poDM = NULL;
    }
}

[Even Rouault]

Since this is mostly working for people, what about doing a solution just for >= C++11 with lock_guard?

Looks good.

[Kurt Schwehr]

I propose adding this to cpl_port.h so that we can stop the insanity of the long checks.

#if defined(__cplusplus)
#  if  __cplusplus >= 201103L
#    define HAVE_CPP11 1
#  endif
/* TODO(schwehr): What are the correct tests for C++ 14 and 17? */
#endif

[Kurt Schwehr]

Maybe HAVE_CXX11 would be better? COMPILER_HAS_CPP11_SUPPORT that I found searching the interwebs not good.

CPP always makes me think C-PreProcessor.

I'm not finding anything particularly nice. e.g.

​https://github.com/saschazelzer/DUNE-superbuild/blob/master/cmake/FindCXX11Features.cmake

[Even Rouault]

HAVE_CXX11 looks good

[Kurt Schwehr]

r33894 adds HAVE_CXX11 to cpl_ports.h

[Even Rouault]

This ticket has been automatically closed because Trac is no longer used for GDAL bug tracking, since the project has migrated to GitHub. If you believe this ticket is still valid, you may file it to ​https://github.com/OSGeo/gdal/issues if it is not already reported there.