From ff5673f95c45db57cae3e39ba9f1d1f3ab4f6780 Mon Sep 17 00:00:00 2001 From: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com> Date: Mon, 27 Jun 2022 15:10:35 +0000 Subject: [PATCH 1/8] port multiprocssing types to heap types --- Modules/_multiprocessing/multiprocessing.c | 85 ++++++++++++++++------ Modules/_multiprocessing/multiprocessing.h | 2 +- Modules/_multiprocessing/semaphore.c | 75 ++++++++----------- 3 files changed, 96 insertions(+), 66 deletions(-) diff --git a/Modules/_multiprocessing/multiprocessing.c b/Modules/_multiprocessing/multiprocessing.c index 0809c2455dbebc..ebabc01c99902a 100644 --- a/Modules/_multiprocessing/multiprocessing.c +++ b/Modules/_multiprocessing/multiprocessing.c @@ -24,6 +24,18 @@ module _multiprocessing #include "clinic/multiprocessing.c.h" +typedef struct { + PyTypeObject *semlock_type; +} _multiprocessingstate; + +static inline +_multiprocessingstate *get_module_state(PyObject *module) +{ + void *state = PyModule_GetState(module); + assert(state != NULL); + return (_multiprocessingstate *)state; +} + /* * Function which raises exceptions based on error codes */ @@ -186,35 +198,38 @@ static PyMethodDef module_methods[] = { static int multiprocessing_exec(PyObject *module) { + _multiprocessingstate *state = get_module_state(module); #ifdef HAVE_MP_SEMAPHORE - /* Add _PyMp_SemLock type to module */ - if (PyModule_AddType(module, &_PyMp_SemLockType) < 0) { + state->semlock_type = (PyTypeObject *)PyType_FromModuleAndSpec( + module, &_PyMp_SemLockType_spec, NULL); + + if (state->semlock_type == NULL) { + return -1; + } + if (PyModule_AddType(module, state->semlock_type) < 0) { return -1; } - { - PyObject *py_sem_value_max; - /* Some systems define SEM_VALUE_MAX as an unsigned value that - * causes it to be negative when used as an int (NetBSD). - * - * Issue #28152: Use (0) instead of 0 to fix a warning on dead code - * when using clang -Wunreachable-code. */ - if ((int)(SEM_VALUE_MAX) < (0)) - py_sem_value_max = PyLong_FromLong(INT_MAX); - else - py_sem_value_max = PyLong_FromLong(SEM_VALUE_MAX); - - if (py_sem_value_max == NULL) { - return -1; - } - if (PyDict_SetItemString(_PyMp_SemLockType.tp_dict, "SEM_VALUE_MAX", - py_sem_value_max) < 0) { - Py_DECREF(py_sem_value_max); - return -1; - } + PyObject *py_sem_value_max; + /* Some systems define SEM_VALUE_MAX as an unsigned value that + * causes it to be negative when used as an int (NetBSD). + * + * Issue #28152: Use (0) instead of 0 to fix a warning on dead code + * when using clang -Wunreachable-code. */ + if ((int)(SEM_VALUE_MAX) < (0)) + py_sem_value_max = PyLong_FromLong(INT_MAX); + else + py_sem_value_max = PyLong_FromLong(SEM_VALUE_MAX); + if (py_sem_value_max == NULL) { + return -1; + } + if (PyDict_SetItemString(state->semlock_type->tp_dict, "SEM_VALUE_MAX", + py_sem_value_max) < 0) { Py_DECREF(py_sem_value_max); + return -1; } + Py_DECREF(py_sem_value_max); #endif @@ -260,6 +275,28 @@ multiprocessing_exec(PyObject *module) return 0; } +static int +multiprocessing_traverse(PyObject *module, visitproc visit, void *arg) +{ + _multiprocessingstate *state = get_module_state(module); + Py_VISIT(state->semlock_type); + return 0; +} + +static int +multiprocessing_clear(PyObject *module) +{ + _multiprocessingstate *state = get_module_state(module); + Py_CLEAR(state->semlock_type); + return 0; +} + +static void +multiprocessing_dealloc(void *module) +{ + (void)multiprocessing_clear((PyObject *)module); +} + static PyModuleDef_Slot multiprocessing_slots[] = { {Py_mod_exec, multiprocessing_exec}, {0, NULL} @@ -268,8 +305,12 @@ static PyModuleDef_Slot multiprocessing_slots[] = { static struct PyModuleDef multiprocessing_module = { PyModuleDef_HEAD_INIT, .m_name = "_multiprocessing", + .m_size = sizeof(_multiprocessingstate), .m_methods = module_methods, .m_slots = multiprocessing_slots, + .m_traverse = multiprocessing_traverse, + .m_clear = multiprocessing_clear, + .m_free = multiprocessing_dealloc, }; PyMODINIT_FUNC diff --git a/Modules/_multiprocessing/multiprocessing.h b/Modules/_multiprocessing/multiprocessing.h index 3a8314b1db8331..b595e5a8dd18de 100644 --- a/Modules/_multiprocessing/multiprocessing.h +++ b/Modules/_multiprocessing/multiprocessing.h @@ -89,7 +89,7 @@ PyObject *_PyMp_SetError(PyObject *Type, int num); * Externs - not all will really exist on all platforms */ -extern PyTypeObject _PyMp_SemLockType; +extern PyType_Spec _PyMp_SemLockType_spec; extern PyObject *_PyMp_sem_unlink(const char *name); #endif /* MULTIPROCESSING_H */ diff --git a/Modules/_multiprocessing/semaphore.c b/Modules/_multiprocessing/semaphore.c index 8607476aff10ff..7b61a77eed6a97 100644 --- a/Modules/_multiprocessing/semaphore.c +++ b/Modules/_multiprocessing/semaphore.c @@ -454,9 +454,7 @@ static PyObject * newsemlockobject(PyTypeObject *type, SEM_HANDLE handle, int kind, int maxvalue, char *name) { - SemLockObject *self; - - self = PyObject_New(SemLockObject, type); + SemLockObject *self = PyObject_GC_New(SemLockObject, type); if (!self) return NULL; self->handle = handle; @@ -570,10 +568,13 @@ _multiprocessing_SemLock__rebuild_impl(PyTypeObject *type, SEM_HANDLE handle, static void semlock_dealloc(SemLockObject* self) { + PyTypeObject *tp = Py_TYPE(self); + PyObject_GC_UnTrack(self); if (self->handle != SEM_FAILED) SEM_CLOSE(self->handle); PyMem_Free(self->name); - PyObject_Free(self); + tp->tp_free(self); + Py_DECREF(tp); } /*[clinic input] @@ -703,6 +704,13 @@ _multiprocessing_SemLock___exit___impl(SemLockObject *self, return _multiprocessing_SemLock_release_impl(self); } +static int +semlock_traverse(SemLockObject *s, visitproc visit, void *arg) +{ + Py_VISIT(Py_TYPE(s)); + return 0; +} + /* * Semaphore methods */ @@ -741,45 +749,26 @@ static PyMemberDef semlock_members[] = { * Semaphore type */ -PyTypeObject _PyMp_SemLockType = { - PyVarObject_HEAD_INIT(NULL, 0) - /* tp_name */ "_multiprocessing.SemLock", - /* tp_basicsize */ sizeof(SemLockObject), - /* tp_itemsize */ 0, - /* tp_dealloc */ (destructor)semlock_dealloc, - /* tp_vectorcall_offset */ 0, - /* tp_getattr */ 0, - /* tp_setattr */ 0, - /* tp_as_async */ 0, - /* tp_repr */ 0, - /* tp_as_number */ 0, - /* tp_as_sequence */ 0, - /* tp_as_mapping */ 0, - /* tp_hash */ 0, - /* tp_call */ 0, - /* tp_str */ 0, - /* tp_getattro */ 0, - /* tp_setattro */ 0, - /* tp_as_buffer */ 0, - /* tp_flags */ Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE, - /* tp_doc */ "Semaphore/Mutex type", - /* tp_traverse */ 0, - /* tp_clear */ 0, - /* tp_richcompare */ 0, - /* tp_weaklistoffset */ 0, - /* tp_iter */ 0, - /* tp_iternext */ 0, - /* tp_methods */ semlock_methods, - /* tp_members */ semlock_members, - /* tp_getset */ 0, - /* tp_base */ 0, - /* tp_dict */ 0, - /* tp_descr_get */ 0, - /* tp_descr_set */ 0, - /* tp_dictoffset */ 0, - /* tp_init */ 0, - /* tp_alloc */ 0, - /* tp_new */ _multiprocessing_SemLock, +PyType_Slot _PyMp_SemLockType_slots[] = { + {Py_tp_dealloc, semlock_dealloc}, + {Py_tp_getattro, PyObject_GenericGetAttr}, + {Py_tp_setattro, PyObject_GenericSetAttr}, + {Py_tp_methods, semlock_methods}, + {Py_tp_members, semlock_members}, + {Py_tp_alloc, PyType_GenericAlloc}, + {Py_tp_new, _multiprocessing_SemLock}, + {Py_tp_traverse, semlock_traverse}, + {Py_tp_free, PyObject_GC_Del}, + {Py_tp_doc, (void *)PyDoc_STR("Semaphore/Mutex type")}, + {0, 0}, +}; + +PyType_Spec _PyMp_SemLockType_spec = { + .name = "_multiprocessing.SemLock", + .basicsize = sizeof(SemLockObject), + .flags = (Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE | + Py_TPFLAGS_HAVE_GC | Py_TPFLAGS_IMMUTABLETYPE), + .slots = _PyMp_SemLockType_slots, }; /* From 100ad999b6cc0a7bccf47fedd27aa18cbf6ee027 Mon Sep 17 00:00:00 2001 From: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com> Date: Mon, 27 Jun 2022 15:18:40 +0000 Subject: [PATCH 2/8] fix allocation --- Modules/_multiprocessing/semaphore.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_multiprocessing/semaphore.c b/Modules/_multiprocessing/semaphore.c index 7b61a77eed6a97..00a6ebe9818a58 100644 --- a/Modules/_multiprocessing/semaphore.c +++ b/Modules/_multiprocessing/semaphore.c @@ -454,7 +454,7 @@ static PyObject * newsemlockobject(PyTypeObject *type, SEM_HANDLE handle, int kind, int maxvalue, char *name) { - SemLockObject *self = PyObject_GC_New(SemLockObject, type); + SemLockObject *self = (SemLockObject *)type->tp_alloc(type, 0); if (!self) return NULL; self->handle = handle; From 08c501dc440a65eccda27f779e78b49c2755fdfe Mon Sep 17 00:00:00 2001 From: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com> Date: Tue, 28 Jun 2022 15:53:42 +0000 Subject: [PATCH 3/8] remove mod state --- Modules/_multiprocessing/multiprocessing.c | 48 +++------------------- 1 file changed, 5 insertions(+), 43 deletions(-) diff --git a/Modules/_multiprocessing/multiprocessing.c b/Modules/_multiprocessing/multiprocessing.c index ebabc01c99902a..87159d3e8b8140 100644 --- a/Modules/_multiprocessing/multiprocessing.c +++ b/Modules/_multiprocessing/multiprocessing.c @@ -24,18 +24,6 @@ module _multiprocessing #include "clinic/multiprocessing.c.h" -typedef struct { - PyTypeObject *semlock_type; -} _multiprocessingstate; - -static inline -_multiprocessingstate *get_module_state(PyObject *module) -{ - void *state = PyModule_GetState(module); - assert(state != NULL); - return (_multiprocessingstate *)state; -} - /* * Function which raises exceptions based on error codes */ @@ -198,16 +186,15 @@ static PyMethodDef module_methods[] = { static int multiprocessing_exec(PyObject *module) { - _multiprocessingstate *state = get_module_state(module); #ifdef HAVE_MP_SEMAPHORE - state->semlock_type = (PyTypeObject *)PyType_FromModuleAndSpec( + PyTypeObject *semlock_type = (PyTypeObject *)PyType_FromModuleAndSpec( module, &_PyMp_SemLockType_spec, NULL); - if (state->semlock_type == NULL) { + if (semlock_type == NULL) { return -1; } - if (PyModule_AddType(module, state->semlock_type) < 0) { + if (PyModule_AddType(module, semlock_type) < 0) { return -1; } @@ -224,7 +211,7 @@ multiprocessing_exec(PyObject *module) if (py_sem_value_max == NULL) { return -1; } - if (PyDict_SetItemString(state->semlock_type->tp_dict, "SEM_VALUE_MAX", + if (PyDict_SetItemString(semlock_type->tp_dict, "SEM_VALUE_MAX", py_sem_value_max) < 0) { Py_DECREF(py_sem_value_max); return -1; @@ -275,28 +262,6 @@ multiprocessing_exec(PyObject *module) return 0; } -static int -multiprocessing_traverse(PyObject *module, visitproc visit, void *arg) -{ - _multiprocessingstate *state = get_module_state(module); - Py_VISIT(state->semlock_type); - return 0; -} - -static int -multiprocessing_clear(PyObject *module) -{ - _multiprocessingstate *state = get_module_state(module); - Py_CLEAR(state->semlock_type); - return 0; -} - -static void -multiprocessing_dealloc(void *module) -{ - (void)multiprocessing_clear((PyObject *)module); -} - static PyModuleDef_Slot multiprocessing_slots[] = { {Py_mod_exec, multiprocessing_exec}, {0, NULL} @@ -305,12 +270,9 @@ static PyModuleDef_Slot multiprocessing_slots[] = { static struct PyModuleDef multiprocessing_module = { PyModuleDef_HEAD_INIT, .m_name = "_multiprocessing", - .m_size = sizeof(_multiprocessingstate), + .m_size = 0, .m_methods = module_methods, .m_slots = multiprocessing_slots, - .m_traverse = multiprocessing_traverse, - .m_clear = multiprocessing_clear, - .m_free = multiprocessing_dealloc, }; PyMODINIT_FUNC From 31d44d9678936d64c03b2230ceaa4544c2cc16b5 Mon Sep 17 00:00:00 2001 From: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com> Date: Tue, 28 Jun 2022 16:01:46 +0000 Subject: [PATCH 4/8] decref --- Modules/_multiprocessing/multiprocessing.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_multiprocessing/multiprocessing.c b/Modules/_multiprocessing/multiprocessing.c index 87159d3e8b8140..3f958456abddaa 100644 --- a/Modules/_multiprocessing/multiprocessing.c +++ b/Modules/_multiprocessing/multiprocessing.c @@ -197,7 +197,7 @@ multiprocessing_exec(PyObject *module) if (PyModule_AddType(module, semlock_type) < 0) { return -1; } - + Py_DECREF(semlock_type); PyObject *py_sem_value_max; /* Some systems define SEM_VALUE_MAX as an unsigned value that * causes it to be negative when used as an int (NetBSD). From 4e71aac7f5d12d468966c6fece4b4148a1d9cdd9 Mon Sep 17 00:00:00 2001 From: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com> Date: Tue, 28 Jun 2022 22:04:24 +0530 Subject: [PATCH 5/8] Apply suggestions from code review Co-authored-by: Erlend Egeberg Aasland --- Modules/_multiprocessing/multiprocessing.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/Modules/_multiprocessing/multiprocessing.c b/Modules/_multiprocessing/multiprocessing.c index 3f958456abddaa..8566ec37712722 100644 --- a/Modules/_multiprocessing/multiprocessing.c +++ b/Modules/_multiprocessing/multiprocessing.c @@ -194,20 +194,23 @@ multiprocessing_exec(PyObject *module) if (semlock_type == NULL) { return -1; } - if (PyModule_AddType(module, semlock_type) < 0) { + int rc = PyModule_AddType(module, semlock_type); + Py_DECREF(semlock_type); + if (rc < 0) { return -1; } - Py_DECREF(semlock_type); PyObject *py_sem_value_max; /* Some systems define SEM_VALUE_MAX as an unsigned value that * causes it to be negative when used as an int (NetBSD). * * Issue #28152: Use (0) instead of 0 to fix a warning on dead code * when using clang -Wunreachable-code. */ - if ((int)(SEM_VALUE_MAX) < (0)) + if ((int)(SEM_VALUE_MAX) < (0)) { py_sem_value_max = PyLong_FromLong(INT_MAX); - else + } + else { py_sem_value_max = PyLong_FromLong(SEM_VALUE_MAX); + } if (py_sem_value_max == NULL) { return -1; } From ccab6e69abd9b607ace3a22174b01780858ece59 Mon Sep 17 00:00:00 2001 From: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com> Date: Sun, 3 Jul 2022 16:31:24 +0000 Subject: [PATCH 6/8] code review from Victor --- Modules/_multiprocessing/multiprocessing.c | 1 + Modules/_multiprocessing/semaphore.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/Modules/_multiprocessing/multiprocessing.c b/Modules/_multiprocessing/multiprocessing.c index 8566ec37712722..75b82f8ad3a06a 100644 --- a/Modules/_multiprocessing/multiprocessing.c +++ b/Modules/_multiprocessing/multiprocessing.c @@ -199,6 +199,7 @@ multiprocessing_exec(PyObject *module) if (rc < 0) { return -1; } + PyObject *py_sem_value_max; /* Some systems define SEM_VALUE_MAX as an unsigned value that * causes it to be negative when used as an int (NetBSD). diff --git a/Modules/_multiprocessing/semaphore.c b/Modules/_multiprocessing/semaphore.c index 00a6ebe9818a58..58fb0eb96aeeed 100644 --- a/Modules/_multiprocessing/semaphore.c +++ b/Modules/_multiprocessing/semaphore.c @@ -749,7 +749,7 @@ static PyMemberDef semlock_members[] = { * Semaphore type */ -PyType_Slot _PyMp_SemLockType_slots[] = { +static PyType_Slot _PyMp_SemLockType_slots[] = { {Py_tp_dealloc, semlock_dealloc}, {Py_tp_getattro, PyObject_GenericGetAttr}, {Py_tp_setattro, PyObject_GenericSetAttr}, From 3e5cb841e035058965b2befb0088c55d4612b8f4 Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Sun, 3 Jul 2022 16:41:04 +0000 Subject: [PATCH 7/8] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20blu?= =?UTF-8?q?rb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../next/Library/2022-07-03-16-41-03.gh-issue-94382.zuVZeM.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Library/2022-07-03-16-41-03.gh-issue-94382.zuVZeM.rst diff --git a/Misc/NEWS.d/next/Library/2022-07-03-16-41-03.gh-issue-94382.zuVZeM.rst b/Misc/NEWS.d/next/Library/2022-07-03-16-41-03.gh-issue-94382.zuVZeM.rst new file mode 100644 index 00000000000000..7f7754bd08bda0 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2022-07-03-16-41-03.gh-issue-94382.zuVZeM.rst @@ -0,0 +1 @@ +Fix memory leak in ``_multiprocessing`` extension module when it is loaded/unloaded multiple times by converting static types to heap types. Patch by Kumar Aditya. From 919bbe0c7f1434124ee17dd0931eb384fe8b6398 Mon Sep 17 00:00:00 2001 From: Kumar Aditya <59607654+kumaraditya303@users.noreply.github.com> Date: Fri, 15 Jul 2022 10:14:15 +0000 Subject: [PATCH 8/8] edit news --- .../next/Library/2022-07-03-16-41-03.gh-issue-94382.zuVZeM.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2022-07-03-16-41-03.gh-issue-94382.zuVZeM.rst b/Misc/NEWS.d/next/Library/2022-07-03-16-41-03.gh-issue-94382.zuVZeM.rst index 7f7754bd08bda0..d79300778f762c 100644 --- a/Misc/NEWS.d/next/Library/2022-07-03-16-41-03.gh-issue-94382.zuVZeM.rst +++ b/Misc/NEWS.d/next/Library/2022-07-03-16-41-03.gh-issue-94382.zuVZeM.rst @@ -1 +1 @@ -Fix memory leak in ``_multiprocessing`` extension module when it is loaded/unloaded multiple times by converting static types to heap types. Patch by Kumar Aditya. +Port static types of ``_multiprocessing`` module to heap types. Patch by Kumar Aditya.