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.

Sign up for GitHub

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

Jump to bottom

Removing thread reference in thread results in leaked reference #86429

Closed
jaraco opened this issue Nov 4, 2020 · 5 comments
Closed

Removing thread reference in thread results in leaked reference #86429

jaraco opened this issue Nov 4, 2020 · 5 comments
Labels
3.10 only security fixes

Comments

@jaraco
Copy link
Member

jaraco commented Nov 4, 2020

BPO 42263
Nosy @ronaldoussoren, @jaraco, @vadmium
Superseder
  • bpo-37788: fix for bpo-36402 (threading._shutdown() race condition) causes reference leak
    		•

    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 2020-11-05.01:28:49.433>
created_at = <Date 2020-11-04.19:42:45.893>
labels = ['3.10']
title = 'Removing thread reference in thread results in leaked reference'
updated_at = <Date 2020-11-05.01:28:49.432>
user = 'https://github.com/jaraco'

    bugs.python.org fields:

    activity = <Date 2020-11-05.01:28:49.432>
actor = 'jaraco'
assignee = 'none'
closed = True
closed_date = <Date 2020-11-05.01:28:49.433>
closer = 'jaraco'
components = []
creation = <Date 2020-11-04.19:42:45.893>
creator = 'jaraco'
dependencies = []
files = []
hgrepos = []
issue_num = 42263
keywords = []
message_count = 5.0
messages = ['380353', '380357', '380381', '380388', '380390']
nosy_count = 3.0
nosy_names = ['ronaldoussoren', 'jaraco', 'martin.panter']
pr_nums = []
priority = 'normal'
resolution = 'duplicate'
stage = 'resolved'
status = 'closed'
superseder = '37788'
type = None
url = 'https://bugs.python.org/issue42263'
versions = ['Python 3.10']
    @jaraco
    Copy link
    Member Author

    jaraco commented Nov 4, 2020

    In bpo-37193, I'd worked on an implementation in which a thread reference would be removed as the thread was closing, but this led to a memory leak caught by the buildbots (https://bugs.python.org/issue37193#msg380172).

    As I tracked down the issue in #67316, I discovered that removing the reference to the thread from within the thread triggered the reference leak detection.

    I've distilled that behavior into its own test which fails on master:

    cpython master $ git diff
diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py
index e0e5406ac2..2b4924d4d0 100644
--- a/Lib/test/test_threading.py
+++ b/Lib/test/test_threading.py
@@ -443,6 +443,15 @@ def _run(self, other_ref, yet_another):
                          msg=('%d references still around' %
                               sys.getrefcount(weak_raising_cyclic_object())))
 
+    def test_remove_thread_ref_in_thread(self):
+        def remove_ref():
+            threads[:] = []
+
+        threads = [
+            threading.Thread(target=remove_ref),
+        ]
+        threads[0].start()
+
     def test_old_threading_api(self):
         # Just a quick sanity check to make sure the old method names are
         # still present

    Running that test with refcount checks leads to this failure:

    cpython master $ ./python.exe Tools/scripts/run_tests.py -R 3:3 test.test_threading -m test_remove_thread_ref_in_thread
/Users/jaraco/code/public/cpython/python.exe -u -W default -bb -E -m test -r -w -j 0 -u all,-largefile,-audio,-gui -R 3:3 test.test_threading -m test_remove_thread_ref_in_thread
Using random seed 8650903
0:00:00 load avg: 1.76 Run tests in parallel using 10 child processes
0:00:01 load avg: 1.78 [1/1/1] test.test_threading failed

== Tests result: FAILURE ==

1 test failed:
    test.test_threading
0:00:01 load avg: 1.78
0:00:01 load avg: 1.78 Re-running failed tests in verbose mode
0:00:01 load avg: 1.78 Re-running test.test_threading in verbose mode
beginning 6 repetitions
123456
......
test.test_threading leaked [1, 1, 1] references, sum=3
test.test_threading leaked [3, 1, 1] memory blocks, sum=5
beginning 6 repetitions
123456
test_remove_thread_ref_in_thread (test.test_threading.ThreadTests) ... ok

----------------------------------------------------------------------

Ran 1 test in 0.001s

OK
.test_remove_thread_ref_in_thread (test.test_threading.ThreadTests) ... ok

----------------------------------------------------------------------

Ran 1 test in 0.001s

OK
.test_remove_thread_ref_in_thread (test.test_threading.ThreadTests) ... ok

----------------------------------------------------------------------

Ran 1 test in 0.001s

OK
.test_remove_thread_ref_in_thread (test.test_threading.ThreadTests) ... ok

----------------------------------------------------------------------

Ran 1 test in 0.001s

OK
.test_remove_thread_ref_in_thread (test.test_threading.ThreadTests) ... ok

----------------------------------------------------------------------

Ran 1 test in 0.001s

OK
.test_remove_thread_ref_in_thread (test.test_threading.ThreadTests) ... ok

----------------------------------------------------------------------

Ran 1 test in 0.001s

OK
.
test.test_threading leaked [1, 1, 1] references, sum=3
test.test_threading leaked [1, 1, 1] memory blocks, sum=3
1 test failed again:
    test.test_threading

== Tests result: FAILURE then FAILURE ==

1 test failed:
    test.test_threading

1 re-run test:
    test.test_threading

Total duration: 2.0 sec
Tests result: FAILURE then FAILURE

    Is that behavior by design? Is it simply not possible to remove a reference to a thread from within the thread?

    @jaraco jaraco added 3.10 only security fixes labels Nov 4, 2020
    @ronaldoussoren
    Copy link
    Contributor

    Could this be a race condition? The thread that's created in the test is not waited on (join), it may or may not have exited by the time the test function returns.

    @jaraco
    Copy link
    Member Author

    jaraco commented Nov 4, 2020

    I don't think it's a race condition for two reasons: adding a time.sleep(1) after .start still raises errors, and in bpo-37193, there were 10 threads created, with at least 9 of those reaching termination before the test ended, yet it showed 10 reference leaks.

    If you join the thread in the test, the leak is not detected. However, I believe that's because, in order to join on the thread, you must also hold a handle to the thread, so the condition isn't triggered.

    @vadmium
    Copy link
    Member

    vadmium commented Nov 5, 2020

    Maybe this is related to (or duplicate of) bpo-37788? Python 3.7 has a regression where threads that are never joined cause leaks; previous code was written assuming you didn't need to join threads.

    Do you still see the leak even if you don't clear the "threads" list (change the target to a no-op), or if you never create a list in the first place?

    @jaraco
    Copy link
    Member Author

    jaraco commented Nov 5, 2020

    Yes, I agree it's a duplicate of bpo-37788.

    And yes, it does still leak if the list is never created or if the target is a no-op.

    @jaraco jaraco closed this as completed Nov 5, 2020
    @jaraco jaraco closed this as completed Nov 5, 2020
    @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
    Assignees
    No one assigned
    Labels
    3.10 only security fixes
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    3 participants
    @jaraco @ronaldoussoren @vadmium