Skip to content

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
@ZeroIntensity
Copy link
Member Author

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

@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