-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
Changes from 17 commits
1828b9a
d0895f9
5c3ee8d
7b9ac59
0868c15
e9ea644
0ebdca4
40989de
d501f35
c5ec89c
4e1f599
3127a3f
82b5b9f
fda9886
f7723c0
62e9549
bc60630
9d8d526
54b0ce0
5955de6
92cf906
b0d0673
0a15beb
16d79de
c2bffcd
911c6b5
b84fa90
9ccf6bd
481caf5
71e1aec
d661578
4249c5d
71f2fd7
03ccb38
bca65fb
510ade1
3971408
6c4c52b
64920a8
05436f3
02bc2d7
03fa2af
b955d85
5236700
a277130
dac0c1a
ab9e3b5
79a1852
47957b8
771d7ed
0c3c1c7
531928e
08a8af6
02f93bc
d6c82bd
082fd69
61f70ae
fa961e9
ea1da77
6f19384
b702da2
40e7e68
96efc81
af6f6fd
2c52cdc
3ffe9d0
588364c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2546,6 +2546,81 @@ toggle_reftrace_printer(PyObject *ob, PyObject *arg) | |
Py_RETURN_NONE; | ||
} | ||
|
||
static PyObject * | ||
test_interp_refcount(PyObject *self, PyObject *unused) | ||
{ | ||
PyThreadState *save = PyThreadState_Get(); | ||
PyThreadState *tstate = Py_NewInterpreter(); | ||
assert(tstate == PyThreadState_Get()); | ||
PyInterpreterState *interp = PyThreadState_GetInterpreter(tstate); | ||
assert(interp != NULL); | ||
|
||
assert(_PyInterpreterState_Refcount(interp) == 0); | ||
PyInterpreterState *held = PyInterpreterState_Hold(); | ||
assert(_PyInterpreterState_Refcount(interp) == 1); | ||
PyInterpreterState_Release(held); | ||
assert(_PyInterpreterState_Refcount(interp) == 0); | ||
|
||
held = PyInterpreterState_Hold(); | ||
Py_EndInterpreter(tstate); | ||
PyThreadState_Swap(save); | ||
assert(_PyInterpreterState_Refcount(interp) == 1); | ||
PyInterpreterState_Release(held); | ||
assert(_PyInterpreterState_Refcount(interp) == 0); | ||
|
||
Py_RETURN_NONE; | ||
} | ||
|
||
static PyObject * | ||
test_interp_lookup(PyObject *self, PyObject *unused) | ||
{ | ||
PyInterpreterState *interp = PyInterpreterState_Get(); | ||
assert(_PyInterpreterState_Refcount(interp) == 0); | ||
int64_t interp_id = PyInterpreterState_GetID(interp); | ||
PyInterpreterState *ref = PyInterpreterState_Lookup(interp_id); | ||
assert(ref == interp); | ||
assert(_PyInterpreterState_Refcount(interp) == 1); | ||
PyInterpreterState_Release(ref); | ||
assert(PyInterpreterState_Lookup(10000) == NULL); | ||
Py_BEGIN_ALLOW_THREADS; | ||
ref = PyInterpreterState_Lookup(interp_id); | ||
assert(ref == interp); | ||
PyInterpreterState_Release(ref); | ||
Py_END_ALLOW_THREADS; | ||
|
||
Py_RETURN_NONE; | ||
} | ||
|
||
static PyObject * | ||
test_interp_ensure(PyObject *self, PyObject *unused) | ||
{ | ||
PyInterpreterState *interp = PyInterpreterState_Get(); | ||
PyThreadState *save_tstate = PyThreadState_Swap(NULL); | ||
PyThreadState *tstate = Py_NewInterpreter(); | ||
PyInterpreterState *subinterp = PyThreadState_GetInterpreter(tstate); | ||
|
||
for (int i = 0; i < 10; ++i) { | ||
_PyInterpreterState_Incref(interp); | ||
int res = PyThreadState_Ensure(interp); | ||
assert(res == 0); | ||
assert(PyInterpreterState_Get() == interp); | ||
vstinner marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
for (int i = 0; i < 10; ++i) { | ||
_PyInterpreterState_Incref(subinterp); | ||
int res = PyThreadState_Ensure(subinterp); | ||
assert(res == 0); | ||
assert(PyInterpreterState_Get() == subinterp); | ||
} | ||
|
||
for (int i = 0; i < 20; ++i) { | ||
PyThreadState_Release(); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose that PyThreadState_GetUnchecked() should be NULL at this point? Maybe add a check for that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I redid this test. |
||
|
||
PyThreadState_Swap(save_tstate); | ||
Py_RETURN_NONE; | ||
} | ||
|
||
static PyMethodDef TestMethods[] = { | ||
{"set_errno", set_errno, METH_VARARGS}, | ||
{"test_config", test_config, METH_NOARGS}, | ||
|
@@ -2640,6 +2715,9 @@ static PyMethodDef TestMethods[] = { | |
{"test_atexit", test_atexit, METH_NOARGS}, | ||
{"code_offset_to_line", _PyCFunction_CAST(code_offset_to_line), METH_FASTCALL}, | ||
{"toggle_reftrace_printer", toggle_reftrace_printer, METH_O}, | ||
{"test_interp_refcount", test_interp_refcount, METH_NOARGS}, | ||
{"test_interp_lookup", test_interp_lookup, METH_NOARGS}, | ||
{"test_interp_ensure", test_interp_ensure, METH_NOARGS}, | ||
{NULL, NULL} /* sentinel */ | ||
}; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2341,6 +2341,56 @@ test_get_incomplete_frame(void) | |
return result; | ||
} | ||
|
||
const char *THREAD_CODE = "import time\n" | ||
ZeroIntensity marked this conversation as resolved.
Show resolved
Hide resolved
|
||
"time.sleep(0.2)\n" | ||
"def fib(n):\n" | ||
" if n <= 1:\n" | ||
" return n\n" | ||
" else:\n" | ||
" return fib(n - 1) + fib(n - 2)\n" | ||
"fib(10)"; | ||
|
||
typedef struct { | ||
PyInterpreterState *interp; | ||
int done; | ||
} ThreadData; | ||
|
||
static void | ||
do_tstate_ensure(void *arg) | ||
{ | ||
ThreadData *data = (ThreadData *)arg; | ||
int res = PyThreadState_Ensure(data->interp); | ||
assert(res == 0); | ||
PyThreadState_Ensure(PyInterpreterState_Hold()); | ||
PyThreadState_Ensure(PyInterpreterState_Hold()); | ||
PyGILState_STATE gstate = PyGILState_Ensure(); | ||
PyThreadState_Ensure(PyInterpreterState_Hold()); | ||
res = PyRun_SimpleString(THREAD_CODE); | ||
PyThreadState_Release(); | ||
PyGILState_Release(gstate); | ||
PyThreadState_Release(); | ||
PyThreadState_Release(); | ||
assert(res == 0); | ||
PyThreadState_Release(); | ||
data->done = 1; | ||
} | ||
|
||
static int | ||
test_thread_state_ensure(void) | ||
{ | ||
_testembed_Py_InitializeFromConfig(); | ||
PyThread_handle_t handle; | ||
PyThread_ident_t ident; | ||
ThreadData data = { PyInterpreterState_Hold() }; | ||
if (PyThread_start_joinable_thread(do_tstate_ensure, &data, | ||
&ident, &handle) < 0) { | ||
PyInterpreterState_Release(data.interp); | ||
return -1; | ||
} | ||
Py_Finalize(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It may be safe to add a PyEvent to wait until the thread started before calling Py_Finalize(). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We hold a strong interpreter reference, we're implicitly waiting for it to start anyway. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh you're right, but I always forget the subtle semantics of interpreter strong references. Can you please add a comment explaining that the interpreter reference makes sure that Py_Finalize() doesn't complete until PyInterpreterRef_Close() is called? Or something like that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
assert(data.done == 1); | ||
return 0; | ||
} | ||
|
||
/* ********************************************************* | ||
* List of test cases and the function that implements it. | ||
|
@@ -2431,6 +2481,7 @@ static struct TestCase TestCases[] = { | |
{"test_frozenmain", test_frozenmain}, | ||
#endif | ||
{"test_get_incomplete_frame", test_get_incomplete_frame}, | ||
{"test_thread_state_ensure", test_thread_state_ensure}, | ||
|
||
{NULL, NULL} | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ | |
#include "pycore_freelist.h" // _PyObject_ClearFreeLists() | ||
#include "pycore_global_objects_fini_generated.h" // _PyStaticObjects_CheckRefcnt() | ||
#include "pycore_initconfig.h" // _PyStatus_OK() | ||
#include "pycore_interp_structs.h" | ||
#include "pycore_long.h" // _PyLong_InitTypes() | ||
#include "pycore_object.h" // _PyDebug_PrintTotalRefs() | ||
#include "pycore_obmalloc.h" // _PyMem_init_obmalloc() | ||
|
@@ -96,6 +97,7 @@ static PyStatus init_android_streams(PyThreadState *tstate); | |
static PyStatus init_apple_streams(PyThreadState *tstate); | ||
#endif | ||
static void wait_for_thread_shutdown(PyThreadState *tstate); | ||
static void wait_for_native_shutdown(PyInterpreterState *interp); | ||
static void finalize_subinterpreters(void); | ||
static void call_ll_exitfuncs(_PyRuntimeState *runtime); | ||
|
||
|
@@ -2012,6 +2014,9 @@ _Py_Finalize(_PyRuntimeState *runtime) | |
// Wrap up existing "threading"-module-created, non-daemon threads. | ||
wait_for_thread_shutdown(tstate); | ||
|
||
// Wrap up non-daemon native threads | ||
wait_for_native_shutdown(tstate->interp); | ||
|
||
// Make any remaining pending calls. | ||
_Py_FinishPendingCalls(tstate); | ||
|
||
|
@@ -2428,6 +2433,9 @@ Py_EndInterpreter(PyThreadState *tstate) | |
// Wrap up existing "threading"-module-created, non-daemon threads. | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. I think this needs to be unified with As currently written, I think you can get to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 My main concern is breakage towards people who are manually using There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI, this is now tracked at gh-136003. |
||
|
||
// Make any remaining pending calls. | ||
_Py_FinishPendingCalls(tstate); | ||
|
||
|
@@ -3454,6 +3462,25 @@ wait_for_thread_shutdown(PyThreadState *tstate) | |
Py_DECREF(threading); | ||
} | ||
|
||
/* Wait for all non-daemon native threads to finish. | ||
See PEP 788. */ | ||
static void | ||
wait_for_native_shutdown(PyInterpreterState *interp) | ||
{ | ||
assert(interp != NULL); | ||
struct _Py_finalizing_threads *finalizing = &interp->threads.finalizing; | ||
_Py_atomic_store_int_release(&finalizing->shutting_down, 1); | ||
PyMutex_Lock(&finalizing->mutex); | ||
if (_Py_atomic_load_ssize_relaxed(&finalizing->countdown) == 0) { | ||
// Nothing to do. | ||
PyMutex_Unlock(&finalizing->mutex); | ||
return; | ||
} | ||
PyMutex_Unlock(&finalizing->mutex); | ||
|
||
PyEvent_Wait(&finalizing->finished); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||
} | ||
|
||
int Py_AtExit(void (*func)(void)) | ||
{ | ||
struct _atexit_runtime_state *state = &_PyRuntime.atexit; | ||
|
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.
Can you move it to the internal C API? (Include/internal/pycore_pystate.h).
Same remark for _PyInterpreterState_Incref().
Uh oh!
There was an error while loading. Please reload this page.
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.
_testcapi
doesn't allow importing from pycore headers. I only need these for those tests.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.
Move tests to _testinternalcapi in this case.
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.
But we're not testing the internal C API, we're testing the public C API. We just use the internal C API to write those tests.
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.
Please don't add private functions (ex:
_PyInterpreterState_Refcount()
) to the public C API. So move it to the internal C API. Sadly, it means moving your tests to the internal C API tests, unless a test only uses public functions.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 split up the tests, so now the internal stuff is in
_testinternalcapi
and the internal functions are in the private headers.