Opened 13 years ago

Closed 12 years ago

Last modified 12 years ago

#4295 closed enhancement (fixed)

Support of user-data in CPLErrorHandler

Reported by: Mateusz Łoskot Owned by: warmerdam
Priority: normal Milestone: 1.9.0
Component: default Version: svn-trunk
Severity: normal Keywords: rfc
Cc: Even Rouault


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 Mateusz Łoskot 13 years ago.
Patch with user-data support for CPLErrorHandler
trampoline_set_handler.cpp (1.5 KB ) - added by Mateusz Łoskot 13 years ago.
Simple example presenting use of different types of erorr handlers
trampoline_streams.cpp (2.0 KB ) - added by Mateusz Łoskot 13 years ago.
Another example
4295-error-handling-hobu-documentation-fix-CPLGetErrorUserData.patch (7.2 KB ) - added by hobu 13 years ago.
4295-error-handling-nowak-with-docs.patch (5.3 KB ) - added by hobu 13 years ago.
4295-hobu-rfc.patch (7.4 KB ) - added by hobu 12 years ago.

Download all attachments as: .zip

Change History (16)

by Mateusz Łoskot, 13 years ago

Patch with user-data support for CPLErrorHandler

by Mateusz Łoskot, 13 years ago

Attachment: trampoline_set_handler.cpp added

Simple example presenting use of different types of erorr handlers

by Mateusz Łoskot, 13 years ago

Attachment: trampoline_streams.cpp added

Another example

comment:1 by hobu, 13 years ago


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 by hobu, 13 years ago

Cc: Even 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 by Even Rouault, 13 years ago

[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 by hobu, 13 years ago

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 by hobu, 13 years ago

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

comment:6 by Even Rouault, 13 years ago

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;
            pfnErrorHandler = &CPLSetErrorHandlerExWrapper;
            pErrorHandlerUserData = pUserData;

        if( pfnOldHandler == &CPLSetErrorHandlerExWrapper )
            *ppfnRetOldErrorHandler = NULL;
            *ppfnRetOldErrorHandlerEx = pfnOldHandlerEx;
            *ppRetOldUserData = pOldErrorHandlerUserData;
            *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 by hobu, 13 years ago

Even, does not have these deficiencies for CPLSetErrorHandler, even if the API is a little clunkier. Shall we choose that one?

by hobu, 12 years ago

Attachment: 4295-hobu-rfc.patch added

comment:8 by Mateusz Łoskot, 12 years ago

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 by Even Rouault, 12 years ago

Milestone: 1.9.0
Resolution: fixed
Status: newclosed

Has been commited in r23286

comment:10 by Even Rouault, 12 years ago

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

Note: See TracTickets for help on using tickets.