Merge pull request #6311 from rouault/fix_6253

netCDF (multidim): workaround crash with using same file in 2 differents threads (each thread with its own dataset object) (fixes #6253)

autotest/gdrivers/netcdf_multidim.py

@@ -2613,3 +2613,19 @@ def create():
     assert delay < 1
 
     gdal.Unlink("tmp/test_netcdf_multidim_read_transposed_bigger_file.nc")
+
+
+def test_netcdf_multidim_var_alldatatypes_opened_twice():
+    """Test https://github.com/OSGeo/gdal/pull/6311"""
+
+    ds1 = gdal.OpenEx("data/netcdf/alldatatypes.nc", gdal.OF_MULTIDIM_RASTER)
+    assert ds1
+    rg1 = ds1.GetRootGroup()
+    assert rg1
+    assert rg1.OpenMDArray("string_var") is not None
+
+    ds2 = gdal.OpenEx("data/netcdf/alldatatypes.nc", gdal.OF_MULTIDIM_RASTER)
+    assert ds2
+    rg2 = ds2.GetRootGroup()
+    assert rg2
+    assert rg2.OpenMDArray("string_var") is not None

frmts/netcdf/netcdfdataset.cpp

@@ -162,6 +162,57 @@ static CPLErr NCDFGetCoordAndBoundVarFullNames( int nCdfId,
 
 CPLMutex *hNCMutex = nullptr;
 
+// Workaround https://github.com/OSGeo/gdal/issues/6253
+// Having 2 netCDF handles on the same file doesn't work in a multi-threaded
+// way. Apparently having the same handle works better (this is OK since
+// we have a global mutex on the netCDF library)
+static std::map<std::string, int> goMapNameToNetCDFId;
+static std::map<int, std::pair<std::string, int>> goMapNetCDFIdToKeyAndCount;
+
+int GDAL_nc_open(const char* pszFilename, int nMode, int* pID)
+{
+    std::string osKey(pszFilename);
+    osKey += "#####";
+    osKey += std::to_string(nMode);
+    auto oIter = goMapNameToNetCDFId.find(osKey);
+    if( oIter == goMapNameToNetCDFId.end() )
+    {
+        int ret = nc_open(pszFilename, nMode, pID);
+        if( ret != NC_NOERR )
+            return ret;
+        goMapNameToNetCDFId[osKey] = *pID;
+        goMapNetCDFIdToKeyAndCount[*pID] = std::pair<std::string, int>(osKey, 1);
+        return ret;
+    }
+    else
+    {
+        *pID = oIter->second;
+        goMapNetCDFIdToKeyAndCount[oIter->second].second ++;
+        return NC_NOERR;
+    }
+}
+
+int GDAL_nc_close(int cdfid)
+{
+    int ret = NC_NOERR;
+    auto oIter = goMapNetCDFIdToKeyAndCount.find(cdfid);
+    if( oIter != goMapNetCDFIdToKeyAndCount.end() )
+    {
+        if( --oIter->second.second == 0 )
+        {
+            ret = nc_close(cdfid);
+            goMapNameToNetCDFId.erase(oIter->second.first);
+            goMapNetCDFIdToKeyAndCount.erase(oIter);
+        }
+    }
+    else
+    {
+        // we can go here if file opened with nc_open_mem() or nc_create()
+        ret = nc_close(cdfid);
+    }
+    return ret;
+}
+
 /************************************************************************/
 /* ==================================================================== */
 /*                         netCDFRasterBand                             */
@@ -2834,7 +2885,7 @@ netCDFDataset::~netCDFDataset()
 #ifdef NCDF_DEBUG
         CPLDebug("GDAL_netCDF", "calling nc_close( %d)", cdfid);
 #endif
-        int status = nc_close(cdfid);
+        int status = GDAL_nc_close(cdfid);
 #ifdef ENABLE_UFFD
         NETCDF_UFFD_UNMAP(pCtx);
 #endif
@@ -7472,7 +7523,7 @@ bool netCDFDataset::GrowDim(int nLayerId, int nDimIdToGrow, size_t nNewSize)
                   eFormat == NCDF_FORMAT_NC4,
                   nLayerId, nDimIdToGrow, nNewSize) )
     {
-        nc_close(new_cdfid);
+        GDAL_nc_close(new_cdfid);
         return false;
     }
 
@@ -7498,15 +7549,15 @@ bool netCDFDataset::GrowDim(int nLayerId, int nDimIdToGrow, size_t nNewSize)
             if( status != NC_NOERR )
             {
                 CPLFree(panGroupIds);
-                nc_close(new_cdfid);
+                GDAL_nc_close(new_cdfid);
                 return false;
             }
             if( !CloneGrp(panGroupIds[i], nNewGrpId,
                           eFormat == NCDF_FORMAT_NC4,
                           nLayerId, nDimIdToGrow, nNewSize) )
             {
                 CPLFree(panGroupIds);
-                nc_close(new_cdfid);
+                GDAL_nc_close(new_cdfid);
                 return false;
             }
         }
@@ -7527,9 +7578,9 @@ bool netCDFDataset::GrowDim(int nLayerId, int nDimIdToGrow, size_t nNewSize)
     }
 #endif
 
-    nc_close(cdfid);
+    GDAL_nc_close(cdfid);
     cdfid = -1;
-    nc_close(new_cdfid);
+    GDAL_nc_close(new_cdfid);
 
     CPLString osOriFilename(osFilename + ".ori");
     if( VSIRename(osFilename, osOriFilename) != 0 ||
@@ -7549,7 +7600,7 @@ bool netCDFDataset::GrowDim(int nLayerId, int nDimIdToGrow, size_t nNewSize)
         CPLFree(pszTemp);
     }
 #endif
-    status = nc_open(osFilenameForNCOpen, NC_WRITE, &cdfid);
+    status = GDAL_nc_open(osFilenameForNCOpen, NC_WRITE, &cdfid);
     NCDF_ERR(status);
     if( status != NC_NOERR )
         return false;
@@ -8281,7 +8332,7 @@ bool netCDFDatasetCreateTempFile( NetCDFFormatEnum eFormat,
         }
     }
 
-    nc_close(nCdfId);
+    GDAL_nc_close(nCdfId);
     return true;
 }
 
@@ -8517,9 +8568,9 @@ GDALDataset *netCDFDataset::Open( GDALOpenInfo *poOpenInfo )
             status2 = nc_open_mem(CPLGetFilename(osFilenameForNCOpen), nMode, static_cast<size_t>(nVmaSize), pVma, &cdfid);
         }
         else
-            status2 = nc_open(osFilenameForNCOpen, nMode, &cdfid);
+            status2 = GDAL_nc_open(osFilenameForNCOpen, nMode, &cdfid);
 #else
-        status2 = nc_open(osFilenameForNCOpen, nMode, &cdfid);
+        status2 = GDAL_nc_open(osFilenameForNCOpen, nMode, &cdfid);
 #endif
     }
     if( status2 != NC_NOERR )
@@ -8604,7 +8655,7 @@ GDALDataset *netCDFDataset::Open( GDALOpenInfo *poOpenInfo )
                      "%s is a netCDF file, but %s is not a variable.",
                      poOpenInfo->pszFilename, osSubdatasetName.c_str());
 
-            nc_close(cdfid);
+            GDAL_nc_close(cdfid);
 #ifdef ENABLE_UFFD
             NETCDF_UFFD_UNMAP(pCtx);
 #endif

frmts/netcdf/netcdfdataset.h

@@ -1126,4 +1126,7 @@ bool netCDFDatasetCreateTempFile( NetCDFFormatEnum eFormat,
                                          VSILFILE* fpSrc );
 #endif
 
+int GDAL_nc_open(const char* pszFilename, int nMode, int* pID);
+int GDAL_nc_close(int cdfid);
+
 #endif

frmts/netcdf/netcdfmultidim.cpp

@@ -508,7 +508,7 @@ netCDFSharedResources::~netCDFSharedResources()
 #ifdef NCDF_DEBUG
         CPLDebug("GDAL_netCDF", "calling nc_close( %d)", m_cdfid);
 #endif
-        int status = nc_close(m_cdfid);
+        int status = GDAL_nc_close(m_cdfid);
         NCDF_ERR(status);
     }
 
@@ -3245,6 +3245,7 @@ std::vector<GUInt64> netCDFVariable::GetBlockSize() const
     // We add 1 to the dimension count, for 2D char variables that we
     // expose as a 1D variable.
     std::vector<size_t> anTemp(1 + nDimCount);
+    CPLMutexHolderD(&hNCMutex);
     nc_inq_var_chunking(m_gid, m_varid, &nStorageType, &anTemp[0]);
     if( nStorageType == NC_CHUNKED )
     {
@@ -4007,10 +4008,10 @@ GDALDataset *netCDFDataset::OpenMultiDim( GDALOpenInfo *poOpenInfo )
             status2 = nc_open_mem(CPLGetFilename(osFilenameForNCOpen), nMode, static_cast<size_t>(nVmaSize), pVma, &cdfid);
         }
         else
-          status2 = nc_open(osFilenameForNCOpen, nMode, &cdfid);
+          status2 = GDAL_nc_open(osFilenameForNCOpen, nMode, &cdfid);
         poSharedResources->m_pUffdCtx = pCtx;
 #else
-        status2 = nc_open(osFilenameForNCOpen, nMode, &cdfid);
+        status2 = GDAL_nc_open(osFilenameForNCOpen, nMode, &cdfid);
 #endif
     }
     if( status2 != NC_NOERR )