-
-
Notifications
You must be signed in to change notification settings - Fork 32k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Implement PEP 788 #133110
Conversation
…'t work and isn't thread-safe.
Python/pylifecycle.c
Outdated
wait_for_thread_shutdown(tstate); | ||
|
||
// Wrap up non-daemon native threads | ||
wait_for_native_shutdown(tstate->interp); |
There was a problem hiding this comment.
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 with wait_for_thread_shutdown
. Threads created from Python may spawn threads in C and vice versa.
As currently written, I think you can get to wait_for_native_shutdown()
and then have a thread spawn a threading.Thread()
that's not waited on.
There was a problem hiding this comment.
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 a PyThreadState_GetDaemon
function, unifying these seems like a good idea.
My main concern is breakage towards people who are manually using threading._shutdown
for whatever reason. We'd have to remove that if we treat threading
threads as native threads (or I guess we could have threading._shutdown
call wait_for_native_shutdown
?).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
import atexit
import threading
@atexit.register
def start_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?
Python/pystate.c
Outdated
PyThreadState *save = _PyThreadState_GET(); | ||
if (save != NULL && save->interp == interp) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the properties of PyGILState_Ensure()
was that if you hade code that did something like:
Py_BEGIN_ALLOW_THREADS
...
PyGILState_Ensure();
Then the PyGILState_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 with PyThreadState_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.
There was a problem hiding this comment.
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 with PyGILState_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.)
Python/pystate.c
Outdated
} | ||
|
||
static void | ||
decrement_daemon_count(PyInterpreterState *interp) |
There was a problem hiding this comment.
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 of non-daemon threads, right? It's not a count of daemon threads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, oops.
Python/pylifecycle.c
Outdated
} | ||
PyMutex_Unlock(&finalizing->mutex); | ||
|
||
PyEvent_Wait(&finalizing->finished); |
There was a problem hiding this comment.
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?
Python/pystate.c
Outdated
PyThreadState_Ensure(PyInterpreterState *interp) | ||
{ | ||
assert(interp != NULL); | ||
_Py_ensured_tstate *entry = PyMem_RawMalloc(sizeof(_Py_ensured_tstate)); |
There was a problem hiding this comment.
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'.
Reference implementation for PEP-788.