Skip to content

Commit 203bb0f

Browse files
rouaultrouault
authored
Merge pull request #14263 from rouault/fix_14262
Fix threading related issues related to warping in GTI driver
2 parents b78bdf7 + 37bd77b commit 203bb0f

File tree

7 files changed

+111
-13
lines changed

7 files changed

+111
-13
lines changed

‎alg/gdalwarpkernel.cpp‎

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -495,11 +495,17 @@ static CPLErr GWKRun(GDALWarpKernel *poWK, const char *pszFuncName,
495495

496496
bool bStopFlag;
497497
{
498-
std::unique_lock<std::mutex> lock(psThreadData->mutex);
498+
{
499+
// Important: do not run the SubmitJob() loop under the mutex
500+
// because in some cases (typically if the current thread has been
501+
// created by the GDAL global thread pool), the task will actually
502+
// be run synchronously by SubmitJob(), and as it tries to acquire
503+
// the mutex, that would result in a dead-lock
504+
std::unique_lock<std::mutex> lock(psThreadData->mutex);
499505

500-
psThreadData->nTotalThreadCountForThisRun = nThreads;
501-
// coverity[missing_lock]
502-
psThreadData->nCurThreadCountForThisRun = 0;
506+
psThreadData->nTotalThreadCountForThisRun = nThreads;
507+
psThreadData->nCurThreadCountForThisRun = 0;
508+
}
503509

504510
// Start jobs.
505511
for (int i = 0; i < nThreads; ++i)
@@ -514,6 +520,7 @@ static CPLErr GWKRun(GDALWarpKernel *poWK, const char *pszFuncName,
514520
/* Report progress. */
515521
/* --------------------------------------------------------------------
516522
*/
523+
std::unique_lock<std::mutex> lock(psThreadData->mutex);
517524
if (poWK->pfnProgress != GDALDummyProgress)
518525
{
519526
while (psThreadData->counter < nDstYSize)
@@ -530,6 +537,17 @@ static CPLErr GWKRun(GDALWarpKernel *poWK, const char *pszFuncName,
530537
break;
531538
}
532539
}
540+
541+
if (!psThreadData->stopFlag)
542+
{
543+
if (!poWK->pfnProgress(poWK->dfProgressBase +
544+
poWK->dfProgressScale,
545+
"", poWK->pProgress))
546+
{
547+
CPLError(CE_Failure, CPLE_UserInterrupt, "User terminated");
548+
psThreadData->stopFlag = true;
549+
}
550+
}
533551
}
534552

535553
bStopFlag = psThreadData->stopFlag;

‎autotest/alg/warp.py‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -937,7 +937,9 @@ def test_warp_30():
937937
cbk_user_data,
938938
) # Progress callback user data
939939

940-
with gdal.config_option("GDAL_NUM_THREADS", "2"), pytest.raises(Exception):
940+
with gdal.config_option("GDAL_NUM_THREADS", "2"), pytest.raises(
941+
Exception, match="User terminated"
942+
):
941943
gdal.ReprojectImage(
942944
src_ds,
943945
dst_ds,

‎autotest/cpp/test_cpl.cpp‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
#include "cpl_compressor.h"
2323
#include "cpl_enumerate.h"
2424
#include "cpl_error.h"
25+
#include "cpl_error_internal.h"
2526
#include "cpl_float.h"
2627
#include "cpl_hash_set.h"
2728
#include "cpl_levenshtein.h"
@@ -1253,6 +1254,7 @@ TEST_F(test_cpl, CPLSetErrorHandler)
12531254
ASSERT_EQ(gbGotError, false);
12541255
gbGotError = false;
12551256
CPLSetErrorHandler(oldHandler);
1257+
CPLSetCurrentErrorHandlerCatchDebug(true);
12561258

12571259
CPLPushErrorHandler(myErrorHandler);
12581260
CPLSetCurrentErrorHandlerCatchDebug(FALSE);
@@ -1274,6 +1276,45 @@ TEST_F(test_cpl, CPLSetErrorHandler)
12741276
CPLSetErrorHandler(oldHandler);
12751277
}
12761278

1279+
TEST_F(test_cpl, global_error_handler_and_CPLSetCurrentErrorHandlerCatchDebug)
1280+
{
1281+
static bool gbGotDebugMessage = false;
1282+
static bool gbGotExpectedUserData = false;
1283+
1284+
struct MyStruct
1285+
{
1286+
static void CPL_STDCALL myErrorHandler(CPLErr eErr, CPLErrorNum,
1287+
const char *msg)
1288+
{
1289+
if (CPLGetErrorHandlerUserData() == &gbGotExpectedUserData)
1290+
{
1291+
gbGotExpectedUserData = true;
1292+
}
1293+
if (eErr == CE_Debug && strcmp(msg, "TEST: my debug message") == 0)
1294+
{
1295+
gbGotDebugMessage = true;
1296+
}
1297+
}
1298+
};
1299+
1300+
CPLErrorHandler oldHandler =
1301+
CPLSetErrorHandlerEx(MyStruct::myErrorHandler, &gbGotExpectedUserData);
1302+
1303+
CPLErrorAccumulator oAccumulator;
1304+
{
1305+
auto scopedAccumulator = oAccumulator.InstallForCurrentScope();
1306+
CPL_IGNORE_RET_VAL(scopedAccumulator);
1307+
1308+
CPLConfigOptionSetter oSetter("CPL_DEBUG", "ON", false);
1309+
CPLDebug("TEST", "my debug message");
1310+
}
1311+
1312+
EXPECT_TRUE(gbGotExpectedUserData);
1313+
EXPECT_TRUE(gbGotDebugMessage);
1314+
1315+
CPLSetErrorHandler(oldHandler);
1316+
}
1317+
12771318
/************************************************************************/
12781319
/* CPLString::replaceAll() */
12791320
/************************************************************************/

‎port/cpl_conv.cpp‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1718,7 +1718,9 @@ const char *CPL_STDCALL CPLGetConfigOption(const char *pszKey,
17181718
if (gbIgnoreEnvVariables)
17191719
{
17201720
const char *pszEnvVar = getenv(pszKey);
1721-
if (pszEnvVar != nullptr)
1721+
// Skipping for CPL_DEBUG to avoid infinite recursion since CPLvDebug()
1722+
// calls CPLGetConfigOption()...
1723+
if (pszEnvVar != nullptr && !EQUAL(pszKey, "CPL_DEBUG"))
17221724
{
17231725
CPLDebug("CPL",
17241726
"Ignoring environment variable %s=%s because of "

‎port/cpl_error.cpp‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -260,7 +260,13 @@ static void ApplyErrorHandler(CPLErrorContext *psCtx, CPLErr eErrClass,
260260
{
261261
if (pfnErrorHandler != nullptr)
262262
{
263+
// Make sure to empty the thread-specific handler stack,
264+
// otherwise the global error handler might get unrelated
265+
// data when calling CPLGetErrorHandlerUserData()
266+
CPLErrorHandlerNode *psCurNodeBackup = psCtx->psHandlerStack;
267+
psCtx->psHandlerStack = nullptr;
263268
pfnErrorHandler(eErrClass, err_no, pszMessage);
269+
psCtx->psHandlerStack = psCurNodeBackup;
264270
}
265271
}
266272
else /* if( eErrClass == CE_Debug ) */
@@ -1576,6 +1582,16 @@ CPLErrorStateBackuper::~CPLErrorStateBackuper()
15761582

15771583
/*! @cond Doxygen_Suppress */
15781584

1585+
/************************************************************************/
1586+
/* CPLErrorAccumulator::Context::Context() */
1587+
/************************************************************************/
1588+
1589+
CPLErrorAccumulator::Context::Context(CPLErrorAccumulator &sAccumulator)
1590+
{
1591+
CPLPushErrorHandlerEx(CPLErrorAccumulator::Accumulator, &sAccumulator);
1592+
CPLSetCurrentErrorHandlerCatchDebug(false);
1593+
}
1594+
15791595
/************************************************************************/
15801596
/* CPLErrorAccumulator::Context::~Context() */
15811597
/************************************************************************/
@@ -1591,9 +1607,7 @@ CPLErrorAccumulator::Context::~Context()
15911607

15921608
CPLErrorAccumulator::Context CPLErrorAccumulator::InstallForCurrentScope()
15931609
{
1594-
CPLPushErrorHandlerEx(CPLErrorAccumulator::Accumulator, this);
1595-
CPLSetCurrentErrorHandlerCatchDebug(false);
1596-
return CPLErrorAccumulator::Context();
1610+
return CPLErrorAccumulator::Context(*this);
15971611
}
15981612

15991613
/************************************************************************/

‎port/cpl_error_internal.h‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,18 @@ class CPL_DLL CPLErrorAccumulator
8383
*/
8484
struct CPL_DLL Context
8585
{
86+
/*! @cond Doxygen_Suppress */
8687
~Context();
88+
89+
Context(const Context &) = delete;
90+
Context &operator=(const Context &) = delete;
91+
Context(Context &&) = delete;
92+
Context &operator=(Context &&) = delete;
93+
94+
private:
95+
friend class CPLErrorAccumulator;
96+
explicit Context(CPLErrorAccumulator &sAccumulator);
97+
/*! @endcond Doxygen_Suppress */
8798
};
8899

89100
/** Install a temporary error handler that will store errors and warnings.

‎swig/include/cpl.i‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -71,14 +71,22 @@ void CPL_STDCALL PyCPLErrorHandler(CPLErr eErrClass, CPLErrorNum err_no, const c
7171
return;
7272
}
7373

74-
void* user_data = CPLGetErrorHandlerUserData();
74+
PyObject* callable = (PyObject*)CPLGetErrorHandlerUserData();
7575
PyObject *psArgs;
7676

7777
SWIG_PYTHON_THREAD_BEGIN_BLOCK;
7878

79-
psArgs = Py_BuildValue("(iis)", eErrClass, err_no, pszErrorMsg );
80-
PyObject_CallObject( (PyObject*)user_data, psArgs);
81-
Py_XDECREF(psArgs);
79+
if (!PyCallable_Check(callable))
80+
{
81+
PyErr_SetString( PyExc_RuntimeError,
82+
"PyCPLErrorHandler: Critical error: callback is not callable" );
83+
}
84+
else
85+
{
86+
psArgs = Py_BuildValue("(iis)", eErrClass, err_no, pszErrorMsg );
87+
PyObject_CallObject( callable, psArgs);
88+
Py_XDECREF(psArgs);
89+
}
8290

8391
SWIG_PYTHON_THREAD_END_BLOCK;
8492
}
@@ -115,7 +123,9 @@ void CPL_STDCALL PyCPLErrorHandler(CPLErr eErrClass, CPLErrorNum err_no, const c
115123
void* user_data = CPLGetErrorHandlerUserData();
116124
if( user_data != NULL )
117125
{
126+
SWIG_PYTHON_THREAD_BEGIN_BLOCK;
118127
Py_XDECREF((PyObject*)user_data);
128+
SWIG_PYTHON_THREAD_END_BLOCK;
119129
}
120130
CPLPopErrorHandler();
121131
}

0 commit comments

Comments
 (0)