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

bpo-1635741: Port _thread to multiphase init #23811

Merged
merged 3 commits into from
Dec 18, 2020
Merged

bpo-1635741: Port _thread to multiphase init #23811

merged 3 commits into from
Dec 18, 2020

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Dec 16, 2020

Port the _thread extension module to the multiphase initialization
API (PEP 489) and convert its static types to heap types.

https://bugs.python.org/issue1635741

Port the _thread extension module to the multiphase initialization
API (PEP 489) and convert its static types to heap types.
@vstinner
Copy link
Member Author

Sadly, the following test leaks:

    def test_leak(self):
        code = textwrap.dedent(r"""
            import os
            import _thread

            lock = _thread.allocate_lock()

            def _after_fork():
                pass

            os.register_at_fork(after_in_child=_after_fork)
        """)
        test.support.run_in_subinterp(code)

Output:

$ ./python -m test test_threading -R 3:3 -m test_leak
0:00:00 load avg: 0.77 Run tests sequentially
0:00:00 load avg: 0.77 [1/1] test_threading
beginning 6 repetitions
123456
......
test_threading leaked [56, 56, 56] references, sum=168
test_threading leaked [21, 21, 21] memory blocks, sum=63
test_threading failed

== Tests result: FAILURE ==

1 test failed:
    test_threading

Total duration: 724 ms
Tests result: FAILURE

I will investigate later why it leaks.

@vstinner vstinner changed the title bpo-1635741: Port _thread to multiphase init [WIP] bpo-1635741: Port _thread to multiphase init Dec 16, 2020
@vstinner
Copy link
Member Author

Workaround for the leak:

diff --git a/Python/pystate.c b/Python/pystate.c
index ead25b08d7..66ad5d6514 100644
--- a/Python/pystate.c
+++ b/Python/pystate.c
@@ -311,6 +311,7 @@ interpreter_clear(PyInterpreterState *interp, PyThreadState *tstate)
 
     /* Last garbage collection on this interpreter */
     _PyGC_CollectNoFail(tstate);
+    _PyGC_CollectNoFail(tstate);
     _PyGC_Fini(tstate);
 
     /* We don't clear sysdict and builtins until the end of this function.

@vstinner
Copy link
Member Author

Example of leaking tests:

./python -m test test_threading -R 3:3 -m test_threads_join -m test_threads_join_2 

These tests run import threading in a subinterpreter, and Lib/threading.py calls os.register_at_fork() which keeps the threading module dictionary alive longer than expected: I created https://bugs.python.org/issue42671 to try to fix this issue.

@vstinner vstinner changed the title [WIP] bpo-1635741: Port _thread to multiphase init bpo-1635741: Port _thread to multiphase init Dec 18, 2020
@vstinner vstinner marked this pull request as ready for review December 18, 2020 00:21
@vstinner
Copy link
Member Author

I fixed the leak by adding a traverse function to the lock type (and convert it to a GC type).

@vstinner vstinner merged commit 6104013 into python:master Dec 18, 2020
@vstinner vstinner deleted the thread_state branch December 18, 2020 00:39
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
Port the _thread extension module to the multiphase initialization
API (PEP 489) and convert its static types to heap types.

Add a traverse function to the lock type, so the garbage collector
can break reference cycles.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants