-
-
Notifications
You must be signed in to change notification settings - Fork 718
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Weakref finalizers prohibit GC #7639
Comments
I believe some of the above examples are actually valid, e.g. when passing |
Would wrapping the func in a lambda avoid this reference? |
No but it's still easy to avoid by using a global function instead of a method. For example # Instead of
class C:
def __init__(self):
weakref.finalize(self, self.shutdown, foo='bar')
def shutdown(self, foo):
...
# ---
# do
def shutdown_C(obj, foo):
...
class C:
def __init__(self):
weakref.finalize(self, shutdown_C, foo='bar') |
Just to note, this patten unfortunately doesn't work if
|
@pentschev and I were diagnosing a recent failure in dask-cuda while using UCX . After bisecting we found the issue was tied to a recent PR removing the weakref for Offload Executor -- it seems like we need an explicit finalizer otherwise we run into shutdown issues at least when also trying to use UCX |
We believe the issue we're experiencing in rapidsai/dask-cuda#1148 stems from Distributed failing to shutdown resources on the expected order now. For example, UCX-Py requires to be explicitly terminated (via We are not 100% certain what is happening in the issue is exactly what is described above, but it is one of the shortcomings that exist with the way Python/async allows us to work with. Therefore, we must always rely on Distributed finalizing resources appropriately, and it seems that not shutting down the offload executor with the weakref finalizer prevents Distributed from fulfilling what we need. To quote similar developments from the past, we had to replace exit handlers by weakref finalizers in #4184 , and it seems #7644 did the exact opposite (although without the use of exit handlers). |
Everything in the docs for
The It would be very helpful to have a proper bug report. What is in rapidsai/dask-cuda#1148 is not sufficient for me to understand what the problem is. The CPython documentation strongly suggests that weakref.finalize will not be called under these circumstances so I do not believe that this behavior can be relied upon. It looks like we're lucky this is called and I wouldn't be surprised for this behavior to change in a future python version (or in a patch even). Besides this I can't put together what this finalizer is even supposed to be doing and am concerned that what you're seeing is a red herring. A proper reproducer or an explanation would go a long way.
I do not believe this issue applies to the current case for a variety of reasons
the
|
In a couple of places in our code base we're using weakref.finalizers in a way that ensures that an object is actually never GCed.
The notes specifically state
see here https://docs.python.org/3/library/weakref.html#weakref.finalize
See a dicussion here #7593 (comment)
Almost every usage of weakref.finalize in our codebase is subject to this fallacy. Here a couple of example but this is non exhaustive.
distributed/distributed/process.py
Lines 110 to 112 in 8a3b44c
distributed/distributed/process.py
Line 148 in 8a3b44c
distributed/distributed/comm/asyncio_tcp.py
Lines 474 to 477 in 8a3b44c
distributed/distributed/comm/inproc.py
Line 182 in 8a3b44c
distributed/distributed/comm/tcp.py
Lines 196 to 206 in 8a3b44c
We should remove these broken finalizers since they obviously do not help a lot other than keeping objects alive. Some might have important functionality which is no longer triggered because of the reference.
The text was updated successfully, but these errors were encountered: