Implement PEP 788 #133110
Implement PEP 788 #133110
Conversation
…'t work and isn't thread-safe.
| wait_for_thread_shutdown(tstate); | ||
|
|
||
| // Wrap up non-daemon native threads | ||
| wait_for_native_shutdown(tstate->interp); |
There was a problem hiding this comment.
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.
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?).
| PyThreadState *save = _PyThreadState_GET(); | ||
| if (save != NULL && save->interp == interp) { |
There was a problem hiding this comment.
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.
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.)
| } | ||
|
|
||
| static void | ||
| decrement_daemon_count(PyInterpreterState *interp) |
There was a problem hiding this comment.
minor, but this decrements the count of non-daemon threads, right? It's not a count of daemon threads.
Reference implementation for PEP-788.