rados.py: remove Rados.__del__(); it just causes problems #3119
Conversation
|
SUCCESS: make check http://paste.ubuntu.com/9437045/ |
Sent from 
ghost
added
|
s/8767/8797/ in the commit message ;-) |
|
It looks like this will leak whatever resource shutdown will free. Would it be possible to self.librados.rados_shutdown(self.cluster) instead of run_in_thread(self.librados.rados_shutdown, (self.cluster,)) in http://workbench.dachary.org/ceph/ceph/blob/giant/src/pybind/rados.py#L251 ? |
|
It would be possible, but if anyone calls shutdown() explicitly they will block, unlike any other rados call. And, yes, it means that users who do not call shutdown() explicitly will leak resources. This is predicated on the idea that there are few users who 1) use more than one cluster connection from a long-running process, AND 2) don't call shutdown on them, ever. I think the number of such users is close to zero. |
|
Forcepushed commit msg fix. Leaving open for discussion of Loic's point. |
|
SUCCESS: make check on b74e16c output is http://paste.ubuntu.com/9450979/ |
|
@dmick I'm not familiar with this code base (disclaimer ;-). My suggestion above is because reading http://workbench.dachary.org/ceph/ceph/blob/giant/src/pybind/rados.py#L160 leads me to think that all calls to run_in_thread are synchronous and the thread is used to workaround the fact that timeouts were not implemented in the past. Am I misinterpreting this piece of code ? |
|
run_in_thread is there so that the main Python thread can receive the SIGINT (or other) signals; for all rados operations there is a potential to block, so we deal with that by running them in child threads, and letting the main Python thread still be scheduled so that it can process signals. Let's chat about this in IRC if you're still awake. |
|
Resolved: document the new requirement, add a release note, and make ceph.in explicitly call shutdown() as a model citizen |
Recent versions of Python contain a change to thread shutdown that causes ceph to hang on exit; see http://bugs.python.org/issue21963. As it turns out, this is relatively easy to avoid by not spawning threads on exit, as Rados.__del__() will certainly do by calling shutdown(); I suspect, but haven't proven, that the problem is that shutdown() tries to start() a threading.Thread() that never makes it all the way back to signal start(). Also add a PendingReleaseNote and extra doc comments to clarify. Fixes: #8797 Signed-off-by: Dan Mick <dan.mick@redhat.com>
|
FAIL: make check on 20e3fb9 output is http://paste.ubuntu.com/9467699/ |
*** DEVELOPER MODE: setting PATH, PYTHONPATH and LD_LIBRARY_PATH ***
{}
Traceback (most recent call last):
File "./ceph", line 885, in
cluster_handle.shutdown()
AttributeError: 'NoneType' object has no attribute 'shutdown'
run: 33: return 1
main: 120: code=1
|
|
Other than the bug pointed out by the make check error, looks good to me |
|
ah, right, cluster_handle might not be set up. duh. |
This is mostly a demonstration of good behavior, as the resources will be reclaimed on exit anyway. Signed-off-by: Dan Mick <dan.mick@redhat.com>
|
SUCCESS: make check on b038e8f output is http://paste.ubuntu.com/9468494/ |
rados.py: remove Rados.__del__(); it just causes problems Reviewed-by: Josh Durgin <jdurgin@redhat.com> Reviewed-by: Loic Dachary <ldachary@redhat.com>
ghost
merged commit 
Recent versions of Python contain a change to thread shutdown that
causes ceph to hang on exit; see http://bugs.python.org/issue21963.
As it turns out, this is relatively easy to avoid by not spawning
threads on exit, as Rados.del() will certainly do by calling
shutdown(); I suspect, but haven't proven, that the problem is
that shutdown() tries to start() a threading.Thread() that never
makes it all the way back to signal start().
Fixes: #8767
Signed-off-by: Dan Mick dan.mick@redhat.com