Skip to content
This repository has been archived by the owner. It is now read-only.

Reference cycle in _TracebackLogger and bug in _TracebackLogger.__del__() #155

Open
GoogleCodeExporter opened this issue Apr 10, 2015 · 7 comments

Comments

@GoogleCodeExporter
Copy link

@GoogleCodeExporter GoogleCodeExporter commented Apr 10, 2015

Hi,

While trying to change how Trollius store tracebacks in Future.set_exception(), 
I found two bugs:

* _TracebackLogger object is never deleted if its activate() method is not 
called. It happens when run_until_complete() is used: the call to the 
activate() method is scheduled, but it will never be executed. It happens in 
many tests (test_tasks.py, test_futures.py, I don't remember which one)

* _TracebackLogger.__del__() does nothing if self.tb is None whereas self.exc 
is still known

The following patch fixes these two bugs:
http://codereview.appspot.com/69350045

Remarks:

- the patch modifies Future.__del__() to have the same output on Python 3.4: 
just add a trailing colon to the error message
- TestLoop.close() closes also BaseEventLoop.close() to clear _ready: it is 
needed in the new unit test to remove the last reference to the _TracebackLogger
- Future.set_exception()

By default (debug mode disabled), Future.set_exception() now clears the 
__traceback__ attribute of the exception to fix the reference cycle issue. So 
the log of unhandled exception doesn't contain the traceback anymore. It makes 
the debug much harder :-( But it avoids uncollectable objects in production.

To get the traceback, enable the debug mode of the event loop, but you might 
create uncollectable objects again.

I don't know what is the best choice: keep the traceback or avoid uncollectable 
objects. Is it common to use run_until_complete() and miss the call the 
activate()?

At least, the change of _TracebackLogger.__del__() must be applied.

--

Even with the patch, test_tasks.test_wait_errors() still creates uncollectable 
objects (Future, frames, TimerHandle, etc.) with Python 3.3. It's probably a 
different bug.

Original issue reported on code.google.com by victor.s...@gmail.com on 27 Feb 2014 at 5:40

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Apr 10, 2015

Original comment by victor.s...@gmail.com on 27 Feb 2014 at 6:03

Attachments:

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Apr 10, 2015

Oh, the issue was already discussed here:
http://code.google.com/p/tulip/issues/detail?id=42

And here:
http://bugs.python.org/issue20032

The CPython issue contains a different example to demonstrate the problem:
http://bugs.python.org/file33238/never_deleted.py

Original comment by victor.s...@gmail.com on 5 Mar 2014 at 12:41

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Apr 10, 2015

Should we merge this into issue 42 or not?

Original comment by gvanrossum@gmail.com on 5 Mar 2014 at 6:44

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Apr 10, 2015

> Should we merge this into issue 42 or not?

I'm not sure that it's exactly the same issue, I prefer to not merge them yet.

In my tests, calling gc.collect() didn't break (all) reference cycles.

Original comment by victor.s...@gmail.com on 5 Mar 2014 at 9:15

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Apr 10, 2015

I wrote classes to create a view of a traceback which doesn't contain locals 
but can still be used to format a traceback:
https://bitbucket.org/haypo/misc/src/ce48d7b3ea1d223691e496e41aca8f5784671cd5/py
thon/suppress_locals.py?at=default

Such view can be used in Tulip to store a traceback without creating reference 
cycles.

Original comment by victor.s...@gmail.com on 3 Jun 2014 at 11:53

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Apr 10, 2015

> I wrote classes to create a view of a traceback which doesn't contain locals 
but can still be used to format a traceback:
> 
https://bitbucket.org/haypo/misc/src/ce48d7b3ea1d223691e496e41aca8f5784671cd5/py
thon/suppress_locals.py?at=default

A similar code was proposed in this issue:
http://bugs.python.org/issue17911

Original comment by victor.s...@gmail.com on 12 Jun 2014 at 12:47

@GoogleCodeExporter
Copy link
Author

@GoogleCodeExporter GoogleCodeExporter commented Apr 10, 2015

Issue 42 has been merged into this issue.

Original comment by victor.s...@gmail.com on 18 Jun 2014 at 12:48

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.