IDLE - regression with exit() and quit() #61785

serwy · 2013-03-31T07:45:00Z

BPO	17585
Nosy	@birkenfeld, @terryjreedy, @pitrou, @larryhastings, @benjaminp, @serwy, @serhiy-storchaka
Files	fileno_close.patch issue17585.patch issue17585_2.7.patch site_reversion.patch

^{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:

assignee = 'https://github.com/serwy'
closed_at = <Date 2013-05-12.09:42:35.286>
created_at = <Date 2013-03-31.07:45:00.006>
labels = ['expert-IDLE', 'type-bug', 'release-blocker']
title = 'IDLE - regression with exit() and quit()'
updated_at = <Date 2013-05-12.09:42:35.285>
user = 'https://github.com/serwy'

bugs.python.org fields:

activity = <Date 2013-05-12.09:42:35.285>
actor = 'georg.brandl'
assignee = 'roger.serwy'
closed = True
closed_date = <Date 2013-05-12.09:42:35.286>
closer = 'georg.brandl'
components = ['IDLE']
creation = <Date 2013-03-31.07:45:00.006>
creator = 'roger.serwy'
dependencies = []
files = ['29618', '29620', '29621', '29622']
hgrepos = []
issue_num = 17585
keywords = ['patch']
message_count = 17.0
messages = ['185619', '185620', '185621', '185628', '185633', '185635', '185636', '185637', '185638', '185640', '185642', '186182', '186545', '186548', '186602', '186603', '188177']
nosy_count = 8.0
nosy_names = ['georg.brandl', 'terry.reedy', 'pitrou', 'larry', 'benjamin.peterson', 'roger.serwy', 'python-dev', 'serhiy.storchaka']
pr_nums = []
priority = 'release blocker'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue17585'
versions = ['Python 2.7', 'Python 3.3', 'Python 3.4']

serwy · 2013-03-31T07:44:59Z

This issue is a split from bpo-5492, where Terry noticed a serious regression that "quit()" and "exit()" no longer work in IDLE.

Before bpo-9290, the PyShell object itself was stdin and it didn't have a "fileno" method. The code in site.py would skip over the call to fileno, leaving fd == -1, which then called close() on stdin, effectively the close() method of PyShell.

The application of bpo-9290 introduced PseudoFile as a subclass of io.TextIOBase which has a "fileno" method. The site.py code find it, calls it, but an error gets raised so that the close() method doesn't get called. Even if you subclass fileno(), you still need to subclass close() so that it calls the original close function in PyShell.

The attached patch, fileno_close.patch, fixes that issue. I would argue that this patch should be applied to all the release candidates as it corrects a serious regression.

I would like to hear your thoughts before applying the patch.

terryjreedy · 2013-03-31T08:07:42Z

I thought of this exact patch, but then decided against it.

The bug is in site.py -- see thread
[Python-Dev] Idle, site.py, and the release candidates
for a better explanation and solution.
The Idle behavior, inherited from io.IOBase is correct.
The consequence on *other* Python code of a fake fileno is unknown.

The solution I gave for site.py is to begin .__call__ with
if sys.stdin.__name__ == 'PseudoInputFile': sys.stdin.close()
This should be safe to do without another release candidate.

The name as seen by the user process should be checked but I cannot do so at the moment (on substitute substitute computer). This would only be done in the release by the file managers. The exact decision is up to them.

So I would replace IDLE with site.py in the title.

serwy · 2013-03-31T08:27:49Z

I agree that site.py's Quitter exception logic has a flaw as described on the email from python-dev. But I disagree that the problem of IDLE not exiting is due to site.py. Even if you fix site.py (which I did), calling sys.stdin.close() won't close IDLE since PseudoInputFile's close method doesn't call PyShell's close method.

terryjreedy · 2013-03-31T15:39:42Z

I see that now. Then both files should be fixed. I still object to introducing a buggy .fileno ;-).

serwy · 2013-03-31T17:41:19Z

Terry, I agree that the original patch introduces a buggy fileno and it "knows" too much about the internals of site.py which could change.

The attached patch modifies site.py, similar to how Nick Coghlan split the two function calls in a single try/except block into two separate try/except blocks. I tried to keep the change as small as possible to avoid introducing any unintended regressions. The change does pass the test suite on 3.4 on Linux.

pitrou · 2013-03-31T18:14:58Z

Please read http://mail.python.org/pipermail/python-dev/2013-March/125021.html . The fileno() check shouldn't be needed anymore.

Also, it can't effect 2.7, or you have another problem.

pitrou · 2013-03-31T18:15:15Z

(s/effect/affect/, sorry)

serwy · 2013-03-31T18:26:59Z

Antoine, it does affect 2.7 since sys.stdin.close() doesn't call PyShell's close method. bpo-9290 introduced this regression. I accept some responsibility because my manual testing of those patches in bpo-9290 didn't include trying the exit() and quit() methods.

The attached patch fixes the problem on the 2.7 branch.

serwy · 2013-03-31T18:36:12Z

I would also argue that the fileno_close.patch ought to be applied to the 3.2.4 and 3.3.1 release candidates. The changes in bpo-9290 were applied only two months ago.

pitrou · 2013-03-31T18:39:49Z

I won't argue about any changes in IDLE, but the fileno-checking code in site.py should probably be removed, not complicated.

serwy · 2013-03-31T19:26:42Z

I agree with Antoine that fileno checking code should be removed. I dug deeper into why that was introduced in the first place.

00b136b7da84 introduced the warning "Trying to close unclosable fd!"
86358b43c8bb intoduced the change to site.py about not closing stdin if wrapping fd 0 to avoid the warning.
23d9cb777d9a removed the warning as part of bpo-4233.

So the code in site.py for checking fileno is cruft and can be safely removed. The site_reversion patch backs out of 86358b43c8bb.

serhiy-storchaka · 2013-04-07T07:46:03Z

Perhaps PseudoInputFile.close() should call super().close() to set closed flag. Perhaps close() should be even implemented in PseudoFile.

It's my fault in this regression. I deliberately have not implemented PseudoFile.close(), because I saw no sense in deliberate calling of sys.std*.close() and considered closing the window with unpremeditated closing of the standard stream is too dangerous.

serwy · 2013-04-11T04:32:44Z

Serhiy, don't worry. There's still plenty of broken found in IDLE.

Antoine, may I modify site.py with site_reversion.patch? If not, then I can include a workaround in IDLE.

pitrou · 2013-04-11T06:29:14Z

Yes, fixing site.py is fine.

python-dev · 2013-04-12T00:24:18Z

New changeset 99e363e67844 by Roger Serwy in branch '2.7':
bpo-17585: Fixed IDLE regression. Now closes when using exit() or quit().
http://hg.python.org/cpython/rev/99e363e67844

New changeset d3c67e2fc68c by Roger Serwy in branch '3.3':
bpo-17585: Fixed IDLE regression. Now closes when using exit() or quit().
http://hg.python.org/cpython/rev/d3c67e2fc68c

New changeset 82451c88b3c0 by Roger Serwy in branch 'default':
bpo-17585: merge with 3.3.
http://hg.python.org/cpython/rev/82451c88b3c0

serwy · 2013-04-12T00:29:42Z

Thanks, Antoine.

I am closing this issue as fixed.

serhiy-storchaka · 2013-04-30T19:12:13Z

This fix introduces other bug (see bpo-17838). I think this fix with bpo-17838 fix should be included in next regression releases.

serwy mannequin self-assigned this Mar 31, 2013

serwy mannequin added topic-IDLE type-bug An unexpected behavior, bug, or error labels Mar 31, 2013

serwy mannequin closed this as completed Apr 12, 2013

serhiy-storchaka reopened this Apr 30, 2013

serhiy-storchaka added the release-blocker label Apr 30, 2013

birkenfeld closed this as completed May 12, 2013

ezio-melotti transferred this issue from another repository Apr 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IDLE - regression with exit() and quit() #61785

IDLE - regression with exit() and quit() #61785

serwy mannequin commented Mar 31, 2013

serwy mannequin commented Mar 31, 2013

terryjreedy commented Mar 31, 2013

serwy mannequin commented Mar 31, 2013

terryjreedy commented Mar 31, 2013

serwy mannequin commented Mar 31, 2013

pitrou commented Mar 31, 2013

pitrou commented Mar 31, 2013

serwy mannequin commented Mar 31, 2013

serwy mannequin commented Mar 31, 2013

pitrou commented Mar 31, 2013

serwy mannequin commented Mar 31, 2013

serhiy-storchaka commented Apr 7, 2013

serwy mannequin commented Apr 11, 2013

pitrou commented Apr 11, 2013

python-dev mannequin commented Apr 12, 2013

serwy mannequin commented Apr 12, 2013

serhiy-storchaka commented Apr 30, 2013

IDLE - regression with exit() and quit() #61785

IDLE - regression with exit() and quit() #61785

Comments

serwy mannequin commented Mar 31, 2013

serwy mannequin commented Mar 31, 2013

terryjreedy commented Mar 31, 2013

serwy mannequin commented Mar 31, 2013

terryjreedy commented Mar 31, 2013

serwy mannequin commented Mar 31, 2013

pitrou commented Mar 31, 2013

pitrou commented Mar 31, 2013

serwy mannequin commented Mar 31, 2013

serwy mannequin commented Mar 31, 2013

pitrou commented Mar 31, 2013

serwy mannequin commented Mar 31, 2013

serhiy-storchaka commented Apr 7, 2013

serwy mannequin commented Apr 11, 2013

pitrou commented Apr 11, 2013

python-dev mannequin commented Apr 12, 2013

serwy mannequin commented Apr 12, 2013

serhiy-storchaka commented Apr 30, 2013