Fix Tkinter Tcl-commands memory-leaks

[384ccc4e-cb8f-445e-b7c8-688ad3cd2535]

BPO	1524639
Nosy	@loewis, @terryjreedy, @asvetlov, @serhiy-storchaka
Files	Tkinter.py-leak.diff Tkinter.py-leak2.diff: Revised, extended, and fixed patch keep_tclcommands_correct.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields: ```python assignee = 'https://github.com/asvetlov' closed_at = None created_at = labels = ['type-bug', 'expert-tkinter'] title = 'Fix Tkinter Tcl-commands memory-leaks' updated_at = user = 'https://bugs.python.org/pysquared' ``` bugs.python.org fields: ```python activity = actor = 'serhiy.storchaka' assignee = 'asvetlov' closed = False closed_date = None closer = None components = ['Tkinter'] creation = creator = 'pysquared' dependencies = [] files = ['7410', '7411', '12430'] hgrepos = [] issue_num = 1524639 keywords = ['patch'] message_count = 14.0 messages = ['50709', '50710', '50711', '50712', '50713', '77953', '77968', '77969', '77971', '77977', '77992', '78007', '78204', '110490'] nosy_count = 8.0 nosy_names = ['loewis', 'terry.reedy', 'pysquared', 'quentin.gallet-gilles', 'gpolo', 'asvetlov', 'matthieu.labbe', 'serhiy.storchaka'] pr_nums = [] priority = 'normal' resolution = None stage = 'patch review' status = 'open' superseder = None type = 'behavior' url = 'https://bugs.python.org/issue1524639' versions = ['Python 2.7', 'Python 3.3', 'Python 3.4'] ```

[384ccc4e-cb8f-445e-b7c8-688ad3cd2535]

Fix 8 memory-leaks by cleaning up created Tcl commands automatically.

I attach a patch against Tkinter 47021.

=== Long explanation \===

I was bitten by a memory leak in Tkinter - 25MB per day on a long-running process. A net search found a couple unrelated Tkinter leaks, and gave me some clues. Investigation using the tracing feature in tkleak.py (see link) found the bug.

I searched for more similar leaks, and fixed them too.

The reasoning for patch bpo-1121234 gives the reason for the changes to _register() and deletecommand().

See http://www.uk.debian.org/~graham/python/tkleak.py for my leak tracing and test script.

[384ccc4e-cb8f-445e-b7c8-688ad3cd2535]

Logged In: YES user_id=543663

The python version I am using in production is 2.1 (for historical reasons), however the memory leak bugs are in 2.1 through to 2.5.

I can generate a patch against any of the older versions (e.g. 2.4) if anyone is interested.

[384ccc4e-cb8f-445e-b7c8-688ad3cd2535]

Logged In: YES user_id=543663

I fixed a bug in the original patch: when destroying a window which had Variable instances attached which in turn had trace commands bound, Tkinter.py was trying to delete the commands twice, as the command was mentioned in Variable instance _tclCommands and _root()._tcl_Commands.

Also fixed 4 more leaks occurring when TclError is raised after a callback has been created.

I have added 6 tests to tkleak.py to test the additional 4 fixes.

The patch is against 50704, hope it helps.

Thanks for your hard work, and for accepting my other Tkinter.py patch.

[61337411-43fc-4a9c-b8d5-4060aede66d0]

Logged In: YES user_id=21627

The patch looks wrong. Why are you deleting the _tkcommands of the master widget if the variable is deleted? The master could live much longer, and the commands might still be needed.

What is the purpose of the changes to trace_variable? You are supposed to invoke trace_vdelete to release the callback.

Why are you catching TclError in so many cases? Under what circumstances can you even get an error?

I think I would prefer if this patch was split into many individual ones, each one fixing only a single bug (assuming there are multiple bugs in Tkinter).

[384ccc4e-cb8f-445e-b7c8-688ad3cd2535]

Logged In: YES user_id=543663

The patch looks wrong. Why are you deleting the _tkcommands of the master widget if the variable is deleted? The master could live much longer, and the commands might still be needed.

AIUI, Tkinter callbacks (Button and Scrollbar commands, Text [xy]view, event, protocol and variable bindings, after callbacks etc) should always be given a Python callable, and not the Tcl name of an already bound Python command. Example: b1 = Button(root, command=pressed) b2 = Button(root, command=b1['command']) # Share command b1.destroy() b2.invoke() # Callback is invalid, and rightly so Given that, when a Variable is deleted, is needs to clean up the Tcl commands it created, as no other widget has a reference to them, and they are no longer needed, and there will be no other way of deleting them. By the time __del__() deletes any 'u' callbacks, they have already been called, as globalunsetvar is called first, so this is safe.

What is the purpose of the changes to trace_variable? You are supposed to invoke trace_vdelete to release the callback.

Is it the aim for Tkinter to work in a similar way to Python's reference counted and garbage collected mem-man? I assume it is, so the changes track commands associated with the variable, for cleanup on collection. See next comment below for explanation of TclError.

Why are you catching TclError in so many cases? Under what circumstances can you even get an error?

The following all raise TclError: w.after("spam", eggs) # bad argument "spam": must be cancel, idle, info, or a number w.bind("\<spam>", eggs) # bad event type or keysym "spam" w.bind_all("\<spam>", eggs) # bad event type or keysym "spam" w.bind_class("Button", "\<spam>", eggs) # bad event type or keysym "spam" w.config(spam=eggs) # unknown option "-spam" v.trace_variable("spam", eggs) # bad operations "spam": should be one or more of rwu

When the exception is raised, the 'eggs' Tcl command for the 'spam' callback has already been created. The exception means the "command-id" is never returned for use in cleaning up manually. So, the patch cleans up on exception automatically. I don't think the argument that it's OK for incorrect use to result in memory leaks holds any water :-) I may of course be missing the obvious.

I think I would prefer if this patch was split into many individual ones, each one fixing only a single bug (assuming there are multiple bugs in Tkinter).

Sorry for the rushed patch, I noticed the release schedule for Python2.5 and just put together the patch from monkey-patch code I had already been using, tested it and uploaded. The tkleak.py program mentioned tests all the fixes, but I did not have time to add them to the Python test suite. I realise you probably have many demands on your time (fielding loads of patches like this one :-), like I do (baby imminent! work demands etc.), so I hope the explanations above help (I should have explained all this before), if not I may be able to submit separate patches and tests at a later date.

Many thanks, Graham Horler

[1fecb825-97ee-46b6-913c-bb07e2a288a6]

Something like the proposed patch is still needed. But allow me to point out my views towards your current patch:

• Changes in Misc.after, Misc._bind: good

• Changes in Misc.unbind can be simplified a bit:

cbs = self._bind_names(self._bind(('bind', self._w), sequence,
func=None, add=None))

could be only:

cbs = self._bind_names(self.bind(sequence))

and the comment in the end should go.

• Changes in Misc.unbind_all and Misc.unbind_class can be simplified in the same as as Misc.unbind (just change self.bind by self.bind_all and self.bind_class)

• Changes in Variable: would you care to elaborate more on these ? Do you need a new _tclCommands there and also use the _tclCommands from its master ?

• Changes in Misc._options are somewhat fine.

  • The comment about Python 2.3, 2.4, 2.5 being bugged because a Tcl_Obj is returned is misleading, so you should change it.
  • What are you gaining by sorting the cnf keys ?

• Changes in Misc._configure uhm.. it is hard to like that one =) Since we are after commands created, why can't we just check the _tclCommands before and after calling tcl here ? So we just remove the difference (maybe move it to a set?) and don't do all those checks in case of error, and then reraise the error.

• Changes in Wm.wm_protocol: I almost like it, except that there are two checks for a callable arg now.

Hopefully you are still around.

[1fecb825-97ee-46b6-913c-bb07e2a288a6]

The changes in Misc.destroy do not look right, why are you deleting commands created by bind_all (for example) from the root window when a simple widget is destroyed ?

It would make more sense to not apply these changes in Misc.destroy, instead, Misc.unbind_all would remove commands from the root using Misc._root.deletecommand and inside Misc._bind it would check if needcleanup is true or false and decide from where to remove. Also, given how this needcleanup parameter is used, I suggest renaming it to widgetcmd, so if it is set 0 it indicates that the command should live in self._root()._tclCommands.

[53b105e9-1b2b-49c1-9485-48b79ea4a607]

A little remark : please replace "if not globals().has_key('TkinterError'):" by "if 'TkinterError' not in globals():". has_key is now deprecated in 2.6 and should be avoided.

[1fecb825-97ee-46b6-913c-bb07e2a288a6]

Fix: My previous comment was about changes in Misc.deletecommand not Misc.destroy (this is what you get for guessing the methods changed in the diff, without applying it).

To Quentin: I wouldn't bother mentioning it since this new TkinterError exception should go away along the changes in Misc.deletecommand as mentioned in my previous comment.

[1fecb825-97ee-46b6-913c-bb07e2a288a6]

Rethinking about the changes done in Misc._configure I found out that you don't need any of those there. Isn't it the case of only improving the changes regarding callable overwriting in Misc._options ? So if the value is a callable and the option doesn't exist you also don't _register the callable. Maybe I'm forgetting about something here, so you could bring an example that fails on this reasoning.

I've applied these changes and others commented before in a thing I'm testing: http://code.google.com/p/plumage/source/detail?r=85 http://code.google.com/p/plumage/source/detail?r=86 http://code.google.com/p/plumage/source/detail?r=87 http://code.google.com/p/plumage/source/detail?r=89 http://code.google.com/p/plumage/source/detail?r=91

I didn't fix the ones related to Variable yet, but if any of these changes looks fine for someone the diffs would probably apply on "the standard" Tkinter.py without much problem.

[1fecb825-97ee-46b6-913c-bb07e2a288a6]

I've fixed the leaks in Variable doing this: http://code.google.com/p/plumage/source/detail?r=93

I don't use an extra _tclCommands for Variable, instead in __del__ I use the information returned by Variable.trace_vinfo to remove associated callbacks.

[1fecb825-97ee-46b6-913c-bb07e2a288a6]

On Wed, Dec 17, 2008 at 3:51 PM, Guilherme Polo \report@bugs.python.org\ wrote:

Guilherme Polo \ggpolo@gmail.com\ added the comment:

Rethinking about the changes done in Misc._configure I found out that you don't need any of those there. Isn't it the case of only improving the changes regarding callable overwriting in Misc._options ? So if the value is a callable and the option doesn't exist you also don't _register the callable. Maybe I'm forgetting about something here, so you could bring an example that fails on this reasoning.

Uhm, this will fail in other places in Tkinter that are using _options for handling options not related to widget options but specific command options.

[1fecb825-97ee-46b6-913c-bb07e2a288a6]

I hope you are not too bored for me commenting on this again.

So, I have re-though about this issue today and decided to solve it differently (and will include a patch here this time, don't worry about mentions to external repo this time). To solve the problem I replaced the tk.call method in the Tk so it can remove registered commands in the call in case it fails.

The other problems reported in this issue are also solved by it, and others that weren't reported are too: Misc.selection_handle (just to name one, but there are others) could leave a undesired item in _tclCommands too.

Another advantage of this solution is that extensions benefit from it, and they do not need to change their code (except if they are using a collection of internal functions, which they shouldn't, affected by this patch).

[83d2e70e-e599-4a04-b820-3814bbdb9bef]

Given the current debate on python-dev regarding IDLE and its dependancy on tkinter, surely something as serious as a memory leak should be looked into.