Add functions to interrupt processing in a specific thread/context #803
Conversation
- Add Interrupt::requestForCurrentThread (C API: GEOS_interruptThread) to request interruption of the current thread only. - Add Interrupt::registerThreadCallback to register an interruption callback for the current thread only. - Add GEOSContext_setInterruptCallback_r to associate an interruption callback with a context handle. The callback will be registered for the current thread each time an _r function is called with the specified handle.
|
@macdrevx @nyalldawson My understanding is that this API is preferable to #761 for your use cases, but I'm inclined to let this PR sit until you've had a chance to test it (on your own timeline) in case it's difficult to use in a way I didn't anticipate. Does that make sense? |
| extern GEOSInterruptThreadCallback GEOS_DLL *GEOSContext_setInterruptCallback_r( | ||
| GEOSContextHandle_t extHandle, | ||
| GEOSInterruptThreadCallback* cb, | ||
| void* userData); |
There was a problem hiding this comment.
@dbaston what do you think about making this callback return a value that indicates whether an interrupt is desired and eliminating GEOS_interruptThread()?
| * Register a function to be called when a possible interruption point is reached | ||
| * in code executed in the specified context. The function can interrupt the | ||
| * thread if desired by calling GEOS_interruptThread. | ||
| * | ||
| * \param extHandle the context returned by \ref GEOS_init_r. | ||
| * \param cb Callback function to invoke | ||
| * \param userData optional data to be pe provided as argument to callback | ||
| * \return the previously registered callback, or NULL | ||
| * \see GEOSInterruptThreadCallback | ||
| */ |
There was a problem hiding this comment.
Should we say that you should not start other GEOS operations inside of the callback? I ccould imagine folks wanting to make the decision based on the result of some geos operation, but that would risk clobbering the thread_local state used to store the interrupt state.
| void | ||
| Interrupt::cancel() | ||
| { | ||
| requested = false; | ||
| requested_for_thread = false; |
There was a problem hiding this comment.
cancel() is invoked from GEOS_init_r() — that doesn't seem quite right if we're using it to cancel an interrupt request for a specific context. Does cancellation even make sense for context-specific interruption?
| return requested; | ||
| return requested || requested_for_thread; |
There was a problem hiding this comment.
Shouldn't there be some synchronization around requested?
| * \see GEOS_interruptRegisterThreadCallback | ||
| * \see GEOS_interruptThread | ||
| */ | ||
| typedef void (GEOSInterruptThreadCallback)(void*); |
There was a problem hiding this comment.
I would avoid the word thread in these new APIs and associated documentation. Maybe this one could be named GEOSContextInterruptCallback and similar for other new symbols.
| @@ -27,33 +27,55 @@ class GEOS_DLL Interrupt { | |||
| public: | |||
|
|
|||
| typedef void (Callback)(void); | |||
| typedef void (ThreadCallback)(void*); | |||
There was a problem hiding this comment.
I think we could simplify the design by keeping the old and new implementations separate: We could create a separate class to house the new context-specific interrupt implementation. This would likely also help with my earlier comment about how not all of the API surface that makes sense for the old API is relevant to the new API. My current thinking is that we could update the GEOS_CHECK_FOR_INTERRUPTS() definition to check for interrupts from both sources. Something along the lines of:
#define GEOS_CHECK_FOR_INTERRUPTS() geos::util::Interrupt::process(); geos::util::ThreadSpecificInterrupt::process();
There was a problem hiding this comment.
Additionally, I think this design would make it easier to one day drop the old interrupt API (something I hope we consider since my main interest in interruption stems from folks who use GEOSwift reporting tooling-detected data races related to the old implementation)
|
Going to land this or #761 for 3.12? |
|
How far are we from landing this in 3.13? |
|
Is this waiting on further feedback? I would LOVE to see this land in a geos release soon 🤞 |
|
@nyalldawson if you're happy with it, then I think all that remains would be to address the good suggestions from @macdrevx . @pramsey , is there a target release date for 3.13? |
|
@dbaston I like this approach! |
Mid-September. I'd say we're "almost there", so if you feel good about this, merge it and I will roll the next beta, in anticipation of a release after testing. |
This PR is an alternative API to #761. Instead of registering an interrupt handler for the calling thread -- which is a bit awkward, the handler is registered with the context handle. Since the C++ library has no notion of a context handle, the C API registers and unregisters the interrupt handler with every C API call. To avoid a significant performance penalty for very short C API functions, the C API
executeerror handler is modified so that functions can be tagged as noninterruptible so the interrupt handler will not be registered when they are called.