Support of user-data in CPLErrorHandler

[mloskot]

Currently, CPLErrorHandler is a dumb stateless function call that causes certain class of inconveniences and difficulties to users. Especially C++ users who want to make use of...C++ concepts like functors.

Here is a simple patch that tries to deal with imperfection of errors callback mechanism in GDAL. Perhaps it could be considered for GDAL 2.0.

With this patch, users can easily use apply trampoline (thunk) called to achieve flexibility in callbacks dispatching.

[mloskot]

Patch with user-data support for CPLErrorHandler

[mloskot]

Simple example presenting use of different types of erorr handlers

[mloskot]

Another example

[hobu]

Mateusz,

The current patch as it exists is unlikely to make it into GDAL because it changes the function signature of the callback. I propose to adapt your patch implement the following:

Add the following methods that can take in the userdata pointer. These methods will be called by the existing CPLPushErrorHandler and CPLSetErrorHandler methods with NULL as their pUserData member.

void CPL_DLL CPL_STDCALL CPLPushErrorHandlerWithData( CPLErrorHandler, void* );
CPLErrorHandler CPL_DLL CPL_STDCALL CPLSetErrorHandlerWithData(CPLErrorHandler, void*);

Add the following method to get the pUserData for the current error context. This is essentially like exiting CPLGetLastErrorNo and CPLGetLastErrorType methods.

void* CPL_STDCALL CPLGetErrorUserData(void);

Finally, we will add the void* pUserData to the CPLErrorHandlerNode, which will be updated by the functions above

typedef struct errHandler
{
    struct errHandler   *psNext;
    void                *pUserData;
    CPLErrorHandler     pfnHandler;
} CPLErrorHandlerNode;

[hobu]

I'd be interested in any comments from Frank and Even before committing. I don't think my patch is too disruptive, and it only adds a few more C API items.

[rouault]

[20:49] <EvenR> hobu: looking at your patch. I'm not sure what CPLGetErrorUserData() will do if CPLSetErrorHandlerWithData() was used. I guess it will crash
[20:49] <EvenR> otherwise looks indeed pretty non disruptive
[20:59] <mloskot> EvenR: CPLGetErrorUserData will crash indeed, unless it set/push is distinguished and proper user data pulled
[21:05] <EvenR> it should likely do : return (void*) psCtx->psHandlerStack ? psCtx->psHandlerStack->pUserData : pErrorHandlerUserData;
[21:06] <mloskot> y

[hobu]

latest patch does as described and changes the name of CPLGetErrorUserData to CPLGetLastErrorUserData to follow the naming convention of the other CPLGetLastError* method names.

[hobu]

The latest attachment implements Adam's proposal, with documentation.

[rouault]

1) I believe (but have not verified by testing) that the 2 wrappers functions should be declared as CPL_STDCALL so that it works properly on Windows. (could also be tagged as static by the way)

2) CPLSetErrorHandlerEx() returns the previously installed error handler, but it might be confusing to the user, because if the previous error handler was also installed with CPLSetErrorHandlerEx(), this error handler will be CPLSetErrorHandlerExWrapper() that the user has no idea it exists. But worse : the wrapper callback without the associated user-data is likely useless. I believe that the intent of returning the previously installed error handler was to be able to restore it afterwards (or chain to it from your new error handler ?), which is impossible with this implementation. I'm not sure what can be done : perhaps we should not try to address this use case and CPLSetErrorHandlerEx() should not try to return anything ? Or use the following which is - I'm well aware of it - not very pretty :

/** 
 * Install a custom global error handler with user context data 
 * 
 * This method is essentially CPLSetErrorHandler() with the addition of  
 * user context data. See its docs for more details. 
 *
 * Note about ppfnRetOldErrorHandler, ppfnRetOldErrorHandlerEx and ppRetOldUserData :
 * after executing this function, *ppfnRetOldErrorHandler and (*ppfnRetOldErrorHandlerEx,
 * *ppRetOldUserData) are exclusive. Which means that if *ppfnRetOldErrorHandler is non-NULL,
 * *ppfnRetOldErrorHandlerEx and *ppRetOldUserData will be NULL. Similarly if 
 * *ppfnRetOldErrorHandlerEx is non-NULL, *ppfnRetOldErrorHandler will be NULL.
 * 
 * @param pfnErrorHandlerExNew new error handler function. 
 * @param pUserData user context data 
 * @param ppfnRetOldErrorHandler a pointer to a location where to store a previous error handler installed with CPLSetErrorHandler(). Must not be NULL
 * @param ppfnRetOldErrorHandlerEx a pointer to a location where to store a previous (extended) error handler installed with CPLSetErrorHandlerEx(). Must not be NULL
 * @param ppRetOldUserData a pointer to a location where to store a previous user data associated with an (extended) error handler installed with CPLSetErrorHandlerEx(). Must not be NULL
 */  
void CPL_STDCALL CPLSetErrorHandlerEx( CPLErrorHandlerEx pfnErrorHandlerExNew,  
                                       void *pUserData,
                                       CPLErrorHandler* ppfnRetOldErrorHandler,
                                       CPLErrorHandlerEx* ppfnRetOldErrorHandlerEx,
                                       void* ppRetOldUserData)  
{  
    CPLErrorContext *psCtx = CPLGetErrorContext();

    if( psCtx->psHandlerStack != NULL )
    {
        CPLDebug( "CPL", 
                  "CPLSetErrorHandlerEx() called with an error handler on\n"
                  "the local stack.  New error handler will not be used immediately.\n" );
    }

    {
        CPLMutexHolderD( &hErrorMutex );

        CPLErrorHandler pfnOldHandler = pfnErrorHandler;
        CPLErrorHandlerEx pfnOldHandlerEx = pfnErrorHandlerEx;
        void* pOldErrorHandlerUserData = pErrorHandlerUserData;
        
        if( pfnErrorHandler == NULL )
            pfnErrorHandler = CPLDefaultErrorHandler;
        else
        {
            pfnErrorHandler = &CPLSetErrorHandlerExWrapper;
            pErrorHandlerUserData = pUserData;
        }

        if( pfnOldHandler == &CPLSetErrorHandlerExWrapper )
        {
            *ppfnRetOldErrorHandler = NULL;
            *ppfnRetOldErrorHandlerEx = pfnOldHandlerEx;
            *ppRetOldUserData = pOldErrorHandlerUserData;
        }
        else
        {
            *ppfnRetOldErrorHandler = pfnOldHandler;
            *ppfnRetOldErrorHandlerEx = NULL;
            *ppRetOldUserData = NULL;
        }
    }
} 

3) CPLSetErrorHandlerEx() as implemented in 4295-error-handling-nowak-with-docs.patch is racy in a MT context. Should likely be fully protected by CPLMutexHolderD( &hErrorMutex ) as done in CPLSetErrorHandler(). (Note: my implementation inlined in 2) is not racy)

[hobu]

Even,

http://trac.osgeo.org/gdal/attachment/ticket/4295/4295-error-handling-hobu-documentation-fix-CPLGetErrorUserData.patch does not have these deficiencies for CPLSetErrorHandler, even if the API is a little clunkier. Shall we choose that one?

[mloskot]

For the record, Howard refactored the idea presented in this ticket to RFC 37: User data callbacks in CPLError and  here is related discussion.

[rouault]

Has been commited in r23286

[rouault]

r23348 /trunk/gdal/port/cpl_error.cpp: Fix CPLSetErrorHandler() (trunk only, #4295)