New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't "wipe" builtins at shutdown #63454
Comments
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 bpo-19021 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. |
This sounds like a reasonable approach. |
I'm not 100% sure that this is what intended, but probably this patch can fix it? Please review it. |
Thanks for the patch, Gennadiy. Two comments:
|
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? |
Here is simpler patch with a test. |
Unfortunately this patch doesn't fix bpo-19021. 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 bpo-19021. 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). |
Here is backported to 3.3 patch. It includes:
I'm not sure about (2), but may be (1) should be applied. |
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. |
New changeset 6a1711c96fa6 by Serhiy Storchaka in branch 'default': |
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(). |
Then perhaps PyImport_Cleanup should call _PyImportHooks_Init instead of |
New changeset fa160c8145e5 by Serhiy Storchaka in branch 'default': |
test_cleanup() of test_builtin fails: I opened issue bpo-20599 to track this bug. |
Your checkin 6a1711c96fa6 caused a Coverity complain: ** CID 1171506: Unchecked return value (CHECKED_RETURN) ________________________________________________________________________________________________________
*** 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__ */ |
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. |
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. |
It's fine. We don't *want* to check the error and fail here. |
New changeset c6e7ba7eee88 by Serhiy Storchaka in branch '2.7': New changeset ffc4f03adcac by Serhiy Storchaka in branch '3.3': New changeset efaf12106d68 by Serhiy Storchaka in branch 'default': |
New changeset 5d3b9862f1bc by Serhiy Storchaka in branch '3.4': New changeset a0bc45a50e43 by Serhiy Storchaka in branch '3.4': New changeset af8f856db136 by Serhiy Storchaka in branch '3.4': |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: