classification
Title: fix for bpo-36402 (threading._shutdown() race condition) causes reference leak
Type: resource usage Stage: patch review
Components: Library (Lib) Versions: Python 3.9, Python 3.8, Python 3.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: anselm.kruis, koobs, pablogsal, vstinner
Priority: normal Keywords: patch

Created on 2019-08-07 16:22 by anselm.kruis, last changed 2019-10-11 15:42 by pablogsal.

Files
File name Uploaded Description Edit
threading-leak-test-case.diff anselm.kruis, 2019-08-07 16:22
Pull Requests
URL Status Linked Edit
PR 15175 open anselm.kruis, 2019-08-08 08:23
PR 15228 merged vstinner, 2019-08-12 15:24
PR 15336 closed miss-islington, 2019-08-19 22:37
PR 15337 closed miss-islington, 2019-08-19 22:37
PR 15338 merged vstinner, 2019-08-19 23:28
Messages (7)
msg349173 - (view) Author: Anselm Kruis (anselm.kruis) * Date: 2019-08-07 16:22
Starting with commit 468e5fec (bpo-36402: Fix threading._shutdown() race condition (GH-13948)) the following trivial test case leaks one reference and one memory block.

class MiscTestCase(unittest.TestCase):
    def test_without_join(self):
        # Test that a thread without join does not leak references.
        # Use a debug build and run "python -m test -R: test_threading"
        threading.Thread().start()

Attached is a patch, that adds this test case to Lib/test/test_threading.py. After you apply this patch "python -m test -R: test_threading" leaks one (additional) reference. This leak is also present in Python 3.7.4 and 3.8.

I'm not sure, if it correct not to join a thread, but it did work flawlessly and didn't leak in previous releases.
I didn't analyse the root cause yet.
msg349225 - (view) Author: Anselm Kruis (anselm.kruis) * Date: 2019-08-08 08:47
The root cause for the reference leak is the global set threading._shutdown_locks. It contains Thread._tstate_lock locks of non-daemon threads. If a non-daemon thread terminates and no other thread joins the terminated thread, the _tstate_lock remains in threading._shutdown_locks forever.

I could imagine that a long running server could accumulate many locks in threading._shutdown_locks over time. Therefore the leak should be fixed.

There are probably several ways to deal with this issue. A straight forward approach is to discard the lock from within `tstate->on_delete` hook, that is function "void release_sentinel(void *)" in _threadmodule.c. Pull request (GH-15175) implements this idea. Eventually I should add another C-Python specific test-case to the PR.
msg349973 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-08-19 22:37
New changeset d3dcc92778807ae8f7ebe85178f36a29711cd478 by Victor Stinner in branch 'master':
bpo-37788: Fix a reference leak if a thread is not joined (GH-15228)
https://github.com/python/cpython/commit/d3dcc92778807ae8f7ebe85178f36a29711cd478
msg349977 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-08-19 23:29
> https://github.com/python/cpython/commit/d3dcc92778807ae8f7ebe85178f36a29711cd478

This change introduced a regression :-(
https://github.com/python/cpython/pull/15228#issuecomment-522792133

I created PR 15338 to revert it
msg349980 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-08-19 23:47
New changeset d11c2c607768fa549b1aed7899edc061b2ebf19f by Victor Stinner in branch 'master':
Revert "bpo-37788: Fix a reference leak if a thread is not joined (GH-15228)" (GH-15338)
https://github.com/python/cpython/commit/d11c2c607768fa549b1aed7899edc061b2ebf19f
msg354460 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-10-11 15:36
Copy of wmanley's comment:
https://github.com/python/cpython/pull/13948#issuecomment-541076882
"""
This caused a regression for people overriding Thread.join to implement custom thread interruption. See https://stackoverflow.com/questions/50486083/ending-non-daemon-threads-when-shutting-down-an-interactive-python-session
"""
msg354463 - (view) Author: Pablo Galindo Salgado (pablogsal) * (Python committer) Date: 2019-10-11 15:42
Hummm....is this really a regression? The docs say:

> No other methods (except for the constructor) should be overridden in a subclass. In other words, only override the __init__() and run() methods of this class.

So if someone is overriding join() they are out of contract
History
Date User Action Args
2019-10-11 15:42:52pablogsalsetnosy: + pablogsal
messages: + msg354463
2019-10-11 15:36:12vstinnersetmessages: + msg354460
2019-08-19 23:47:10vstinnersetmessages: + msg349980
2019-08-19 23:30:53vstinnerlinkissue37889 superseder
2019-08-19 23:29:05vstinnersetmessages: + msg349977
2019-08-19 23:28:01vstinnersetpull_requests: + pull_request15055
2019-08-19 22:37:45miss-islingtonsetpull_requests: + pull_request15054
2019-08-19 22:37:31miss-islingtonsetpull_requests: + pull_request15053
2019-08-19 22:37:20vstinnersetmessages: + msg349973
2019-08-12 15:24:20vstinnersetpull_requests: + pull_request14952
2019-08-09 12:31:44koobssetnosy: + koobs
2019-08-08 08:48:42anselm.kruissetnosy: + vstinner
2019-08-08 08:47:16anselm.kruissetmessages: + msg349225
2019-08-08 08:23:10anselm.kruissetstage: patch review
pull_requests: + pull_request14906
2019-08-07 16:22:52anselm.kruiscreate