Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

socketserver.ThreadingMixIn leaks running threads after server_close() #75416

Closed
vstinner opened this issue Aug 18, 2017 · 19 comments
Closed

socketserver.ThreadingMixIn leaks running threads after server_close() #75416

vstinner opened this issue Aug 18, 2017 · 19 comments
Labels
3.7 performance stdlib

Comments

@vstinner
Copy link
Member

@vstinner vstinner commented Aug 18, 2017

BPO 31233
Nosy @pitrou, @vstinner, @ned-deily, @vadmium
PRs
  • #3523
  • #3524
  • #5417
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2018-05-29.22:00:51.118>
    created_at = <Date 2017-08-18.16:24:59.372>
    labels = ['3.7', 'library', 'performance']
    title = 'socketserver.ThreadingMixIn leaks running threads after server_close()'
    updated_at = <Date 2019-06-17.01:28:15.814>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2019-06-17.01:28:15.814>
    actor = 'martin.panter'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-05-29.22:00:51.118>
    closer = 'vstinner'
    components = ['Library (Lib)']
    creation = <Date 2017-08-18.16:24:59.372>
    creator = 'vstinner'
    dependencies = []
    files = []
    hgrepos = ['376']
    issue_num = 31233
    keywords = ['patch']
    message_count = 19.0
    messages = ['300514', '300518', '300533', '300536', '302009', '302010', '302014', '302024', '302034', '302037', '304619', '307048', '311016', '311021', '311025', '311092', '316835', '318115', '345795']
    nosy_count = 4.0
    nosy_names = ['pitrou', 'vstinner', 'ned.deily', 'martin.panter']
    pr_nums = ['3523', '3524', '5417']
    priority = None
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'resource usage'
    url = 'https://bugs.python.org/issue31233'
    versions = ['Python 3.7']

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Aug 18, 2017

    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.

    @vstinner vstinner added performance 3.7 stdlib labels Aug 18, 2017
    @pitrou
    Copy link
    Member

    @pitrou pitrou commented Aug 18, 2017

    The difference is that letting a thread run doesn't create a zombie thread, so I don't think the issue is really similar.

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Aug 18, 2017

    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)

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Aug 18, 2017

    New changeset 6966960 by Victor Stinner in branch 'master':
    bpo-30830: test_logging uses threading_setup/cleanup (bpo-3137)
    6966960

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Sep 13, 2017

    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

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Sep 13, 2017

    Multiple test_logging tests have been skipped until this issue is fixed: see bpo-30830 and commit 6966960.

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Sep 13, 2017

    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.

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Sep 13, 2017

    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?

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Sep 13, 2017

    New changeset 97d7e65 by Victor Stinner in branch 'master':
    bpo-30830: logging.config.listen() calls server_close() (bpo-3524)
    97d7e65

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Sep 13, 2017

    New changeset b8f4163 by Victor Stinner in branch 'master':
    bpo-31233: socketserver.ThreadingMixIn.server_close() (bpo-3523)
    b8f4163

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Oct 19, 2017

    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?

    @Holabuenastardesleenvolosarchivos Holabuenastardesleenvolosarchivos mannequin added build type-security and removed performance labels Nov 25, 2017
    @berkerpeksag berkerpeksag added performance and removed build type-security labels Nov 25, 2017
    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Nov 27, 2017

    Klon666 added 96a6cbc 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...

    @ned-deily
    Copy link
    Member

    @ned-deily ned-deily commented Jan 28, 2018

    What's the status of this? Feature freeze is here.

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Jan 28, 2018

    What's the status of this?

    The status is bad. The commit b8f4163 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.

    @ned-deily
    Copy link
    Member

    @ned-deily ned-deily commented Jan 28, 2018

    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?

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented Jan 29, 2018

    New changeset db8189b by Victor Stinner in branch 'master':
    bpo-31233, bpo-31151: Document socketserver changes (bpo-5417)
    db8189b

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented May 16, 2018

    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.

    @vstinner
    Copy link
    Member Author

    @vstinner vstinner commented May 29, 2018

    The bug has been fixed in Python 3.7.

    @vstinner vstinner closed this May 29, 2018
    @vadmium
    Copy link
    Member

    @vadmium vadmium commented Jun 17, 2019

    FYI the change here to remember all the thread objects ever created looks to be the cause of the memory leak reported in bpo-37193

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 performance stdlib
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants