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

gh-127041: Prevent new threads after an interpreter has started finalizing #127044

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ZeroIntensity
Copy link
Member

@ZeroIntensity ZeroIntensity commented Nov 19, 2024

cc @ericsnowcurrently

I decided to use a sentinel pointer instead of a new field like I originally did, which I think is a little better. I've left a few open issues as XXX comments :)

@ZeroIntensity ZeroIntensity marked this pull request as draft November 20, 2024 01:40
Python/pystate.c Show resolved Hide resolved
Python/pystate.c Outdated Show resolved Hide resolved
@ZeroIntensity
Copy link
Member Author

I'm not too sure how I broke the threading tests with this.

Python/pystate.c Show resolved Hide resolved
Python/pystate.c Show resolved Hide resolved
@ZeroIntensity
Copy link
Member Author

As it turns out, the threading aborts are a result of a separate bug, I just managed to trigger it by raising in close. It seems that we don't clean up threads in a subinterpreter properly:

import _interpreters

interp = _interpreters.create()
source = """
import threading

def task():
    time.sleep(100)

t = threading.Thread(target=task)
t.start()
"""
_interpreters.run_string(interp, source)

This causes an assertion failure on my end:

python: Python/pystate.c:1969: tstate_activate: Assertion `!tstate->_status.bound_gilstate || tstate == gilstate_tss_get((tstate->interp->runtime))' failed.

I'm pretty sure that the problem is that even though the interpreter doesn't have a threads.main, the remaining threads.head that finalize_subinterpreters tries to use is still running, and trying to attach to it is a big no-no. The easiest fix would be to call wait_for_thread_shutdown much earlier for subinterpreters (probably where the main interpreter does it), but that doesn't fix anything for C threads, so maybe it's better if we come up with a more robust fix that waits for all threads using a _PySemaphore or something like that. Thoughts, @ericsnowcurrently?

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

Successfully merging this pull request may close these issues.

2 participants