Created on 2009-05-26 15:27 by exarkun, last changed 2011-05-07 00:03 by benjamin.peterson.
|msg88365 - (view)||Author: Jean-Paul Calderone (exarkun) *||Date: 2009-05-26 15:27|
When a frame's f_locals attribute is accessed, one of two things happens: * If this is the first access, a new dictionary is created and it is populated with the locals from the frame (since they are not stored internally as a dictionary). The dictionary is both saved and returned. * Otherwise, the previously saved dictionary is updated to reflect the current locals of the frame and then returned. This means that the lifetime of locals in the frame is strangely altered if the frame's f_locals attribute is ever accessed. In particular, the locals are kept alive until the frame object is discarded or the f_locals attribute is accessed again. The drawback of this is that references to locals may be "leaked" in a surprising and difficult to understand way. Another is that when debugging an application, this will cause objects to have a different lifetime than they will when the application is run without the debugger. One way the former of these issues has manifest is described in this Twisted bug report: http://twistedmatrix.com/trac/ticket/3853 There, I suggested three possible solutions, in the form of a replacement API for the f_locals attribute which either: * Gave you a copy of the locals at the current time and didn't pretend it would be automatically updated * Gave you a view onto the locals at the current time, instead of a real dictionary, and didn't actually hold any references to the locals itself, only granted access to the "canonical" locals data in the frame. * Gave you a real dictionary of the locals but didn't hold a strong reference to that dictionary internally, so that if all external users of the dictionary went away, the frame itself would also stop referring to the locals.
|msg88428 - (view)||Author: Antoine Pitrou (pitrou) *||Date: 2009-05-27 18:53|
IMO, any of these three solutions should be accepted. The second is of course the nicest but also the most complex one, and it's probably not worth it. I'm not sure the third solution is easily feasible given that dicts currently don't accept weakref'ing. We could of course create a special subclass of dict, but, again, is it worth it? This leaves us with the first solution which is, IMO, good enough, and obviously the simplest of all three. You're welcome to propose a patch!
|msg88429 - (view)||Author: Jean-Paul Calderone (exarkun) *||Date: 2009-05-27 18:58|
Oh, I forgot dictionaries aren't weakrefable. That's such a pain, I thought the third solution would be a good balance between easy and good. :/ Regarding the first solution, my only question right now is whether this should be a new attribute/method, or if the behavior of f_locals should just be changed. The former would be nicer, I think, and perhaps f_locals can be deprecated in favor of it.
|msg88430 - (view)||Author: Antoine Pitrou (pitrou) *||Date: 2009-05-27 19:13|
> Regarding the first solution, my only question right now is whether this > should be a new attribute/method, or if the behavior of f_locals should > just be changed. The former would be nicer, I think, and perhaps > f_locals can be deprecated in favor of it. I think the behaviour of f_locals can just be changed. (Other developers may disagree)
|msg135210 - (view)||Author: Facundo Batista (facundobatista) *||Date: 2011-05-05 14:45|
Making a copy of f_locals values to return a dictionary that doesn't hold references to the locals of the frame is not that simple (for me, at least): - shallow copy: we'll return always a new dict, with the values being a copy of locals; this is simpler, but what about other objects that are referenced in in those values? For example, in locals exists a list A which holds a reference to object B; the new dict we return will have copy of A, but this copy will still reference B. - deep copy: we'll return a new dict with a deep copy of all values; this is safer, but what about memory? In any case, how we could do a deep copy here? calling Python's code copy.deepcopy()? I want to add a fourth option to JP's initial ones: - have other attribute, something like f_locals_weak that is a list of tuples [(name, value), ...], being the values a weak reference to the real locals. What do you think? Regards,
|msg135214 - (view)||Author: Antoine Pitrou (pitrou) *||Date: 2011-05-05 15:50|
> Making a copy of f_locals values to return a dictionary that doesn't > hold references to the locals of the frame is not that simple I think you misunderstood the bug report. The issue here is not that locals are referenced from the dict, it's that the dict is kept alive (since it is cached) longer that it should. By the way, reading the discussion again, I think we could simply make dicts weak-referenceable. A dict is already quite big in memory and I don't think adding a pointer to the struct would make a significant difference.
|msg135216 - (view)||Author: Benjamin Peterson (benjamin.peterson) *||Date: 2011-05-05 16:15|
I vote for option 1. We already eschew hacks with locals().
|msg135229 - (view)||Author: Facundo Batista (facundobatista) *||Date: 2011-05-05 18:45|
Antoine, to see if I understood correctly... if we build the dict, and just return it but don't save it in the frame, this leak would be solved? (yes, it'd be slower because everytime it's asked, it'd need to be built again)
|msg135327 - (view)||Author: Antoine Pitrou (pitrou) *||Date: 2011-05-06 16:58|
Le jeudi 05 mai 2011 à 18:45 +0000, Facundo Batista a écrit : > > Antoine, to see if I understood correctly... if we build the dict, and > just return it but don't save it in the frame, this leak would be > solved? (yes, it'd be slower because everytime it's asked, it'd need > to be built again) I would let Jean-Paul confirm the answer, but: yes.
|msg135389 - (view)||Author: Benjamin Peterson (benjamin.peterson) *||Date: 2011-05-07 00:03|
I tried making f_locals always a copy but it wreaks havoc with some code which expects changing it to "work", namely pdb. I resign myself to using weakrefs.
|2011-05-07 00:03:23||benjamin.peterson||set||messages: + msg135389|
|2011-05-06 16:58:18||pitrou||set||messages: + msg135327|
|2011-05-05 18:45:59||facundobatista||set||messages: + msg135229|
messages: + msg135216
+ gvanrossum, rhettinger|
messages: + msg135214
messages: + msg135210
|2009-05-27 19:13:34||pitrou||set||messages: + msg88430|
|2009-05-27 18:58:07||exarkun||set||messages: + msg88429|
messages: + msg88428