Title: socketserver.ThreadingMixIn leaks running threads after server_close()
Type: resource usage Stage: patch review
Components: Library (Lib) Versions: Python 3.7
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: ned.deily, pitrou, vstinner
Priority: deferred blocker Keywords: patch

Created on 2017-08-18 16:24 by vstinner, last changed 2017-11-27 11:04 by vstinner.

Pull Requests
URL Status Linked Edit
PR 3523 merged vstinner, 2017-09-13 00:16
PR 3524 merged vstinner, 2017-09-13 01:12
Repositories containing patches
Messages (12)
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:

test_TCPServer (test.test_socketserver.SocketServerTest) ... creating server
waiting for server

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)
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) ...
Warning -- threading_cleanup() detected 1 leaked threads (count: 1, dangling: 2)
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":

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)
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)
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 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...
Date User Action Args
2017-11-27 11:04:39vstinnersetmessages: + msg307048
2017-11-25 14:30:40berker.peksagsetnosy: - Tu madre
type: security -> resource usage
components: - Build
2017-11-25 13:56:24Tu madresetnosy: + Tu madre
type: resource usage -> security
components: + Build
2017-11-25 13:48:20Tu madresethgrepos: + hgrepo376
2017-10-19 13:32:28vstinnersetmessages: + msg304619
2017-09-18 19:39:47ned.deilysetpriority: release blocker -> deferred blocker
2017-09-13 09:36:22vstinnerunlinkissue30830 dependencies
2017-09-13 08:47:24vstinnersetmessages: + msg302037
2017-09-13 08:44:14vstinnersetmessages: + msg302034
2017-09-13 01:17:24vstinnersetmessages: + msg302024
2017-09-13 01:12:28vstinnersetpull_requests: + pull_request3524
2017-09-13 00:42:51vstinnersetpriority: normal -> release blocker
nosy: + ned.deily
messages: + msg302014

2017-09-13 00:16:16vstinnersetkeywords: + patch
stage: patch review
pull_requests: + pull_request3521
2017-09-13 00:05:24vstinnersetmessages: + msg302010
2017-09-13 00:04:51vstinnersetmessages: + msg302009
2017-08-18 21:47:56vstinnersetmessages: + msg300536
2017-08-18 21:35:30vstinnersetmessages: + msg300533
2017-08-18 17:29:29vstinnerlinkissue30830 dependencies
2017-08-18 17:01:04pitrousetnosy: + pitrou
messages: + msg300518
2017-08-18 16:24:59vstinnercreate