[Python-Dev] Idle, site.py, and the release candidates

[Terry Jan Reedy]

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-module
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

_______________________________________________
Python-Dev mailing list
Pytho...@python.org
http://mail.python.org/mailman/listinfo/python-dev
Unsubscribe: http://mail.python.org/mailman/options/python-dev/dev-python%2Bgarchive-30976%40googlegroups.com

[Nick Coghlan]

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   |   ncog...@gmail.com   |   Brisbane, Australia

[Terry Reedy]

On 3/31/2013 2:39 AM, Nick Coghlan wrote:

On Sun, Mar 31, 2013 at 12:34 PM, Terry Jan Reedy <tjr...@udel.edu>

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?)

The other problem is that making *two* function calls inside a broad try/except is almost always a terrible idea.

That too. I could not decide which function an exception was expected from.

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)

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

[Antoine Pitrou]

On Sat, 30 Mar 2013 22:34:32 -0400

Terry Jan Reedy <tjr...@udel.edu> wrote:
>

> 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.

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-what-revision-changed-a-line-last

$ 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 <chri...@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:

>>> import sys
>>> sys.stdin.fileno()
0
>>> sys.stdin.close()
>>>


That said, if IDLE users expect those global functions, perhaps IDLE
should define its own ones rather than rely on site.py.

Regards

Antoine.

[Terry Jan Reedy]

On 3/31/2013 3:52 AM, Terry Reedy wrote:
> 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).

In addition, idlelib.PyShell.PseudoInputFile needs a .close method

+ def close(self):
+ self.shell.close()
+
http://bugs.python.org/issue17585

[Terry Jan Reedy]

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.

[Antoine Pitrou]

On Sun, 31 Mar 2013 11:38:07 -0400

Terry Jan Reedy <tjr...@udel.edu> wrote:

> 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.

I never use them myself, but they are more discoverable than keyboard
shortcuts, and hence can be useful for beginners or casual users.

Regards

Antoine.