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

Created on 2017-08-18 16:24 by vstinner, last changed 2018-05-29 22:00 by vstinner. 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 (18)
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.
History
Date User Action Args
2018-05-29 22:00:51vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg318115

stage: patch review -> resolved
2018-05-16 16:28:14vstinnersetpriority: deferred blocker ->

messages: + msg316835
2018-01-29 11:10:24vstinnersetmessages: + msg311092
2018-01-29 08:36:21vstinnersetpull_requests: + pull_request5251
2018-01-28 22:49:01ned.deilysetmessages: + msg311025
2018-01-28 22:32:55vstinnersetmessages: + msg311021
2018-01-28 22:20:30ned.deilysetmessages: + msg311016
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