Extend error and notice notification with threadsafe variants

[pepijnve]

Extends the C wrapper interface with a means to associate user data with the notification handlers. This enables threadsafe handling of error and notice messages on the consumer side.

[strk]

Further simplification: why would GEOSMessageHandler_r need to take a pointer to GEOSContext instance ?
It can already ask to get passed an arbitrary pointer, so if it needs a GEOSMessageHandler it could make it available as part of the structure it'll request as an argument. The signature could be reduced to take only a const char pointer and a void pointer.

[pepijnve]

That does make more sense. I removed it.

[strk]

A quick local patch that didn't want to loose:

diff --git a/capi/geos_c.h.in b/capi/geos_c.h.in
index fe9180f..55395dc 100644
--- a/capi/geos_c.h.in
+++ b/capi/geos_c.h.in
@@ -178,14 +178,15 @@ extern void GEOS_DLL GEOS_interruptRequest();
 extern void GEOS_DLL GEOS_interruptCancel();

 /*
- * @deprecated initialize using GEOS_init_r() and set the message handlers using GEOSContext_setNoticeHandler_r and/or
- *             GEOSContext_setErrorHandler_r subsequently.
+ * @deprecated in 3.5.0
+ *     initialize using GEOS_init_r() and set the message handlers using
+ *     GEOSContext_setNoticeHandler_r and/or GEOSContext_setErrorHandler_r
  */
 extern GEOSContextHandle_t GEOS_DLL initGEOS_r(
                                     GEOSMessageHandler notice_function,
                                     GEOSMessageHandler error_function);
 /*
- * @deprecated replaced by GEOS_finish_r.
+ * @deprecated in 3.5.0 replaced by GEOS_finish_r.
  */
 extern void GEOS_DLL finishGEOS_r(GEOSContextHandle_t handle);

[strk]

So last thing I'd like to think more about is the naming of functions to set the new handlers.

I don't like how it looks like the old and the new both take a"GEOSMessageHandler" (one with an "_r" suffix)
but the new has a "MessageHandler" rather than "Handler" naming. Sounds confusing.

[pepijnve]

Yep it's a bit confusing this way. I had the feeling all the good names were already taken. The MessageHandler_r is the actual _r variant, but GEOSContext_setNoticeHandler_r already exists. I initially had GEOSContext_setNoticeMessageHandler_r_v2, but I found that kind of ugly. Perhaps a name that refers to the userdata parameter. I don't have any intuitive sounding, self explanatory suggestions I'm afraid.

[strk]

@rouault could you take a look at this ? I guess these are the signature I was mentioning on the mailing list, for lack of _r suffix.
REF: http://lists.osgeo.org/pipermail/geos-devel/2014-June/006878.html

[pepijnve]

@srtk revisiting this based on a question on the spatialite mailing list. What was the naming issue we got stuck on exactly? I tried to figure it out based on the past discussion, but I can't seem to pinpoint what the exact issue was.

[strk]

It was the confusion between GEOSContext_setNoticeHandler_r and GEOSContext_setNoticeMessageHandler_r signatures (and the one for Error).
I guess adding deprecation notices on those functions too could do it (re-reading the changes)

[pepijnve]

Right, thanks for refreshing my memory.
Agreed the distinction between those two and which one is preferred is not obvious.
I see three options:

1. Come up with a more distinctive name for the new variants. I still don't have any that don't sounds ridiculous I'm afraid.
2. Change the name to GEOSContext_setNoticeHandler_r_ex or GEOSContext_setNoticeHandler_r _v2 or something like that. Not an uncommon solution in C libraries although it's a bit ugly IMO.
3. Keep the GEOSContext_setNoticeMessageHandler_r name

If you let me know which one has your preference I'll make the necessary changes.

Either way a deprecation comment on the older version pointing to the new one is indeed a good idea.

[strk]

I surrender to GEOSContext_setNoticeMessageHandler_r (current naming). The deprecation warning must be in place _and_ it must be ensured that old clients do not break.

[strk]

@robe2 I think this would be better included in next release (3.5.0)
@pepijnve it needs a ticket on the official tracker to ensure this is the case, could you file one ?
http://trac.osgeo.org/geos/

[strk]

I found an already existing trac ticket: https://trac.osgeo.org/geos/ticket/663

[pepijnve]

@strk I'm a little strapped for time at the moment. What's the target release date for 3.5.0? In other words, by when do you need this PR polished up and ready to merge?

[strk]

It's @robe taking care of release this round. I think we're talking about 2/3 weeks from now. Ideally the patch would be clean and ready within 1 week for further testing. BTW, @esseffe spotted a few initialization problems, hopefully he'll be submitting the patch here soon

[spatialite]

I've rather extensively tested this patch on SpatiaLite, and I can confirm that it works nicely and doesn't break any back compatibility. more important than all, it definitely fixes all thread-safety issues.

anyway during my tests (heavily based on Valgrind) I discovered a rather trivial error (uninitialized variables) causing crash, the following patch effectively fixes the problem.

GEOSContextHandle_HS(GEOSMessageHandler_r nf,
                     void *nd, void(*free_nd)(void*),
                     GEOSMessageHandler_r ef, void *ed,
                     void(*free_ed)(void*))
{
    memset(msgBuffer, 0, sizeof(msgBuffer));
    geomFactory = GeometryFactory::getDefaultInstance();
    WKBOutputDims = 2;
    WKBByteOrder = getMachineByteOrder();
+   noticeData = NULL;
+   freeNoticeData = NULL;
+   errorData = NULL;
+   freeErrorData = NULL;
    setNoticeHandler(nf, nd, free_nd);
    setErrorHandler(ef, ed, free_ed);
    initialized = 1;
}

[strk]

@pepijnve please also rebase to trunk after accepting @spatialite contrib.
Please let me know if you'll be able to do within the upcoming week.

[strk]

@spatialite there's no "freeNoticeData" nor "freeErrorData" in the code I'm reading ?

[strk]

So this was merged in r4060 and @spatialite concerns were addressed in r4061, those two should be shortly available in the git mirror.