Bug 26257 - Make sure we actually dispose of objects when we get closed.
Summary: Make sure we actually dispose of objects when we get closed.
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: fargo (show other bugs)
Version: unspecified
Hardware: Other All
: high normal
Assignee: David Laban
QA Contact: Simon McVittie
URL: http://git.collabora.co.uk/?p=user/al...
Whiteboard: milestone4.9, est=5h
Keywords:
Depends on: 26099
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-26 12:41 UTC by David Laban
Modified: 2010-03-22 05:42 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Description David Laban 2010-01-26 12:41:32 UTC
I suspect we don't. Should just be a case of removing ourselves from our parent object when we are closed, but we might be being kept alive by our iq callbacks, in which case we need to create a mechanism for pruning callbacks when we die.

estimate: 2-7 hours.
Comment 1 David Laban 2010-02-03 09:29:20 UTC
I think we do: we delete Calls from conn when the Channels are invalidated at least.

Having a test which checks with the garbage collector that no instances of Call, StreamedMediaChannel, Connection etc exist at relevant points would be nice before I close this bug.
Comment 2 David Laban 2010-02-08 13:01:19 UTC
Pushed fixes and tests to [1]. The last commit "optionally use objgraph to help debug..." is optional because it uses a module that's available from pypi but not apt. It shows how I debugged the leaks in the first place, so I thought I should include it.

[1] http://git.collabora.co.uk/?p=user/alsuren/telepathy-fargo.git;a=shortlog;h=refs/heads/leaks
Comment 3 Simon McVittie 2010-02-09 04:10:37 UTC
> \ No newline at end of file

No thanks. (But consider this change insta-reviewed :-P)

> +        if isinstance(obj, class_) and not isinstance(obj, weakref.ProxyType):

Slightly terrifying, but appears necessary... :-/

(I generally prefer weakref.ref, for this sort of reason - also, "explicit is better than implicit".)

> +from twisted.internet import reactor

Is it strictly necessary to add this to from_xmpp.py? It doesn't seem to be used. If it's being imported for its side-effects (what are they?), please comment that.

> +def gc_get_instances(class_):

If it's deliberate that class_ can actually be a tuple of classes, please add a docstring that says so.

The optional commit is fine too, since it only triggers when we're about to fail and is robust against the module's absence.
Comment 4 David Laban 2010-02-17 12:38:05 UTC
I made a branch to ban the use of IQs. See your #telepathy logs for details of why (sorry: got to dash)

http://git.collabora.co.uk/?p=user/alsuren/telepathy-fargo.git;a=shortlog;h=refs/heads/ban-twisted-iq-class

Take particular note of the log message "Should probably file a bug for cancelling the call if we get an iq error or timeout."

This was also brainstormed by me to #telepathy. Comments on this bug.
Comment 5 David Laban 2010-03-22 05:42:29 UTC
I'm pretty convinced that this is fixed now, thanks to 26099.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.