-
-
Notifications
You must be signed in to change notification settings - Fork 31.1k
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
Clear state of threads earlier in Python shutdown #63665
Comments
Each Python thread holds references to objects, in its current frame for example. At Python shutdown, clearing threads state happens very late: the import machinery is already dead, types are finalized, etc. If a thread has an object with a destructor, bad things may happen. For example, when open files are destroyed, a warning is emitted. The problem is that you cannot emits warnings because the warnings module has been unloaded, and so you miss warnings. See the issue bpo-19442 for this specific case. It is possible to clear the Python threads earlier since Python 3.2, because Python threads will now exit cleanly when they try to acquire the GIL: see PyEval_RestoreThread(). The value of the tstate pointer is used in PyEval_RestoreThread(), but the pointer is not dereferenced (the content of a Python state is not read). So it is even possible to free the memory of the threads state (not only to clear the threads state). Attached patch implements destroy the all threads except the current thread, and clear the state of the current thread. The patch calls also the garbage collector before flushing stdout and stderr, and disable signal handlers just before PyImport_Cleanup(). The main idea is to reorder functions like this:
The side effect of the patch is that the destructor of destroyed objects are now called, especially for daemon threads. If you try the warn_shutdown.py script attached to bpo-19442, you now get the warning with the patch! As a result, test_threading.test_4_daemon_threads() is also modified by my patch to ignore warnings :-) If I understood correctly, the patch only has an impact on daemon threads, the behaviour of classic threads is unchanged. |
Is it possible to write a test for this behavior? |
I'm afraid clearing thread states is a bit too brutal. What if some destructor relies on contents of the thread states (e.g. thread locals)? |
2013/11/4 Antoine Pitrou <report@bugs.python.org>:
When Py_Finalize() is called, only one Python thread hold the GIL. Is that correct? If yes, in which thread would the destructor be |
2013/11/4 Serhiy Storchaka <report@bugs.python.org>:
It is possible to test it manually using warn_shutdown.py attached to |
I am referring to the following part of your patch: + /* Clear the state of the current thread */ You are clearing the thread state of the currently executing thread, which doesn't sound right. |
Oh, I didn't realize that PyThreadState_Clear() clears also Python thread locals. Here is a new patch without PyThreadState_Clear(tstate) and with two unit tests:
|
neologix made a review on rietveld: There was an issue in finalize_threads-2.patch: the test didn't pass in release mode. I fixed it by adding -Wd to the command line option. I also replaced threading.Lock with threading.Event in the unit test to synchronize the two threads. => finalize_threads-3.patch |
New changeset c2a13acd5e2b by Victor Stinner in branch 'default': |
Typo: s/immediatly/immediately. I attached the patch. |
New changeset 10a8e676b87b by Victor Stinner in branch 'default': |
The fix raised a new issue: bpo-19565. I also saw a crash on Windows which is probably related: http://buildbot.python.org/all/builders/x86 Windows Server 2003 [SB] 3.x/builds/1717/steps/test/logs/stdio ====================================================================== Traceback (most recent call last):
File "E:\Data\buildslave\cpython\3.x.snakebite-win2k3r2sp2-x86\build\lib\test\test_threading.py", line 783, in test_4_daemon_threads
rc, out, err = assert_python_ok('-c', script)
File "E:\Data\buildslave\cpython\3.x.snakebite-win2k3r2sp2-x86\build\lib\test\script_helper.py", line 69, in assert_python_ok
return _assert_python(True, *args, **env_vars)
File "E:\Data\buildslave\cpython\3.x.snakebite-win2k3r2sp2-x86\build\lib\test\script_helper.py", line 55, in _assert_python
"stderr follows:\n%s" % (rc, err.decode('ascii', 'ignore')))
AssertionError: Process return code is 3221225477, stderr follows: |
I'm unable to reproduce the test_4_daemon_threads failure, can anyone try on Windows? |
Hum... For example, if we have: and the main thread exits, then fileobj will be deallocated. It's actually fairly easy to write a short crasher launching a deamon |
"It's actually fairly easy to write a short crasher launching a deamon I'm unable to write such crasher, can you please give an example? Are you able to crash Python on Linux or on Python 3.3? Does the threading test makes sense? |
I didn't see test_threading failing anymore recently, so I'm closing the issue. Thanks. |
Hm... Could someone review my message http://bugs.python.org/msg206028? Because AFAICT, this will lead to random crashes with daemon threads. |
I replied with questions and you didn't reply :-) |
New changeset 1166b3321012 by Victor Stinner in branch 'default': |
I reverted the changes because it introduced regressions. |
New changeset 9ce58a73b6b5 by Victor Stinner in branch '3.4': |
The change has been reverted in 2014. I reopen the issue since using daemon thraeds became safer in Python 3.9. As soon as a daemon thread attempts to take the GIL, it does exit immediately. With the commit eb4e2ae (bpo-39877), it should be even more reliable. Let me retry to fix this issue again. I reopen the issue and I wrote a new PR. |
asyncio_gc.py: script to reproduce bpo-20526 (copied from there), but ported to Python 3.9 (replace asyncio.async with asyncio.ensure_future). Sadly, Python with PR 18848 does crash when running asyncio_gc.py if I press CTRL+c twice:
It seems like the main thread does *not* hold the hold, the thread which crashed does. It should not happen: _PyRuntimeState_SetFinalizing(runtime, tstate) has been called and so no other thread should be able to take the GIL anymore, but exit immediately. |
I merged more changes in bpo-39877 which made possible to finally fix this issue. |
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: