Fix finalizers when Python futures are enabled #318
Conversation
pentschev
added
bug
| shared_from_this().get()); | ||
| std::lock_guard<std::mutex> lock(_futuresPoolMutex); | ||
| PyGILState_STATE state = PyGILState_Ensure(); | ||
| decltype(_futuresPool) newFuturesPool; |
There was a problem hiding this comment.
note: I guess this also applies to populateFuturesPool as well.
This function takes two locks, so we need to think about lock ordering. Is there a scenario where a thread holding the GIL might be attempting to call getFuture (which will need to acquire _futuresPoolMutex), while a different thread not holding the GIL has called clearFuturesPool and therefore holds the lock => deadlock?
Or is it the case that only one python thread is allowed to talk to these worker functions?
There was a problem hiding this comment.
Currently yes, only one Python thread is supported (aka, tested/used). However, this is a very much valid concern as we have almost everything in place for multi-thread Python. I've filed #325 to track that and fix it in a follow-up PR.
|
Thanks Lawrence for this review as well. As mentioned above I've filed an issue to think better about the double lock case, it shouldn't be a problem for our current use cases though so I'll go ahead and merge this for now. |
|
/merge |
The Python futures pool retains a reference to
_notifier, which prevents further cleanup of theucxx::{python::,}Worker. Thus a newclearFuturesPoolmethod is introduced to allow clearing the pool so that these references are dropped, which is called before the Python_notifierThreadexits.Additionally, move
ApplicationContext.progress_tasksto a globalProgressTasksdictionary. Having the progress tasks as a member attribute ofApplicationContextforces us to keep a reference to it that prevents from properly cleaning up in theweakref.finalize. Using an external object allows us to prevent the self-reference and clean it up properly when no further references toApplicationContextexist orucxx.reset()is called.