gc.garbage from paramiko

[Andrew Bennetts]

On Fri, Feb 03, 2006 at 10:41:11AM -0800, Robey Pointer wrote:
> 
> On 2 Feb 2006, at 17:19, Andrew Bennetts wrote:
> 
> >When I run tests for some code I have here that uses bzr, the test  
> >runner (Zope
> >3's) warns me after each test that there's garbage in gc.garbage  
> >(i.e. cycles
> >that Python's garbage collector can't collect).  After the first  
> >test, it's
> >just:
> >
> >[<paramiko.Transport at 0xB5DBD44CL (unconnected)>,  
> ><paramiko.Packetizer object at 0xb72d006c>, <paramiko.Transport at  
> >0xB5D0672CL (unconnected)>, <paramiko.Packetizer object at  
> >0xb5d0682c>]
> 
> Do you have any way to reproduce this outside of Zope?  I tried  
> running the bzr selftests (and a few other tests, like 'bzr push' to  
> an sftp site) with 'python -i' and then doing gc.collect() and  
> looking in gc.garbage, but wasn't able to reproduce it.

I'm working on this.  I'll let you know when I've got something minimal and
complete I can share with you.

> >The root cause is that paramiko is using __del__.  This is almost  
> >always a bad
> >idea, because objects with __del__ cannot be garbage collected if  
> >they're in a
> >reference cycle.  It's an almost certain way to cause memory leaks,  
> >unless
> >you're *extremely* careful to always break cycles manually.
> >
> >I think paramiko should not use __del__.
> 
> Unfortunately __del__ is needed for some python-file emulation, which  
> bzr and other apps rely on heavily, often without knowing (which is  
> the point).  But it should be okay as long as no cycles are created.   
> But yes, I agree that use of __del__ is really a black art.

What do you need to do on __del__?  socket.sockets already close themselves just
like files.

Cycles are really, really hard to avoid.  If a traceback occurs, that can create
cycles.  There's lots of ways to unexpectedly make a cycle.

> Paramiko actually goes to some trouble to scrupulously avoid cycles  
> with __del__.  Obviously there's a bug. :)  In your case, something  
> is creating a link from the Packetizer back to the Transport, which  
> shouldn't ever happen.  The Packetizer has no need to ever refer to  
> the Transport.  So I'd love to find a reproducable case and figure  
> out how that reference is getting attached.

There's a pretty obvious cycle right here:

        self.packetizer.set_keepalive(interval,
            lambda x=self: x.global_request('keepalive at lag.net', wait=False))

The callback you pass is a lambda with a reference to the packetizer, and I
don't see anywhere that this is broken.

I'll work on a reproducible case of my problem, but I'd like to hear more about
what you need __del__ for.

-Andrew.