Timeout generic callbacks used in destructors and inflight request cancelation

[pentschev]

Destructors may be called from Python's garbage collector, disallowing us to disable the GIL for that operation. When that occurs, there might be a deadlock as other threads have the GIL, such as the progress thread when a Python's listener callback is invoked and executed in that thread.

[wence-]

Some minor cleanups (to avoid excessive looping when maxAttempts > 1 and a suggestion of how to implement a timeout for the spinlock case.

I was trying to think about how we could drop the gil (say) when destructing in python, but I am not sure if it is possible.

[wence-]

I wonder if it is also a good idea to explicitly release the gil when dropping the C++ shared_ptr attributes of UCXX python cdef classes. I think something like:

diff --git a/python/ucxx/_lib/libucxx.pyx b/python/ucxx/_lib/libucxx.pyx
index 3e26bfb..ef8b4be 100644
--- a/python/ucxx/_lib/libucxx.pyx
+++ b/python/ucxx/_lib/libucxx.pyx
@@ -862,6 +862,10 @@ cdef class UCXEndpoint():
         bint _enable_python_future
         dict _close_cb_data
 
+    def __dealloc__(self):
+        with nogil:
+            self._endpoint = <shared_ptr[Endpoint]>nullptr;
+
     def __init__(
             self,
             uintptr_t shared_ptr_endpoint,

(For example) Makes sure that (if this is the last reference to the Endpoint) ~Endpoint runs with the GIL released.

WDYT?

I think this change is worthwhile anyway so I can propose in a separate PR.

[pentschev]

I wonder if it is also a good idea to explicitly release the gil when dropping the C++ shared_ptr attributes of UCXX python cdef classes. I think something like:

diff --git a/python/ucxx/_lib/libucxx.pyx b/python/ucxx/_lib/libucxx.pyx
index 3e26bfb..ef8b4be 100644
--- a/python/ucxx/_lib/libucxx.pyx
+++ b/python/ucxx/_lib/libucxx.pyx
@@ -862,6 +862,10 @@ cdef class UCXEndpoint():
         bint _enable_python_future
         dict _close_cb_data
 
+    def __dealloc__(self):
+        with nogil:
+            self._endpoint = <shared_ptr[Endpoint]>nullptr;
+
     def __init__(
             self,
             uintptr_t shared_ptr_endpoint,

(For example) Makes sure that (if this is the last reference to the Endpoint) ~Endpoint runs with the GIL released.

WDYT?

I think this change is worthwhile anyway so I can propose in a separate PR.

Sorry, I missed this reply earlier. I think this is a great idea, it might actually do what we were attempting of releasing the GIL at destruction time, and __dealloc__ didn't occur to me before so thanks for thinking of that! I agree a separate PR is probably good, if you want to do it please feel free to submit it, otherwise I can work on it next week as well. 🙂

[wence-]

I agree a separate PR is probably good, if you want to do it please feel free to submit it, otherwise I can work on it next week as well.

#132

[wence-]

Thanks Peter

[pentschev]

/merge

[pentschev]

Thanks for the review @wence- !