Idle, site.py, and the release candidates
While trying to test the patch for http://bugs.python.org/issue5492 on Windows, I discovered that quit() and exit() in the Idle Shell are now disabled, it seems, for all versions on all systems rather than just sometimes on Linux. The problem is a change in idlelib that invalidated an assumption made in site.py. Revs 81718-81721 for http://bugs.python.org/issue9290 changed idlelib.PyShell.PseudoFile (line 1277 in 3.3) to subclass io.TextIOBase, which subclasses IOBase. This gave PseudoFile and its subclasses a .fileno instance method attribute that raises io.UnsupportedOperation: fileno. This is not a bug since the doc for io.IOBase.fileno says: "Return the underlying file descriptor (an integer) of the stream if it exists. An OSError is raised if the IO object does not use a file descriptor." (the particular error raised is not an issue here). This is the code for Quitter.__call__ in site.py (line 368 in 3.3): def __call__(self, code=None): # Shells like IDLE catch the SystemExit, but listen when # stdin wrapper is closed. try: fd = -1 if hasattr(sys.stdin, "fileno"): fd = sys.stdin.fileno() if fd != 0: # Don't close stdin if it wraps fd 0 sys.stdin.close() except: pass raise SystemExit(code) The incorrect assumption is that if sys.stdin.fileno exits but raises, the call did not come from a shell that needs .close called. I do not know enough about other circumstances in which stdin.fileno would do something other than return 0 to be sure of what the proper fix would be. (I increasingly dislike bare excepts as they hide the thinking and knowledge of the original programmer. What exception was expected that should be passed away?) Given that the callable constants exit and quit and are optionally suppressed on startup and are not standard builtins http://docs.python.org/3/library/constants.html#constants-added-by-the-site-... it would be alright with me to ignore this regression and release as scheduled. But I though people should be aware of it. -- Terry Jan Reedy
On Sun, Mar 31, 2013 at 12:34 PM, Terry Jan Reedy <tjreedy@udel.edu> wrote:
The other problem is that making *two* function calls inside a broad try/except is almost always a terrible idea. It seems to me that the intended logic is more like this: try: # Close stdin if it wraps any fd other than 0 close_stdin = (sys.stdin.fileno() != 0) except (AttributeError, OSError, io.UnsupportedOperation): # Also close stdin if it doesn't expose a file descriptor close_stdin = True if close_stdin: try: sys.stdin.close() except Exception: pass raise SystemExit(code) Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On 3/31/2013 2:39 AM, Nick Coghlan wrote:
That too. I could not decide which function an exception was expected from.
There are 4 possible situations for sys.stdin: 1. No .fileno 2. .fileno raises 3. .fileno() != 0 4. lfileno() == 0 I believe the current code calls .close for 1 and raises SystemExit for 2-4. Your code calls for 1-3 and raises for 4. I suspect that is correct. For an rc patch, the safest temporary patch would be to start .__call__ with if sys.stdin.__name__ == 'PseudoInputFile': sys.stdin.close() I would have to check that the name is correct as seen in the user process (cannot at moment). The deeper problem, I think, is that none of sys.version, sys.hexversion, sys._version, sys.platform tell a program that it is running under Idle. It usually does not matter but there are a few situations in which it does. -- Terry Jan Reedy
On 3/31/2013 3:52 AM, Terry Reedy wrote:
In addition, idlelib.PyShell.PseudoInputFile needs a .close method + def close(self): + self.shell.close() + http://bugs.python.org/issue17585 -- Terry Jan Reedy
On Sat, 30 Mar 2013 22:34:32 -0400 Terry Jan Reedy <tjreedy@udel.edu> wrote:
You should learn to use the power of version control: http://docs.python.org/devguide/faq.html#how-do-i-find-out-who-edited-or-wha... $ hg ann Lib/site.py will tell you that the lines you mention come from the following changeset: $ hg log -r 86358b43c8bb -vp changeset: 44390:86358b43c8bb parent: 44385:5670104acd39 user: Christian Heimes <christian@cheimes.de> date: Mon Dec 31 03:07:24 2007 +0000 files: Lib/site.py description: Don't close sys.stdin with quit() if sys.stdin wraps fd 0. Otherwise it will raise a warning: Lib/io.py:1221: RuntimeWarning: Trying to close unclosable fd diff --git a/Lib/site.py b/Lib/site.py --- a/Lib/site.py +++ b/Lib/site.py @@ -247,7 +247,12 @@ # Shells like IDLE catch the SystemExit, but listen when their # stdin wrapper is closed. try: - sys.stdin.close() + fd = -1 + if hasattr(sys.stdin, "fileno"): + fd = sys.stdin.fileno() + if fd != 0: + # Don't close stdin if it wraps fd 0 + sys.stdin.close() except: pass raise SystemExit(code) In this case the original motivation seems obsolete:
That said, if IDLE users expect those global functions, perhaps IDLE should define its own ones rather than rely on site.py. Regards Antoine.
On 3/31/2013 6:01 AM, Antoine Pitrou wrote:
That said, if IDLE users expect those global functions, perhaps IDLE should define its own ones rather than rely on site.py.
I thought of that. Idle would have to check the beginning of every statement before sending it to the user process, which would do the same thing. I personally think exit/quit should eventually go, as ^d/^z or [x] button are as easy.
On Sun, Mar 31, 2013 at 12:34 PM, Terry Jan Reedy <tjreedy@udel.edu> wrote:
The other problem is that making *two* function calls inside a broad try/except is almost always a terrible idea. It seems to me that the intended logic is more like this: try: # Close stdin if it wraps any fd other than 0 close_stdin = (sys.stdin.fileno() != 0) except (AttributeError, OSError, io.UnsupportedOperation): # Also close stdin if it doesn't expose a file descriptor close_stdin = True if close_stdin: try: sys.stdin.close() except Exception: pass raise SystemExit(code) Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On 3/31/2013 2:39 AM, Nick Coghlan wrote:
That too. I could not decide which function an exception was expected from.
There are 4 possible situations for sys.stdin: 1. No .fileno 2. .fileno raises 3. .fileno() != 0 4. lfileno() == 0 I believe the current code calls .close for 1 and raises SystemExit for 2-4. Your code calls for 1-3 and raises for 4. I suspect that is correct. For an rc patch, the safest temporary patch would be to start .__call__ with if sys.stdin.__name__ == 'PseudoInputFile': sys.stdin.close() I would have to check that the name is correct as seen in the user process (cannot at moment). The deeper problem, I think, is that none of sys.version, sys.hexversion, sys._version, sys.platform tell a program that it is running under Idle. It usually does not matter but there are a few situations in which it does. -- Terry Jan Reedy
On 3/31/2013 3:52 AM, Terry Reedy wrote:
In addition, idlelib.PyShell.PseudoInputFile needs a .close method + def close(self): + self.shell.close() + http://bugs.python.org/issue17585 -- Terry Jan Reedy
On Sat, 30 Mar 2013 22:34:32 -0400 Terry Jan Reedy <tjreedy@udel.edu> wrote:
You should learn to use the power of version control: http://docs.python.org/devguide/faq.html#how-do-i-find-out-who-edited-or-wha... $ hg ann Lib/site.py will tell you that the lines you mention come from the following changeset: $ hg log -r 86358b43c8bb -vp changeset: 44390:86358b43c8bb parent: 44385:5670104acd39 user: Christian Heimes <christian@cheimes.de> date: Mon Dec 31 03:07:24 2007 +0000 files: Lib/site.py description: Don't close sys.stdin with quit() if sys.stdin wraps fd 0. Otherwise it will raise a warning: Lib/io.py:1221: RuntimeWarning: Trying to close unclosable fd diff --git a/Lib/site.py b/Lib/site.py --- a/Lib/site.py +++ b/Lib/site.py @@ -247,7 +247,12 @@ # Shells like IDLE catch the SystemExit, but listen when their # stdin wrapper is closed. try: - sys.stdin.close() + fd = -1 + if hasattr(sys.stdin, "fileno"): + fd = sys.stdin.fileno() + if fd != 0: + # Don't close stdin if it wraps fd 0 + sys.stdin.close() except: pass raise SystemExit(code) In this case the original motivation seems obsolete:
That said, if IDLE users expect those global functions, perhaps IDLE should define its own ones rather than rely on site.py. Regards Antoine.
On 3/31/2013 6:01 AM, Antoine Pitrou wrote:
That said, if IDLE users expect those global functions, perhaps IDLE should define its own ones rather than rely on site.py.
I thought of that. Idle would have to check the beginning of every statement before sending it to the user process, which would do the same thing. I personally think exit/quit should eventually go, as ^d/^z or [x] button are as easy.
participants (4)
-
Antoine Pitrou -
Nick Coghlan -
Terry Jan Reedy -
Terry Reedy