Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Implement PEP 788#133110

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
ZeroIntensity wants to merge30 commits intopython:main
base:main
Choose a base branch
Loading
fromZeroIntensity:pep-788-impl
Draft

Conversation

ZeroIntensity
Copy link
Member

Reference implementation forPEP-788.

Comment on lines 2434 to 2437
wait_for_thread_shutdown(tstate);

// Wrap up non-daemon native threads
wait_for_native_shutdown(tstate->interp);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this needs to be unified withwait_for_thread_shutdown. Threads created from Python may spawn threads in C and vice versa.

As currently written, I think you can get towait_for_native_shutdown() and then have a thread spawn athreading.Thread() that's not waited on.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Yeah, I thought this might be an issue. Especially since there seems to be some inclination for aPyThreadState_GetDaemon function, unifying these seems like a good idea.

My main concern is breakage towards people who are manually usingthreading._shutdown for whatever reason. We'd have to remove that if we treatthreading threads as native threads (or I guess we could havethreading._shutdown callwait_for_native_shutdown?).

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I think this problem is out of the scope of PEP 788. Pending calls or atexit handlers can also start threads, so we need to fix this in some other way. I think a loop that runs all these finalizer functions until there's nothing called anymore should work.

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree. Spawning threads from other threads is a very common operation. Spawning threads from atexit handlers is not.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Right, but we should fix it anyway. You can get some nifty fatal errors if you do something like this right now:

importatexitimportthreading@atexit.registerdefstart_thread():threading.Thread(target=print,args=("hello world",)).start()

My point is that we should implement that fix regardless of PEP 788, and then update the reference implementation once it lands. Is that ok?

Comment on lines 3328 to 3329
PyThreadState *save = _PyThreadState_GET();
if (save != NULL && save->interp == interp) {
Copy link
Contributor

@colesburycolesburyApr 28, 2025
edited
Loading

Choose a reason for hiding this comment

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

One of the properties ofPyGILState_Ensure() was that if you hade code that did something like:

Py_BEGIN_ALLOW_THREADS...PyGILState_Ensure();

Then thePyGILState_Ensure() code would reuse the same thread state; it wouldn't create a new one just because the thread state isn't active. The equivalent code withPyThreadState_Ensure() would create a new thread state.

This can come up when you release the GIL and make a possibly long running call into a C library function, which then calls back into Python.

  • I didn't see this in the PEP. I think it's worth mentioning if it's not already there.
  • Creating thread states unnecessarily can be expensive
  • Creating a new thread state means that you have different thread local variables and other weird behavior.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Ok, we can reuse the gilstate pointer for this case. That has the additional benefit of being compatible withPyGILState_Ensure at the same time. Thanks for bringing this up.

(I'll update the PEP in a big "round 1 comments" PR sometime tomorrow or in the next few days. I want to give people a chance to look at the current draft first, so I can get a good idea of what needs to change.)

}

static void
decrement_daemon_count(PyInterpreterState *interp)
Copy link
Contributor

Choose a reason for hiding this comment

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

minor, but this decrements the count ofnon-daemon threads, right? It's not a count ofdaemon threads.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Oh yeah, oops.

}
PyMutex_Unlock(&finalizing->mutex);

PyEvent_Wait(&finalizing->finished);
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to interrupt it with CTRL+C? If not, maybe PyEvent_WaitTimed() should be called in a loop and call PyErr_CheckSignals() time to time?

ZeroIntensity reacted with thumbs up emoji
PyThreadState_Ensure(PyInterpreterState *interp)
{
assert(interp != NULL);
_Py_ensured_tstate *entry = PyMem_RawMalloc(sizeof(_Py_ensured_tstate));
Copy link
Member

Choose a reason for hiding this comment

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

You might rename 'entry' to 'ensured'.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@vstinnervstinnervstinner left review comments

@colesburycolesburycolesbury left review comments

@ericsnowcurrentlyericsnowcurrentlyAwaiting requested review from ericsnowcurrentlyericsnowcurrently will be requested when the pull request is marked ready for reviewericsnowcurrently is a code owner

Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@ZeroIntensity@vstinner@colesbury

[8]ページ先頭

©2009-2025 Movatter.jp