From beff684af320e044c68e3a0fde9edb3b9ab27aa1 Mon Sep 17 00:00:00 2001 From: rdb Date: Thu, 19 Sep 2024 20:43:06 +0200 Subject: [PATCH] Further thread safety changes for Python 3.13 free threading See #1683 --- direct/src/dcparser/dcClass_ext.cxx | 58 +++++++++------- direct/src/dcparser/dcPacker_ext.cxx | 2 + dtool/Config.cmake | 2 +- dtool/src/interrogatedb/dtool_super_base.cxx | 16 +++++ dtool/src/interrogatedb/py_panda.cxx | 73 +++++++++++++++----- dtool/src/interrogatedb/py_panda.h | 7 ++ makepanda/makepandacore.py | 4 +- panda/src/collide/collisionPolygon_ext.cxx | 10 ++- panda/src/express/pointerToArray_ext.I | 6 +- panda/src/gobj/textureCollection_ext.cxx | 8 +-- panda/src/pgraph/nodePathCollection_ext.cxx | 8 +-- panda/src/pgraph/pythonLoaderFileType.cxx | 13 ++-- panda/src/pgraph/shaderInput_ext.cxx | 13 +++- panda/src/putil/typedWritable_ext.cxx | 29 +++++--- 14 files changed, 171 insertions(+), 78 deletions(-) diff --git a/direct/src/dcparser/dcClass_ext.cxx b/direct/src/dcparser/dcClass_ext.cxx index cf09a97d34..65f55bf33b 100644 --- a/direct/src/dcparser/dcClass_ext.cxx +++ b/direct/src/dcparser/dcClass_ext.cxx @@ -522,31 +522,35 @@ client_format_generate_CMU(PyObject *distobj, DOID_TYPE do_id, } // Also specify the optional fields. - int num_optional_fields = 0; if (PyObject_IsTrue(optional_fields)) { - num_optional_fields = PySequence_Size(optional_fields); - } - packer.raw_pack_uint16(num_optional_fields); + optional_fields = PySequence_Tuple(optional_fields); + Py_ssize_t num_optional_fields = PyTuple_GET_SIZE(optional_fields); + packer.raw_pack_uint16(num_optional_fields); - for (int i = 0; i < num_optional_fields; i++) { - PyObject *py_field_name = PySequence_GetItem(optional_fields, i); - std::string field_name = PyUnicode_AsUTF8(py_field_name); - Py_XDECREF(py_field_name); + for (Py_ssize_t i = 0; i < num_optional_fields; i++) { + PyObject *py_field_name = PyTuple_GET_ITEM(optional_fields, i); + std::string field_name = PyUnicode_AsUTF8(py_field_name); - DCField *field = _this->get_field_by_name(field_name); - if (field == nullptr) { - std::ostringstream strm; - strm << "No field named " << field_name << " in class " << _this->get_name() - << "\n"; - nassert_raise(strm.str()); - return Datagram(); + DCField *field = _this->get_field_by_name(field_name); + if (field == nullptr) { + std::ostringstream strm; + strm << "No field named " << field_name << " in class " << _this->get_name() + << "\n"; + nassert_raise(strm.str()); + Py_DECREF(optional_fields); + return Datagram(); + } + packer.raw_pack_uint16(field->get_number()); + packer.begin_pack(field); + if (!pack_required_field(packer, distobj, field)) { + Py_DECREF(optional_fields); + return Datagram(); + } + packer.end_pack(); } - packer.raw_pack_uint16(field->get_number()); - packer.begin_pack(field); - if (!pack_required_field(packer, distobj, field)) { - return Datagram(); - } - packer.end_pack(); + Py_DECREF(optional_fields); + } else { + packer.raw_pack_uint16(0); } return Datagram(packer.get_data(), packer.get_length()); @@ -602,13 +606,13 @@ ai_format_generate(PyObject *distobj, DOID_TYPE do_id, // Also specify the optional fields. if (has_optional_fields) { - int num_optional_fields = PySequence_Size(optional_fields); + optional_fields = PySequence_Tuple(optional_fields); + Py_ssize_t num_optional_fields = PyTuple_GET_SIZE(optional_fields); packer.raw_pack_uint16(num_optional_fields); - for (int i = 0; i < num_optional_fields; ++i) { - PyObject *py_field_name = PySequence_GetItem(optional_fields, i); + for (Py_ssize_t i = 0; i < num_optional_fields; ++i) { + PyObject *py_field_name = PyTuple_GET_ITEM(optional_fields, i); std::string field_name = PyUnicode_AsUTF8(py_field_name); - Py_XDECREF(py_field_name); DCField *field = _this->get_field_by_name(field_name); if (field == nullptr) { @@ -616,6 +620,7 @@ ai_format_generate(PyObject *distobj, DOID_TYPE do_id, strm << "No field named " << field_name << " in class " << _this->get_name() << "\n"; nassert_raise(strm.str()); + Py_DECREF(optional_fields); return Datagram(); } @@ -623,10 +628,13 @@ ai_format_generate(PyObject *distobj, DOID_TYPE do_id, packer.begin_pack(field); if (!pack_required_field(packer, distobj, field)) { + Py_DECREF(optional_fields); return Datagram(); } packer.end_pack(); } + + Py_DECREF(optional_fields); } return Datagram(packer.get_data(), packer.get_length()); diff --git a/direct/src/dcparser/dcPacker_ext.cxx b/direct/src/dcparser/dcPacker_ext.cxx index 276a2dd7f9..9ffca60ba9 100644 --- a/direct/src/dcparser/dcPacker_ext.cxx +++ b/direct/src/dcparser/dcPacker_ext.cxx @@ -136,6 +136,7 @@ pack_object(PyObject *object) { // The supplied object is not an instance of the expected class object, // but it is a sequence. This is case (2). _this->push(); + Py_BEGIN_CRITICAL_SECTION(object); int size = PySequence_Size(object); for (int i = 0; i < size; ++i) { PyObject *element = PySequence_GetItem(object, i); @@ -146,6 +147,7 @@ pack_object(PyObject *object) { std::cerr << "Unable to extract item " << i << " from sequence.\n"; } } + Py_END_CRITICAL_SECTION(); _this->pop(); } else { // The supplied object is not a sequence, and we weren't expecting a diff --git a/dtool/Config.cmake b/dtool/Config.cmake index e9b1badd6d..56410de0f5 100644 --- a/dtool/Config.cmake +++ b/dtool/Config.cmake @@ -265,7 +265,7 @@ if(BUILD_INTERROGATE) panda3d-interrogate GIT_REPOSITORY https://github.com/panda3d/interrogate.git - GIT_TAG 742ea56a1934a7a05ee2518787cf6816f30ee096 + GIT_TAG c343350a6e210029cfe3fd8468e530e1ea6bcead PREFIX ${_interrogate_dir} CMAKE_ARGS diff --git a/dtool/src/interrogatedb/dtool_super_base.cxx b/dtool/src/interrogatedb/dtool_super_base.cxx index 0b7613617e..c42b39c823 100644 --- a/dtool/src/interrogatedb/dtool_super_base.cxx +++ b/dtool/src/interrogatedb/dtool_super_base.cxx @@ -61,8 +61,18 @@ static void Dtool_FreeInstance_DTOOL_SUPER_BASE(PyObject *self) { */ Dtool_PyTypedObject *Dtool_GetSuperBase() { Dtool_TypeMap *type_map = Dtool_GetGlobalTypeMap(); + + // If we don't have the GIL, we have to protect this with a lock to make + // sure that there is only one DTOOL_SUPER_BASE instance in the world. +#ifdef Py_GIL_DISABLED + PyMutex_Lock(&type_map->_lock); +#endif + auto it = type_map->find("DTOOL_SUPER_BASE"); if (it != type_map->end()) { +#ifdef Py_GIL_DISABLED + PyMutex_Unlock(&type_map->_lock); +#endif return it->second; } @@ -148,6 +158,9 @@ Dtool_PyTypedObject *Dtool_GetSuperBase() { if (PyType_Ready((PyTypeObject *)&super_base_type) < 0) { PyErr_SetString(PyExc_TypeError, "PyType_Ready(Dtool_DTOOL_SUPER_BASE)"); +#ifdef Py_GIL_DISABLED + PyMutex_Unlock(&type_map->_lock); +#endif return nullptr; } Py_INCREF(&super_base_type._PyType); @@ -155,6 +168,9 @@ Dtool_PyTypedObject *Dtool_GetSuperBase() { PyDict_SetItemString(super_base_type._PyType.tp_dict, "DtoolGetSuperBase", PyCFunction_New(&methods[0], (PyObject *)&super_base_type)); (*type_map)["DTOOL_SUPER_BASE"] = &super_base_type; +#ifdef Py_GIL_DISABLED + PyMutex_Unlock(&type_map->_lock); +#endif return &super_base_type; } diff --git a/dtool/src/interrogatedb/py_panda.cxx b/dtool/src/interrogatedb/py_panda.cxx index f8b2881039..8e3614b713 100644 --- a/dtool/src/interrogatedb/py_panda.cxx +++ b/dtool/src/interrogatedb/py_panda.cxx @@ -375,23 +375,26 @@ static PyObject *Dtool_EnumType_Repr(PyObject *self) { * should be a tuple of (name, value) pairs. */ PyTypeObject *Dtool_EnumType_Create(const char *name, PyObject *names, const char *module) { - static PyObject *enum_class = nullptr; #if PY_VERSION_HEX >= 0x03040000 - static PyObject *enum_meta = nullptr; - static PyObject *enum_create = nullptr; - if (enum_meta == nullptr) { - PyObject *enum_module = PyImport_ImportModule("enum"); - nassertr_always(enum_module != nullptr, nullptr); + PyObject *enum_module = PyImport_ImportModule("enum"); + nassertr_always(enum_module != nullptr, nullptr); - enum_class = PyObject_GetAttrString(enum_module, "Enum"); - enum_meta = PyObject_GetAttrString(enum_module, "EnumMeta"); - enum_create = PyObject_GetAttrString(enum_meta, "_create_"); - nassertr(enum_meta != nullptr, nullptr); - } + PyObject *enum_meta = PyObject_GetAttrString(enum_module, "EnumMeta"); + nassertr(enum_meta != nullptr, nullptr); + + PyObject *enum_class = PyObject_GetAttrString(enum_module, "Enum"); + Py_DECREF(enum_module); + nassertr(enum_class != nullptr, nullptr); + + PyObject *enum_create = PyObject_GetAttrString(enum_meta, "_create_"); + Py_DECREF(enum_meta); PyObject *result = PyObject_CallFunction(enum_create, (char *)"OsN", enum_class, name, names); + Py_DECREF(enum_create); + Py_DECREF(enum_class); nassertr(result != nullptr, nullptr); #else + static PyObject *enum_class = nullptr; static PyObject *name_str; static PyObject *name_sunder_str; static PyObject *value_str; @@ -528,16 +531,39 @@ PyObject *DTool_CreatePyInstance(void *local_this, Dtool_PyTypedObject &in_class * Returns a borrowed reference to the global type dictionary. */ Dtool_TypeMap *Dtool_GetGlobalTypeMap() { +#if PY_VERSION_HEX >= 0x030d0000 // 3.13 + PyObject *istate_dict = PyInterpreterState_GetDict(PyInterpreterState_Get()); + PyObject *key = PyUnicode_InternFromString("_interrogate_types"); + PyObject *capsule = PyDict_GetItem(istate_dict, key); + if (capsule != nullptr) { + Py_DECREF(key); + return (Dtool_TypeMap *)PyCapsule_GetPointer(capsule, nullptr); + } +#else PyObject *capsule = PySys_GetObject((char *)"_interrogate_types"); if (capsule != nullptr) { return (Dtool_TypeMap *)PyCapsule_GetPointer(capsule, nullptr); - } else { - Dtool_TypeMap *type_map = new Dtool_TypeMap; - capsule = PyCapsule_New((void *)type_map, nullptr, nullptr); - PySys_SetObject((char *)"_interrogate_types", capsule); - Py_DECREF(capsule); - return type_map; } +#endif + + Dtool_TypeMap *type_map = new Dtool_TypeMap; + capsule = PyCapsule_New((void *)type_map, nullptr, nullptr); + +#if PY_VERSION_HEX >= 0x030d0000 // 3.13 + PyObject *result; + if (PyDict_SetDefaultRef(istate_dict, key, capsule, &result) != 0) { + // Another thread already beat us to it. + Py_DECREF(capsule); + delete type_map; + capsule = result; + type_map = (Dtool_TypeMap *)PyCapsule_GetPointer(capsule, nullptr); + } + Py_DECREF(key); +#endif + + PySys_SetObject((char *)"_interrogate_types", capsule); + Py_DECREF(capsule); + return type_map; } /** @@ -594,6 +620,10 @@ PyObject *Dtool_PyModuleInitHelper(const LibraryDef *defs[], const char *modulen Dtool_TypeMap *type_map = Dtool_GetGlobalTypeMap(); +#ifdef Py_GIL_DISABLED + PyMutex_Lock(&type_map->_lock); +#endif + // the module level function inits.... MethodDefmap functions; for (size_t i = 0; defs[i] != nullptr; i++) { @@ -627,12 +657,19 @@ PyObject *Dtool_PyModuleInitHelper(const LibraryDef *defs[], const char *modulen if (it != type_map->end()) { types->type = it->second; } else { - return PyErr_Format(PyExc_NameError, "name '%s' is not defined", types->name); + PyErr_Format(PyExc_NameError, "name '%s' is not defined", types->name); +#ifdef Py_GIL_DISABLED + PyMutex_Unlock(&type_map->_lock); +#endif + return nullptr; } ++types; } } } +#ifdef Py_GIL_DISABLED + PyMutex_Unlock(&type_map->_lock); +#endif PyMethodDef *newdef = new PyMethodDef[functions.size() + 1]; MethodDefmap::iterator mi; diff --git a/dtool/src/interrogatedb/py_panda.h b/dtool/src/interrogatedb/py_panda.h index ca79501601..6300009d9c 100644 --- a/dtool/src/interrogatedb/py_panda.h +++ b/dtool/src/interrogatedb/py_panda.h @@ -181,7 +181,14 @@ static void Dtool_FreeInstance_##CLASS_NAME(PyObject *self) {\ // forward declared of typed object. We rely on the fact that typed objects // are uniquly defined by an integer. +#if PY_VERSION_HEX >= 0x030d0000 +class Dtool_TypeMap : public std::map { +public: + PyMutex _lock { 0 }; +}; +#else typedef std::map Dtool_TypeMap; +#endif EXPCL_PYPANDA Dtool_TypeMap *Dtool_GetGlobalTypeMap(); diff --git a/makepanda/makepandacore.py b/makepanda/makepandacore.py index 4729be69b7..6c7b5c5016 100644 --- a/makepanda/makepandacore.py +++ b/makepanda/makepandacore.py @@ -592,8 +592,8 @@ def GetInterrogateDir(): return INTERROGATE_DIR dir = os.path.join(GetOutputDir(), "tmp", "interrogate") - if not os.path.isdir(os.path.join(dir, "panda3d_interrogate-0.1.1.dist-info")): - oscmd("\"%s\" -m pip install --force-reinstall -t \"%s\" panda3d-interrogate==0.1.1" % (sys.executable, dir)) + if not os.path.isdir(os.path.join(dir, "panda3d_interrogate-0.2.0.dist-info")): + oscmd("\"%s\" -m pip install --force-reinstall -t \"%s\" panda3d-interrogate==0.2.0" % (sys.executable, dir)) INTERROGATE_DIR = dir diff --git a/panda/src/collide/collisionPolygon_ext.cxx b/panda/src/collide/collisionPolygon_ext.cxx index 22ada8c344..3f7d6503aa 100644 --- a/panda/src/collide/collisionPolygon_ext.cxx +++ b/panda/src/collide/collisionPolygon_ext.cxx @@ -72,6 +72,9 @@ convert_points(pvector &vec, PyObject *points) { return false; } + bool success = true; + + Py_BEGIN_CRITICAL_SECTION(seq); PyObject **items = PySequence_Fast_ITEMS(seq); Py_ssize_t len = PySequence_Fast_GET_SIZE(seq); void *ptr; @@ -90,13 +93,14 @@ convert_points(pvector &vec, PyObject *points) { } else { Dtool_Raise_TypeError("Argument must be of LPoint3 type."); - Py_DECREF(seq); - return false; + success = false; + break; } } + Py_END_CRITICAL_SECTION(); Py_DECREF(seq); - return true; + return success; } #endif diff --git a/panda/src/express/pointerToArray_ext.I b/panda/src/express/pointerToArray_ext.I index 3406a67263..0fb4230d1d 100644 --- a/panda/src/express/pointerToArray_ext.I +++ b/panda/src/express/pointerToArray_ext.I @@ -106,12 +106,13 @@ __init__(PyObject *self, PyObject *source) { // We need to initialize the this pointer before we can call push_back. DtoolInstance_INIT_PTR(self, this->_this); + Py_BEGIN_CRITICAL_SECTION(source); Py_ssize_t size = PySequence_Size(source); this->_this->reserve(size); for (Py_ssize_t i = 0; i < size; ++i) { PyObject *item = PySequence_GetItem(source, i); if (item == nullptr) { - return; + break; } PyObject *result = PyObject_CallFunctionObjArgs(push_back, self, item, nullptr); Py_DECREF(item); @@ -121,10 +122,11 @@ __init__(PyObject *self, PyObject *source) { PyErr_Format(PyExc_TypeError, "Element %zd in sequence passed to PointerToArray " "constructor could not be added", i); - return; + break; } Py_DECREF(result); } + Py_END_CRITICAL_SECTION(); } /** diff --git a/panda/src/gobj/textureCollection_ext.cxx b/panda/src/gobj/textureCollection_ext.cxx index 127bba9ac5..46e9920280 100644 --- a/panda/src/gobj/textureCollection_ext.cxx +++ b/panda/src/gobj/textureCollection_ext.cxx @@ -31,13 +31,14 @@ __init__(PyObject *self, PyObject *sequence) { return; } + Py_BEGIN_CRITICAL_SECTION(fast); Py_ssize_t size = PySequence_Fast_GET_SIZE(fast); _this->reserve(size); for (int i = 0; i < size; ++i) { PyObject *item = PySequence_Fast_GET_ITEM(fast, i); if (item == nullptr) { - return; + break; } Texture *tex; @@ -48,13 +49,12 @@ __init__(PyObject *self, PyObject *sequence) { stream << "Element " << i << " in sequence passed to TextureCollection constructor is not a Texture"; std::string str = stream.str(); PyErr_SetString(PyExc_TypeError, str.c_str()); - Py_DECREF(fast); - return; + break; } else { _this->add_texture(tex); } } - + Py_END_CRITICAL_SECTION(); Py_DECREF(fast); } diff --git a/panda/src/pgraph/nodePathCollection_ext.cxx b/panda/src/pgraph/nodePathCollection_ext.cxx index a06caf8380..33cedfb5c3 100644 --- a/panda/src/pgraph/nodePathCollection_ext.cxx +++ b/panda/src/pgraph/nodePathCollection_ext.cxx @@ -36,13 +36,14 @@ __init__(PyObject *self, PyObject *sequence) { return; } + Py_BEGIN_CRITICAL_SECTION(fast); Py_ssize_t size = PySequence_Fast_GET_SIZE(fast); _this->reserve(size); for (int i = 0; i < size; ++i) { PyObject *item = PySequence_Fast_GET_ITEM(fast, i); if (item == nullptr) { - return; + break; } NodePath *path; @@ -52,13 +53,12 @@ __init__(PyObject *self, PyObject *sequence) { stream << "Element " << i << " in sequence passed to NodePathCollection constructor is not a NodePath"; std::string str = stream.str(); PyErr_SetString(PyExc_TypeError, str.c_str()); - Py_DECREF(fast); - return; + break; } else { _this->add_path(*path); } } - + Py_END_CRITICAL_SECTION(); Py_DECREF(fast); } diff --git a/panda/src/pgraph/pythonLoaderFileType.cxx b/panda/src/pgraph/pythonLoaderFileType.cxx index 9076a3535e..aa4b84cceb 100644 --- a/panda/src/pgraph/pythonLoaderFileType.cxx +++ b/panda/src/pgraph/pythonLoaderFileType.cxx @@ -89,26 +89,25 @@ init(PyObject *loader) { return false; } - PyObject *sequence = PySequence_Fast(extensions, "extensions must be a sequence"); - PyObject **items = PySequence_Fast_ITEMS(sequence); - Py_ssize_t num_items = PySequence_Fast_GET_SIZE(sequence); + PyObject *tuple = PySequence_Tuple(extensions); + Py_ssize_t num_items = PyTuple_GET_SIZE(tuple); Py_DECREF(extensions); if (num_items == 0) { PyErr_SetString(PyExc_ValueError, "extensions list may not be empty"); - Py_DECREF(sequence); + Py_DECREF(tuple); return false; } bool found_extension = false; for (Py_ssize_t i = 0; i < num_items; ++i) { - PyObject *extension = items[i]; + PyObject *extension = PyTuple_GET_ITEM(tuple, i); const char *extension_str; Py_ssize_t extension_len; extension_str = PyUnicode_AsUTF8AndSize(extension, &extension_len); if (extension_str == nullptr) { - Py_DECREF(sequence); + Py_DECREF(tuple); return false; } @@ -127,7 +126,7 @@ init(PyObject *loader) { } } } - Py_DECREF(sequence); + Py_DECREF(tuple); if (!found_extension) { PyObject *repr = PyObject_Repr(loader); diff --git a/panda/src/pgraph/shaderInput_ext.cxx b/panda/src/pgraph/shaderInput_ext.cxx index d7496120b6..e9c83c18fc 100644 --- a/panda/src/pgraph/shaderInput_ext.cxx +++ b/panda/src/pgraph/shaderInput_ext.cxx @@ -269,12 +269,20 @@ __init__(CPT_InternalName name, PyObject *value, int priority) { } else if (PySequence_Check(value) && !PyUnicode_CheckExact(value)) { // Iterate over the sequence to make sure all have the same type. +#ifdef Py_GIL_DISABLED + PyObject *fast = PySequence_Tuple(value); +#else PyObject *fast = PySequence_Fast(value, "unknown type passed to ShaderInput"); +#endif if (fast == nullptr) { return; } +#ifdef Py_GIL_DISABLED + Py_ssize_t num_items = PyTuple_GET_SIZE(fast); +#else Py_ssize_t num_items = PySequence_Fast_GET_SIZE(fast); +#endif if (num_items <= 0) { // We can't determine the type of a list of size 0. _this->_type = ShaderInput::M_numeric; @@ -289,6 +297,9 @@ __init__(CPT_InternalName name, PyObject *value, int priority) { for (Py_ssize_t i = 0; i < num_items; ++i) { PyObject *item = items[i]; + //FIXME: if these items are not tuples, this is not thread-safe in the + // free-threaded build. Convert everything to tuples, and push to a + // vector? if (PySequence_Check(item)) { Py_ssize_t itemsize = PySequence_Size(item); if (known_itemsize >= 0 && itemsize != known_itemsize) { @@ -310,7 +321,7 @@ __init__(CPT_InternalName name, PyObject *value, int priority) { Dtool_Raise_TypeError("unknown element type in sequence passed as element of sequence passed to ShaderInput"); Py_DECREF(subitem); Py_DECREF(fast); - break; + return; } Py_DECREF(subitem); } diff --git a/panda/src/putil/typedWritable_ext.cxx b/panda/src/putil/typedWritable_ext.cxx index f64ec302a8..0fc665e0e6 100644 --- a/panda/src/putil/typedWritable_ext.cxx +++ b/panda/src/putil/typedWritable_ext.cxx @@ -349,17 +349,24 @@ find_global_decode(PyObject *this_class, const char *func_name) { PyObject *bases = PyObject_GetAttrString(this_class, "__bases__"); if (bases != nullptr) { - if (PySequence_Check(bases)) { - Py_ssize_t size = PySequence_Size(bases); - for (Py_ssize_t i = 0; i < size; ++i) { - PyObject *base = PySequence_GetItem(bases, i); - if (base != nullptr) { - PyObject *func = find_global_decode(base, func_name); - Py_DECREF(base); - if (func != nullptr) { - Py_DECREF(bases); - return func; - } + { + PyObject *tuple = PySequence_Tuple(bases); + Py_DECREF(bases); + if (tuple == nullptr) { + PyErr_Clear(); + return nullptr; + } + bases = tuple; + } + + Py_ssize_t size = PyTuple_GET_SIZE(bases); + for (Py_ssize_t i = 0; i < size; ++i) { + PyObject *base = PyTuple_GET_ITEM(bases, i); + if (base != nullptr) { + PyObject *func = find_global_decode(base, func_name); + if (func != nullptr) { + Py_DECREF(bases); + return func; } } }