Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#4295 closed enhancement (fixed)

Support of user-data in CPLErrorHandler

Reported by: mloskot Owned by: warmerdam
Priority: normal Milestone: 1.9.0
Component: default Version: svn-trunk
Severity: normal Keywords: rfc
Cc: rouault

Description

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.

Attachments (6)

cplerrorhandler-user-data-support.patch (6.3 KB) - added by mloskot 5 years ago.
Patch with user-data support for CPLErrorHandler
trampoline_set_handler.cpp (1.5 KB) - added by mloskot 5 years ago.
Simple example presenting use of different types of erorr handlers
trampoline_streams.cpp (2.0 KB) - added by mloskot 5 years ago.
Another example
4295-error-handling-hobu-documentation-fix-CPLGetErrorUserData.patch (7.2 KB) - added by hobu 5 years ago.
4295-error-handling-nowak-with-docs.patch (5.3 KB) - added by hobu 5 years ago.
4295-hobu-rfc.patch (7.4 KB) - added by hobu 5 years ago.

Download all attachments as: .zip

Change History (16)

Changed 5 years ago by mloskot

Patch with user-data support for CPLErrorHandler

Changed 5 years ago by mloskot

Simple example presenting use of different types of erorr handlers

Changed 5 years ago by mloskot

Another example

comment:1 Changed 5 years ago by 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;

comment:2 Changed 5 years ago by hobu

  • Cc rouault added

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.

comment:3 Changed 5 years ago by 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

comment:4 Changed 5 years ago by 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.

comment:5 Changed 5 years ago by hobu

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

comment:6 Changed 5 years ago by 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)

comment:7 Changed 5 years ago by 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?

Changed 5 years ago by hobu

comment:8 Changed 5 years ago by mloskot

  • Keywords rfc added

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

comment:9 Changed 5 years ago by rouault

  • Milestone set to 1.9.0
  • Resolution set to fixed
  • Status changed from new to closed

Has been commited in r23286

comment:10 Changed 5 years ago by rouault

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

Note: See TracTickets for help on using tickets.