Opened 8 years ago
Closed 5 years ago
#6432 closed defect (wontfix)
Better cleanup in gdaldrivermanager.cpp:GDALDestroyDriverManager()
Reported by: | Kurt Schwehr | Owned by: | Kurt Schwehr |
---|---|---|---|
Priority: | normal | Milestone: | closed_because_of_github_migration |
Component: | default | Version: | svn-trunk |
Severity: | normal | Keywords: | threading mutex |
Cc: |
Description
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
diff -w -u a/gdaldrivermanager.cpp b/gdaldrivermanager.cpp
a b 52 53 53 54 static volatile GDALDriverManager *poDM = NULL; 54 55 static CPLMutex *hDMMutex = NULL; 56 static CPLMutex *hDestroyDriverManagerMutex = NULL; 55 57 56 58 CPLMutex** GDALGetphDMMutex() { return &hDMMutex; } 57 59 … … 81 83 82 84 if( poDM == NULL ) 83 85 poDM = new GDALDriverManager(); 86 87 if( hDestroyDriverManagerMutex == NULL ) 88 { 89 // Create the mutex as a side effect of holding it. 90 CPLMutexHolder oHolder( &hDestroyDriverManagerMutex, 0.1, 91 __FILE__, __LINE__ ); 92 } 84 93 } 85 94 86 95 CPLAssert( NULL != poDM ); … … 842 851 // needs to be reacquired within the destructor during driver 843 852 // deregistration. 844 853 if( poDM != NULL ) 854 { 855 CPLMutexHolderD( &hDestroyDriverManagerMutex ); 845 856 delete poDM; 857 poDM = NULL; 858 } 846 859 }
Change History (8)
comment:1 by , 8 years ago
follow-up: 3 comment:2 by , 8 years ago
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; } }
comment:3 by , 8 years ago
Since this is mostly working for people, what about doing a solution just for >= C++11 with lock_guard?
Looks good.
comment:4 by , 8 years ago
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
comment:5 by , 8 years ago
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
comment:8 by , 5 years ago
Milestone: | → closed_because_of_github_migration |
---|---|
Resolution: | → wontfix |
Status: | new → closed |
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.
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