classification
Title: Don't "wipe" builtins at shutdown
Type: behavior Stage: resolved
Components: Interpreter Core Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: serhiy.storchaka Nosy List: Arfrever, brett.cannon, christian.heimes, eric.snow, gennad, ncoghlan, pitrou, python-dev, serhiy.storchaka, vstinner
Priority: critical Keywords: patch

Created on 2013-10-13 21:34 by serhiy.storchaka, last changed 2014-03-20 08:56 by serhiy.storchaka. This issue is now closed.

Files
File name Uploaded Description Edit
19255.patch gennad, 2014-01-04 00:04 review
builtins_cleanup.patch pitrou, 2014-01-07 18:48 review
modules_cleanup.patch serhiy.storchaka, 2014-02-09 10:01 review
modules_cleanup-3.3.patch serhiy.storchaka, 2014-02-10 16:12 review
issue19255-coverity.patch serhiy.storchaka, 2014-02-11 21:21 review
Messages (21)
msg199810 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-10-13 21:34
Many __del__ methods expect that builtins are existing at the time of executing. But when the object survives until the builtins module is wiped, __del__ will fail with unexpected AttributeError (see issue19021 for example). We can change __del__'s in the stdlib to not use non-cached builtins, but third party code will be broken.

Perhaps we should special case shutdown of the builtins module. E.g. reset it to initial value (all initial members of the builtins module are immutable and can't create resource loops) instead wiping.
msg199816 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-10-13 22:16
> Perhaps we should special case shutdown of the builtins module. E.g.
> reset it to initial value (all initial members of the builtins module
> are immutable and can't create resource loops) instead wiping.

This sounds like a reasonable approach.
msg207259 - (view) Author: Gennadiy Zlobin (gennad) * Date: 2014-01-04 00:04
I'm not 100% sure that this is what intended, but probably this patch can fix it? Please review it.
msg207477 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-01-06 20:46
Thanks for the patch, Gennadiy. Two comments:
1) I don't think re-doing the whole initialization like that is ok. It's too brutal and may also not work as you want.
2) To validate your changes, you should add a unit test anyway (in test_builtin perhaps).
msg207478 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-01-06 20:47
Also, since such changes are subtle and error-prone, I think it's better to defer until 3.5 now that 3.4b2 has been released. Serhiy, what do you think?
msg207581 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-01-07 18:48
Here is simpler patch with a test.
msg210739 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-02-09 10:01
Unfortunately this patch doesn't fix issue19021. Popen.__del__ is called when whiping the idlelib.rpc module which was collected in a weaklist. But the builtins module also was collected in a weaklist and wiped before idlelib.rpc.

Here is revised patch which fixes issue19021. It defers wiping of the builtins and sys modules (this partially restores 3.3 algorithm) and fixes other related issues (some of these fixes should be backported to older versions).
msg210845 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-02-10 16:12
Here is backported to 3.3 patch. It includes:

1. Operates not with sys.modules['builtins'].__dict__ and sys.modules['sys'].__dict__, but with cached interp->builtins and interp->sysdict, because sys.modules['builtins'] and sys.modules['sys'] can be changed, but builtins and standard streams for builtins are retrieved from cached interp->builtins and interp->sysdict.

2. interp->builtins is restored to saved copy.

3. Backported test. Actually it passed with current code, but can prevent future bugs.

I'm not sure about (2), but may be (1) should be applied.
msg210847 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-02-10 16:17
I'm not convinced this should be backported. It's always a bit risky to change interpreter shutdown, and it fixes an issue which has been well-known (and worked around) for many years.
msg210848 - (view) Author: Roundup Robot (python-dev) Date: 2014-02-10 16:21
New changeset 6a1711c96fa6 by Serhiy Storchaka in branch 'default':
Issue #19255: The builtins module is restored to initial value before
http://hg.python.org/cpython/rev/6a1711c96fa6
msg210849 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-02-10 17:00
I agree that it is risky and don't insist. We can return to this patch when encounter more serious bug.

Last commit breaks test_create_at_shutdown_without_encoding in test_io for Python implementation of io.

This happens because Python implementation of TextIOWrapper.__init__() imports the locale module if encoding is not specified. Then _find_spec() in Lib/importlib/_bootstrap.py iterates sys.meta_path, but "meta_path" is one of names cleared in sys in PyImport_Cleanup().
msg210850 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-02-10 17:08
> This happens because Python implementation of TextIOWrapper.__init__()
> imports the locale module if encoding is not specified. Then
> _find_spec() in Lib/importlib/_bootstrap.py iterates sys.meta_path,
> but "meta_path" is one of names cleared in sys in PyImport_Cleanup().

Then perhaps PyImport_Cleanup should call _PyImportHooks_Init instead of
setting import-related attributes to None.
msg210851 - (view) Author: Roundup Robot (python-dev) Date: 2014-02-10 17:09
New changeset fa160c8145e5 by Serhiy Storchaka in branch 'default':
Temporary silence test broken by issue19255.
http://hg.python.org/cpython/rev/fa160c8145e5
msg210973 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2014-02-11 16:52
test_cleanup() of test_builtin fails: I opened issue #20599 to track this bug.
msg211003 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-02-11 19:26
Turns out call _PyImportHooks_Init is not enough, see issue20602 and issue20603.
msg211014 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2014-02-11 20:39
Your checkin 6a1711c96fa6 caused a Coverity complain:

** CID 1171506:  Unchecked return value  (CHECKED_RETURN)
/Objects/moduleobject.c: 352 in _PyModule_ClearDict()
/Objects/moduleobject.c: 333 in _PyModule_ClearDict()


________________________________________________________________________________________________________
*** CID 1171506:  Unchecked return value  (CHECKED_RETURN)
/Objects/moduleobject.c: 352 in _PyModule_ClearDict()
346                         const char *s = _PyUnicode_AsString(key);
347                         if (s != NULL)
348                             PySys_WriteStderr("#   clear[2] %s\n", s);
349                         else
350                             PyErr_Clear();
351                     }
>>>     CID 1171506:  Unchecked return value  (CHECKED_RETURN)
>>>     No check of the return value of "PyDict_SetItem(d, key, &_Py_NoneStruct)".
352                     PyDict_SetItem(d, key, Py_None);
353                 }
354             }
355         }
356     
357         /* Note: we leave __builtins__ in place, so that destructors
/Objects/moduleobject.c: 333 in _PyModule_ClearDict()
327                         const char *s = _PyUnicode_AsString(key);
328                         if (s != NULL)
329                             PySys_WriteStderr("#   clear[1] %s\n", s);
330                         else
331                             PyErr_Clear();
332                     }
>>>     CID 1171506:  Unchecked return value  (CHECKED_RETURN)
>>>     No check of the return value of "PyDict_SetItem(d, key, &_Py_NoneStruct)".
333                     PyDict_SetItem(d, key, Py_None);
334                 }
335             }
336         }
337     
338         /* Next, clear all names except for __builtins__ */
msg211023 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2014-02-11 21:17
The two problematic code paths were marked as "intentional" and "ignore" in the past. There is no point in reporting errors that late in the shutdown phase. Also the code paths are very unlikely to trigger an error.
msg211025 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2014-02-11 21:21
> Your checkin 6a1711c96fa6 caused a Coverity complain:
>
> ** CID 1171506:  Unchecked return value  (CHECKED_RETURN)
> /Objects/moduleobject.c: 352 in _PyModule_ClearDict()
> /Objects/moduleobject.c: 333 in _PyModule_ClearDict()

Actually this was in old code. May now the code is just simpler for analysis by Coverity.

What can we do with this? Only call PyErr_Clear() as for other errors.
msg211030 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2014-02-11 22:21
It's fine. We don't *want* to check the error and fail here.
msg211059 - (view) Author: Roundup Robot (python-dev) Date: 2014-02-12 07:56
New changeset c6e7ba7eee88 by Serhiy Storchaka in branch '2.7':
Issue #19255: Clear error after failed PyDict_SetItem() on shutdown.
http://hg.python.org/cpython/rev/c6e7ba7eee88

New changeset ffc4f03adcac by Serhiy Storchaka in branch '3.3':
Issue #19255: Clear error after failed PyDict_SetItem() on shutdown.
http://hg.python.org/cpython/rev/ffc4f03adcac

New changeset efaf12106d68 by Serhiy Storchaka in branch 'default':
Issue #19255: Clear error after failed PyDict_SetItem() on shutdown.
http://hg.python.org/cpython/rev/efaf12106d68
msg213818 - (view) Author: Roundup Robot (python-dev) Date: 2014-03-17 06:30
New changeset 5d3b9862f1bc by Serhiy Storchaka in branch '3.4':
Issue #19255: The builtins module is restored to initial value before
http://hg.python.org/cpython/rev/5d3b9862f1bc

New changeset a0bc45a50e43 by Serhiy Storchaka in branch '3.4':
Temporary silence test broken by issue19255.
http://hg.python.org/cpython/rev/a0bc45a50e43

New changeset af8f856db136 by Serhiy Storchaka in branch '3.4':
Issue #19255: Clear error after failed PyDict_SetItem() on shutdown.
http://hg.python.org/cpython/rev/af8f856db136
History
Date User Action Args
2014-03-20 08:56:33serhiy.storchakasetstatus: open -> closed
assignee: serhiy.storchaka
resolution: fixed
stage: patch review -> resolved
2014-03-17 06:30:59python-devsetmessages: + msg213818
2014-02-12 07:56:11python-devsetmessages: + msg211059
2014-02-11 22:21:31christian.heimessetmessages: + msg211030
2014-02-11 21:21:04serhiy.storchakasetfiles: + issue19255-coverity.patch

messages: + msg211025
2014-02-11 21:17:16christian.heimessetmessages: + msg211023
2014-02-11 20:39:24christian.heimessetnosy: + christian.heimes
messages: + msg211014
2014-02-11 19:26:31pitrousetmessages: + msg211003
2014-02-11 16:52:25vstinnersetmessages: + msg210973
2014-02-10 17:09:42python-devsetmessages: + msg210851
2014-02-10 17:08:23pitrousetmessages: + msg210850
2014-02-10 17:00:55serhiy.storchakasetnosy: + brett.cannon, ncoghlan, eric.snow
messages: + msg210849
2014-02-10 16:21:57python-devsetnosy: + python-dev
messages: + msg210848
2014-02-10 16:18:00pitrousetmessages: + msg210847
2014-02-10 16:12:21serhiy.storchakasetfiles: + modules_cleanup-3.3.patch

messages: + msg210845
2014-02-09 10:01:09serhiy.storchakasetfiles: + modules_cleanup.patch

stage: needs patch -> patch review
messages: + msg210739
versions: + Python 3.4, - Python 3.5
2014-01-07 18:49:00pitrousetfiles: + builtins_cleanup.patch

messages: + msg207581
2014-01-06 21:08:23vstinnersetnosy: + vstinner
2014-01-06 20:47:47pitrousetmessages: + msg207478
versions: + Python 3.5, - Python 3.4
2014-01-06 20:46:44pitrousetmessages: + msg207477
2014-01-04 00:04:18gennadsetfiles: + 19255.patch

nosy: + gennad
messages: + msg207259

keywords: + patch
2013-11-29 21:02:10Arfreversetnosy: + Arfrever
2013-11-16 17:35:42serhiy.storchakasetpriority: normal -> critical
2013-10-13 22:16:50pitrousetmessages: + msg199816
2013-10-13 21:34:15serhiy.storchakacreate