Add functions to interrupt a single thread #761
Conversation
In Shapely we don't use this callback. @caspervdw experimented with this callback earlier this year (shapely/shapely#1370), but we ended up implementing a way to interrupt on the shapely side (so in between calls to GEOS). |
The big benefit of this call is that it can potentially interrupt stupidly long geos internal methods -- ie. when a user tries to intersection a >million vertex polygon with another >million vertex polygon 🤣 . Right now there's no clean way to interrupt those operations without completely killing the whole process. |
QGIS currently doesn't use it, because we can't isolate to individual threads. I'd love to use it to allow users to cancel long running geos operations though (such as the previously mentioned ridiculous intersection case) The api for the interruption handling is a little tricky to understand. I gather I register the callback, and then from the callback I call GEOS_interruptRequest() when I want to interrupt a process. Is this correct? (If so, this may be a nice opportunity to clean up that api and make the callback return bool for when the interruption is desired) |
dbaston
force-pushed
the
interrupt-thread
branch
from
d6413ef
to
b6597c9
Compare
Yes, this is the case. (It took me a while to figure it out too!) The API sort of makes sense for a system where In your case, would you want to register a different callback for each thread? (Does the callback need to be thread-local?) Or would you use a single callback like I did in |
I could work with either. The per-thread/context callback would be cleaner though. |
Yes, and so ideally we should certainly do both (and thus also implement the GEOS interrupt callback). |
- Add Interrupt::requestForCurrentThread (C API: GEOS_interruptThread) to request interruption of the current thread only. - Add Interrupt::registerThreadCallback (C API: GEOS_interruptRegisterThreadCallback) to register an interruption callback for the current thread only.
dbaston
force-pushed
the
interrupt-thread
branch
from
b6597c9
to
14f2a62
Compare
dbaston
changed the title
|
@dbaston I can work with this api 👍 many thanks!! |
OK, with the current iteration you can interrupt the current thread using I used a new callback signature for |
Co-authored-by: Even Rouault <even.rouault@spatialys.com>
|
@dbaston I'm glad to see this work taking shape as it will help to address GEOSwift/GEOSwift#190. The proposed API lets you register a per-thread callback. I'm curious whether you've considered making it per-context rather than per-thread. My understanding is that GEOS contexts are meant to be confined to a single thread anyway, so tying the interrupt callback to the context seems like it could also solve the issue. It might also fit nicely alongside other existing context-specific APIs like the ones to register error and message handlers. It could also be easier to use from systems where you don't select which thread to use directly (e.g. Grand Central Dispatch on Apple platforms, where you schedule work to be performed and the work is executed by one of several threads in a pool). In GEOSwift, we actually use short-lived GEOS contexts that never out-live a single call into the GEOSwift API, so if we were going to use the interrupt API in the future, tying the lifetime of the callback to the lifetime of the context would be a natural design. That said, I think we could use the per-thread design as well by simply un-registering the handler (which I see is possible by passing NULL for the new handler) before returning from each GEOSwift API call that uses this feature. |
Yes, and that would certainly make for a cleaner API. The problem is that the "context" is a C API construct to which the C++ library (the code actually being interrupted) has no access, so we would need to introduce the notion of a context into the C++ library in such a way that any interruptible code can access its current context. I'm not sure of a simple way to do that, but I'm open to ideas. |
|
Maybe we could have a thread-local global variable in the C++ library that stores the context, and every C API call sets that global variable before calling code in the C++ library? |
That might have perhaps performance implications, but couldn't that interrupt handler be registered at the C context level, and each time a C _r function is invoked it would automatically install the per-thread interrupt handler before calling the C++ method, and uninstalls it at return / when an exception is caught ? |
|
I think that would also work. Pushing the context object into the C++ library might reduce the penalty because we would be setting/checking one thread-local variable instead of two. I would need to set up a benchmark to see if that actually matters. |
|
See #803 for an alternative API that is based on context handles. |
This PR changes
GEOS_interruptRegisterCallbackto operate on a per-thread basis; the callback will only be registered on the thread from whichGEOS_interruptRegisterCallbackis called. This would break existing expectations of a global callback, so maybe a better solution is to add a new function that registers a thread-local callback.I'm fuzzy on how this gets used outside of PostGIS which (I think?) would not be affected by this change.
cc @nyalldawson