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

Open
wants to merge 3 commits into
base: master
from

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

@vstinner vstinner commented Dec 16, 2020

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

@vstinner vstinner commented Dec 16, 2020

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

@vstinner vstinner commented Dec 17, 2020

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 Dec 18, 2020
@vstinner
Copy link
Member Author

@vstinner vstinner commented Dec 18, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.