Fix thread leak in Python 3.7 #340 #356
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
The leak is caused by the fact that in Python 3.7, the default behavior of the `ThreadingMixin` is to use no daemon threads, but to request to block on threads on close. Because of that, it collects references to all created threads, creating the "leak": https://github.com/python/cpython/blob/v3.7.0/Lib/socketserver.py#L661 * Python 3.7: `block_on_close` is `True`: https://github.com/python/cpython/blob/v3.7.0/Lib/socketserver.py#L635 * Python 3.6: `_block_on_close` is `False`: https://github.com/python/cpython/blob/v3.6.7/Lib/socketserver.py#L639 * Python 2.7: There is no `block_on_close`, thus no logic for collecting references: https://github.com/python/cpython/blob/v2.7.15/Lib/SocketServer.py#L582 Fix by setting `daemon_threads` to `True`, which in our case should be a reasonable setting for all Python versions. Also, the new in Python 3.7 `ThreadingHTTPServer` stdlib class also sets it by default: https://github.com/python/cpython/blob/v3.7.0/Lib/http/server.py#L144 Signed-off-by: Sebastian Brandt <sebastian.brandt@friday.de>
sbrandtb force-pushed the fix_thread_leak_in_python_3_7 branch from b5e2040 to ef36e1c Compare 
| Thanks for digging into this. Can you add a comment to the code explaining this briefly? |
| @brian-brazil Added some comments. |
Signed-off-by: Sebastian Brandt <sebastian.brandt@friday.de>
sbrandtb force-pushed the fix_thread_leak_in_python_3_7 branch from a9b7b6d to 51de8ba Compare 
| Thanks! |
wojons added a commit to wojons/dnslib that referenced this pull request Aug 13, 2020
#https://bugs.python.org/issue37788 #prometheus/client_python#356 daemon_threads = True # fix for python 3.7 please see links above for details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
The leak is caused by the fact that in Python 3.7, the default behavior
of the
ThreadingMixinis to use no daemon threads, but to request toblock on threads on close. Because of that, it collects references to
all created threads, creating the "leak":
https://github.com/python/cpython/blob/v3.7.0/Lib/socketserver.py#L661
block_on_closeisTrue: https://github.com/python/cpython/blob/v3.7.0/Lib/socketserver.py#L635_block_on_closeisFalse: https://github.com/python/cpython/blob/v3.6.7/Lib/socketserver.py#L639block_on_close, thus no logic for collectingreferences: https://github.com/python/cpython/blob/v2.7.15/Lib/SocketServer.py#L582
Fix by setting
daemon_threadstoTrue, which in our case should be areasonable setting for all Python versions. Also, the new in Python 3.7
ThreadingHTTPServerstdlib class also sets it by default:https://github.com/python/cpython/blob/v3.7.0/Lib/http/server.py#L144
Fixes #340