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-124872: Mark the thread's default context as entered #125638

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

Conversation

rhansen
Copy link
Contributor

@rhansen rhansen commented Oct 17, 2024

Starting with commit 843d28f (temporarily reverted in d3c82b9 and restored in commit bee112a), it is now technically possible to access a thread's default context created by context_get. Mark that context as entered so that users cannot push that context onto the thread's stack a second time, which would cause a cycle.

This change also causes a CONTEXT_SWITCHED event to be emitted when the default context is created, which might be important in some use cases.

Also exit the default context when the thread exits, for symmetry and in case the user wants to re-enter it for some reason.

(Even if the CONTEXT_SWITCHED event is removed, entering the default context is good defensive practice, and the consistent treatment of all contexts on the stack makes it easier to understand the code.)

@rhansen rhansen marked this pull request as draft October 17, 2024 07:35
@rhansen rhansen force-pushed the gh-124872-enter-initial branch 2 times, most recently from 11c4709 to ea46685 Compare October 17, 2024 08:16
@rhansen rhansen marked this pull request as ready for review October 17, 2024 08:23
Starting with commit 843d28f
(temporarily reverted in d3c82b9 and
restored in commit bee112a), it is
now technically possible to access a thread's default context created
by `context_get`.  Mark that context as entered so that users cannot
push that context onto the thread's stack a second time, which would
cause a cycle.

This change also causes a `CONTEXT_SWITCHED` event to be emitted when
the default context is created, which might be important in some use
cases.

Also exit the default context when the thread exits, for symmetry and
in case the user wants to re-enter it for some reason.

(Even if the `CONTEXT_SWITCHED` event is removed, entering the default
context is good defensive practice, and the consistent treatment of
all contexts on the stack makes it easier to understand the code.)
@rhansen rhansen force-pushed the gh-124872-enter-initial branch from ea46685 to 5337fc3 Compare October 17, 2024 17:12
@ZeroIntensity ZeroIntensity added skip news 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section labels Oct 18, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @ZeroIntensity for commit 5337fc3 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Oct 18, 2024
@ZeroIntensity
Copy link
Member

I'm testing this with the buildbots this time--I don't want another refleak fiasco :)

@ZeroIntensity ZeroIntensity self-requested a review October 18, 2024 11:21
@rhansen rhansen requested review from ZeroIntensity and 1st1 November 7, 2024 08:35
@ZeroIntensity
Copy link
Member

I'll review this further today. I suspect this won't play well if the calling thread state is in another interpreter.

Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this! Unfortunately, there needs to be some significant reworking if we want to get this merged. (I would suggest marking it as a draft in the meantime.)

The main issue is that running callbacks in PyThreadState_Clear is really, really bad for subinterpreters. Right now, we assume that PyThreadState_Clear can't really run arbitrary Python code, which is important for the finalization process.

Namely, the runtime has to finalize subinterpreters automatically if the user didn't do it, but that has to happen pretty late during finalization, so it's important that shutting down a subinterpreter can't end up doing something evil. With this PR, you can now run arbitrary Python code via a callback, which is bound to cause lots of unexpected bugs. For example, this causes a segfault on my end:

static int
my_callback(PyContextEvent event, PyObject *op)
{
     return PyRun_SimpleString("import _interpreters; _interpreters.create()")
}

This ends up creating a new thread state when the interpreter doesn't expect it, which breaks things. There's not any easy way around this, so I suggest we deal with subinterpreters in a different way.

My approach would be:

  • Ditch doing this from PyThreadState_Clear, we don't want to run code in that.
  • Find somewhere else, much earlier during finalization, where we can run the callbacks for subinterpreters. Ideally, this should be before any threads have been cleaned up and while the target interpreter (and all the other interpreters!) are fully intact, and creating new ones or starting new threads won't break things.
  • When finding that place, think very hard about "what could I break here"--even finalizing on the interpreter or thread state being 1 is enough to cause assertion failures.
  • Finally, add some tests for subinterpreters to make sure we can't break anything. We need to be very careful, because this is essentially a way to run things upon interpreter finalization.

@rhansen rhansen marked this pull request as draft November 9, 2024 21:54
@1st1
Copy link
Member

1st1 commented Nov 14, 2024

Good review, @ZeroIntensity!

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.

4 participants