Issue 31233: socketserver.ThreadingMixIn leaks running threads after server_close() (original) (raw)

Created on 2017-08-18 16:24 by vstinner, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 3523 merged vstinner,2017-09-13 00:16
PR 3524 merged vstinner,2017-09-13 01:12
PR 5417 merged vstinner,2018-01-29 08:36

| Repositories containing patches | | | | | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | | | | https://github.com/python/cpython/commit/96a6cbc5b0aee267f47e5efb50fba183b52cd8c6 | | | |

Messages (19)
msg300514 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-08-18 16:24
test_logging has a race condition: sometimes, it leaks dangling threads on FreeBSD: bpo-30830. I identified the root issue: socketserver.ThreadingMixIn spawns threads without waiting for their completion in server_close(). This issue is very similar to socketserver.ForkingMixIn which leaks child processes and so create zombie processes. See bpo-31151.
msg300518 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-08-18 17:01
The difference is that letting a thread run doesn't create a zombie thread, so I don't think the issue is really similar.
msg300533 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-08-18 21:35
Oh, test_socketserver just failed with ENV_CHANGED on AMD64 Windows7 SP1 3.x: http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.x/builds/865/steps/test/logs/stdio test_TCPServer (test.test_socketserver.SocketServerTest) ... creating server (...) waiting for server done Warning -- threading_cleanup() failed to cleanup -1 threads after 5 sec (count: 0, dangling: 1)
msg300536 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-08-18 21:47
New changeset 6966960468327c958b03391f71f24986bd697307 by Victor Stinner in branch 'master': bpo-30830: test_logging uses threading_setup/cleanup (#3137) https://github.com/python/cpython/commit/6966960468327c958b03391f71f24986bd697307
msg302009 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-13 00:04
Another recent example while testing bpo-31234: test_ThreadingUDPServer (test.test_socketserver.SocketServerTest) ... (...) done Warning -- threading_cleanup() detected 1 leaked threads (count: 1, dangling: 2) ok
msg302010 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-13 00:05
Multiple test_logging tests have been skipped until this issue is fixed: see bpo-30830 and commit 6966960468327c958b03391f71f24986bd697307.
msg302014 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-13 00:42
Attached PR 3523 is a temporary solution until we agree how to handle these threads and child processes in socketserver. I wrote the PR to fix "dangling threads" warnings to fix random buildbot failures. The PR fixes warnings but also reenable test_logging tests which are currently skipped. These tests were skipped by bpo-30830 to prevent random buildbot failures. See also the thread on the python-dev mailing list to discuss about bpo-31151 "socketserver.ForkingMixIn.server_close() leaks zombie processes": https://mail.python.org/pipermail/python-dev/2017-August/148826.html I tag this issue as release blocker as a remainder that we have to agree how to handle threads/processes before Python 3.7 feature freeze.
msg302024 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-13 01:17
Again, PR 3523 is a temporary fix to unblock the situation. For a proper fix, I have a question: Should server_close() calls self.close_request(request) on thread requests?
msg302034 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-13 08:44
New changeset 97d7e65dfed1d42d40d9bc2f630af56240555f02 by Victor Stinner in branch 'master': bpo-30830: logging.config.listen() calls server_close() (#3524) https://github.com/python/cpython/commit/97d7e65dfed1d42d40d9bc2f630af56240555f02
msg302037 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-13 08:47
New changeset b8f4163da30e16c7cd58fe04f4b17e38d53cd57e by Victor Stinner in branch 'master': bpo-31233: socketserver.ThreadingMixIn.server_close() (#3523) https://github.com/python/cpython/commit/b8f4163da30e16c7cd58fe04f4b17e38d53cd57e
msg304619 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-10-19 13:32
The current code leaks memory since it never clears threads which completed. We need a cleanup function similar to ForkingMixIn.collect_children() which is called by handle_timeout() and service_actions(). We can check if a thread is alive: thread.is_alive(), to decide to remove it or not. Moreover, maybe we should keep a list of weak references?
msg307048 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-11-27 11:04
Klon666 added https://github.com/python/cpython/commit/96a6cbc5b0aee267f47e5efb50fba183b52cd8c6 repository. But this URL is unrelated. It looks like a bot spamming the bug tracker and I don't see how to remove the URL...
msg311016 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2018-01-28 22:20
What's the status of this? Feature freeze is here.
msg311021 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-01-28 22:32
> What's the status of this? The status is bad. The commit b8f4163da30e16c7cd58fe04f4b17e38d53cd57e is backward incompatible, it's not documented in the "Porting the Python 3.7" section of What's New in Python 3.7, and it's not possible to opt-in for the previous behaviour. > Feature freeze is here. IMHO it's ok to post-pone this issue to beta2. IMHO it's better to wait until all threads complete on server_close(), and it's possible to workaround the issue using daemon_threads = True which can be seen as the option to opt-in for the previous behaviour.
msg311025 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2018-01-28 22:49
If we're going to leave this open until 3.7.0b2, can you at least add a brief warning in the "Porting section" for b1 that the issue is unresolved?
msg311092 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-01-29 11:10
New changeset db8189bb8db609ca3993dec31fb95fdbe134469a by Victor Stinner in branch 'master': bpo-31233, bpo-31151: Document socketserver changes (#5417) https://github.com/python/cpython/commit/db8189bb8db609ca3993dec31fb95fdbe134469a
msg316835 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-05-16 16:28
I created bpo-33540: "socketserver: Add an opt-in option to get Python 3.6 behaviour on server_close()". I set the release blocker priority on this new issue.
msg318115 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2018-05-29 22:00
The bug has been fixed in Python 3.7.
msg345795 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2019-06-17 01:28
FYI the change here to remember all the thread objects ever created looks to be the cause of the memory leak reported in Issue 37193
History
Date User Action Args
2022-04-11 14:58:50 admin set github: 75416
2019-06-17 01:28:15 martin.panter set nosy: + martin.pantermessages: +
2018-05-29 22:00:51 vstinner set status: open -> closedresolution: fixedmessages: + stage: patch review -> resolved
2018-05-16 16:28:14 vstinner set priority: deferred blocker -> messages: +
2018-01-29 11:10:24 vstinner set messages: +
2018-01-29 08:36:21 vstinner set pull_requests: + <pull%5Frequest5251>
2018-01-28 22:49:01 ned.deily set messages: +
2018-01-28 22:32:55 vstinner set messages: +
2018-01-28 22:20:30 ned.deily set messages: +
2017-11-27 11:04:39 vstinner set messages: +
2017-11-25 14:30:40 berker.peksag set nosy: - Tu madretype: security -> resource usagecomponents: - Build
2017-11-25 13:56:24 Tu madre set nosy: + Tu madretype: resource usage -> securitycomponents: + Build
2017-11-25 13:48:20 Tu madre set hgrepos: + hgrepo376
2017-10-19 13:32:28 vstinner set messages: +
2017-09-18 19:39:47 ned.deily set priority: release blocker -> deferred blocker
2017-09-13 09:36:22 vstinner unlink issue30830 dependencies
2017-09-13 08:47:24 vstinner set messages: +
2017-09-13 08:44:14 vstinner set messages: +
2017-09-13 01:17:24 vstinner set messages: +
2017-09-13 01:12:28 vstinner set pull_requests: + <pull%5Frequest3524>
2017-09-13 00:42:51 vstinner set priority: normal -> release blockernosy: + ned.deilymessages: +
2017-09-13 00:16:16 vstinner set keywords: + patchstage: patch reviewpull_requests: + <pull%5Frequest3521>
2017-09-13 00:05:24 vstinner set messages: +
2017-09-13 00:04:51 vstinner set messages: +
2017-08-18 21:47:56 vstinner set messages: +
2017-08-18 21:35:30 vstinner set messages: +
2017-08-18 17:29:29 vstinner link issue30830 dependencies
2017-08-18 17:01:04 pitrou set nosy: + pitroumessages: +
2017-08-18 16:24:59 vstinner create