| msg20332 - (view) |
Author: Sverker Nilsson (svenil) |
Date: 2004-03-27 00:28 |
| A leak to do with readline & cmd, in Python 2.3. I found out what hold on to my interactive objects too long ('for ever') in certain circumstances. The circumstance had to do with an exception being raised in Cmd.cmdloop and handled (or not handled) outside of Cmd.cmdloop. In cmd.py, class Cmd, in cmdloop(), if an exception is raised and propagated out from the interior of cmdloop, the function postloop() is not called. The default function of this, (in 2.3) when the readline library is present, is to restore the completer, via: readline.set_completer(self.old_completer) If this is not done, the newly (by preloop) inserted completer will remain. Even if the loop is called again and run without exception, the new completer will remain, because then in postloop the old completer will be set to our new completer. When we exit, the completer will remain the one we set. This will hold on to our object, aka 'leak'. - In cmd.py in 2.2 no attempt was made to restore the completer, so this was also a kind of leak, but it was replaced the next time a Cmd instance was initialized. Now, however, the next time we will not replace the old completer, but both of them will remain in memory. The old one will be stored as self.old_completer. If we terminate with an exception, bad luck... the stored completer retains both of the instances. If we terminate normally, the old one will be retained. In no case do we restore the space of the first instance. The only way that would happen, would be if the second instance first exited the loop with an exception, and then entered the loop again an exited normally. But then, the second instance is retained instead! If each instance happens to terminate with an exception, it seems well possible that an ever increasing chain of leaking instances will be accumulated. My fix is to always call the postloop, given the preloop succeeded. This is done via a try:finally clause. def cmdloop(self, intro=None): ... self.preloop() try: ... finally: # Make sure postloop called self.postloop() I am attaching my patched version of cmd.py. It was originally from the tarball of Python 2.3.3 downloaded from Python.org some month or so ago in which cmd.py had this size & date: 14504 Feb 19 2003 cmd.py Best regards, Sverker Nilsson |
|
|
| msg20333 - (view) |
Author: Raymond Hettinger (rhettinger) *  |
Date: 2004-05-19 01:52 |
| Logged In: YES user_id=80475 Michael, this touches some of your code. Do you want to handle this one? |
|
|
| msg20334 - (view) |
Author: Michael Hudson (mwh) |
Date: 2004-05-19 09:38 |
| Logged In: YES user_id=6656 This is where I go "I wish I'd reviewed that patch more carefully". In particular, the documentation of {pre,post}loop is now out of date. I wonder setting/getting the completer in these functions was a good idea. Hmm. This bug report confuses me :-) but I can certainly see the intent of the patch... |
|
|
| msg20335 - (view) |
Author: Michael Hudson (mwh) |
Date: 2004-05-26 16:36 |
| Logged In: YES user_id=6656 What do you think of the attached? This makes the documentation of pre & post loop more accurate again, which I think is nice. |
|
|
| msg20336 - (view) |
Author: Sverker Nilsson (svenil) |
Date: 2004-05-29 07:43 |
| Logged In: YES user_id=356603 I couldn't find a new attached file. I acknowledge some problems with my original patch, but have no other suggestion at the moment. |
|
|
| msg20337 - (view) |
Author: Michael Hudson (mwh) |
Date: 2004-06-01 10:10 |
| Logged In: YES user_id=6656 Bah. I don't have the laptop with the patch with me, I'll try uploading again in a couple of days. |
|
|
| msg20338 - (view) |
Author: Michael Hudson (mwh) |
Date: 2004-06-11 12:26 |
| Logged In: YES user_id=6656 trying again... |
|
|
| msg20339 - (view) |
Author: Michael Hudson (mwh) |
Date: 2004-06-11 12:29 |
| Logged In: YES user_id=6656 yay, that appears to have worked. let me know what you think. |
|
|
| msg20340 - (view) |
Author: Sverker Nilsson (svenil) |
Date: 2004-06-18 19:13 |
| Logged In: YES user_id=356603 I think it is OK. Just noting that it changes the completer (just like my version) even if use_rawinput is false. I guess one should remember to pass a null completekey in that case, in case some other thread was using raw_input. Perhaps a check for use_rawinput could be added in cmd.py to avoid changing the completer in that case, for less risk of future mistakes. |
|
|
| msg20341 - (view) |
Author: Michael Hudson (mwh) |
Date: 2004-06-21 10:35 |
| Logged In: YES user_id=6656 Well, that would seem to be easy enough to fix (see attached). If you're using cmd.Cmd instances from different threads at the same time, mind, I think you're screwed anyway. You're certainly walking on thin ice... |
|
|
| msg20342 - (view) |
Author: Sverker Nilsson (svenil) |
Date: 2004-06-22 00:34 |
| Logged In: YES user_id=356603 Your comment about threads worries me, I am not sure I understand it. Would it be unsafe to use a cmd.Cmd instance in a separate thread, talking eg via a socket file? The instance is used only by that thread and not by others, but there may be other threads using other instances. I understand that it could be unsafe to have two threads share the same instance, but how about different instances? |
|
|
| msg20343 - (view) |
Author: Michael Hudson (mwh) |
Date: 2004-06-22 09:07 |
| Logged In: YES user_id=6656 Um. Unless I'm *hopelessly* misreading things, cmd.Cmd writes to sys.stdout unconditionally and either calls raw_input() or sys.stdin.readline(). So I'm not sure how one would "use a cmd.Cmd instance in a separate thread, talking eg via a socket file" without rewriting such huge amounts of the class that thread- safety becomes your own problem. Apologies if I'm being dumb... also, please note: I didn't write this module :-) |
|
|
| msg20344 - (view) |
Author: Sverker Nilsson (svenil) |
Date: 2004-06-23 17:58 |
| Logged In: YES user_id=356603 The constructor takes parameters stdin and stdout, and sets self.stdin and self.stdout from them or from sys. sys.std[in,out] are then not used directly except implicitly by raw_input. This seems to have changed somewhere between Python 2.2 and 2.3. Also, I remember an old version had the cmdqueue list as a class variable which was not at all thread safe - now it is an instance variable. I am hoping/thinking it is thread safe now... It seems superfluos to have both the use_rawinput flag and stdin parameter. At least raw_input should not be used if stdin is some other file than the raw input. But I don't have a simple suggestion to fix this, for one thing, it wouldn't be sufficient to compare the stdin parameter to sys.stdin since that file could have been changed so it wasn't the raw stdin anymore. Perhaps the module could store away the original sys.stdin as it was imported... but that wouldn't quite work since there is no guarantee sys.stdin hadn't already been changed. I guess if it is worth the trouble, if someone has an idea, could be a good thing to fix, anyway... |
|
|
| msg20345 - (view) |
Author: Michael Hudson (mwh) |
Date: 2004-06-29 18:59 |
| Logged In: YES user_id=6656 OK, so I was misreading (or reading an old version, or something). I agree with your comments about the bogosities, however it's not my problem today :-) Do you think I should check the attached patch in? I do. |
|
|
| msg20346 - (view) |
Author: Sverker Nilsson (svenil) |
Date: 2004-06-30 16:20 |
| Logged In: YES user_id=356603 I agree you should check it in. |
|
|
| msg20347 - (view) |
Author: Michael Hudson (mwh) |
Date: 2004-07-01 14:52 |
| Logged In: YES user_id=6656 Good. Checked in as Lib/cmd.py revision 1.38 If you want to fix any of the other issues, please file a separate patch... |
|
|