From 3620fa4593c3977e019aff08555c15d90c87364a Mon Sep 17 00:00:00 2001 From: sobolevn Date: Mon, 22 Jan 2024 13:44:25 +0300 Subject: [PATCH 1/8] wip --- Lib/threading.py | 3 +-- Modules/_threadmodule.c | 23 +++++++++++++++++++---- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/Lib/threading.py b/Lib/threading.py index 85aff58968082d..075d7b0dce1208 100644 --- a/Lib/threading.py +++ b/Lib/threading.py @@ -5,7 +5,6 @@ import _thread import functools import warnings -import _weakref from time import monotonic as _time from _weakrefset import WeakSet @@ -108,7 +107,7 @@ def gettrace(): # Synchronization classes -Lock = _allocate_lock +Lock = type(_allocate_lock()) def RLock(*args, **kwargs): """Factory function that returns a new reentrant lock. diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index afcf646e3bc19e..4037e254d22987 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -349,6 +349,18 @@ lock__at_fork_reinit(lockobject *self, PyObject *Py_UNUSED(args)) } #endif /* HAVE_FORK */ +static lockobject *newlockobject(PyObject *module); + +static PyObject * +lock_new(PyTypeObject *type, PyObject *args, PyObject *kwargs) +{ + PyObject *module = PyType_GetModuleByDef(type, &thread_module); + if (module == NULL) { + return NULL; + } + return (PyObject *)newlockobject(module); +} + static PyMethodDef lock_methods[] = { {"acquire_lock", _PyCFunction_CAST(lock_PyThread_acquire_lock), @@ -398,6 +410,7 @@ static PyType_Slot lock_type_slots[] = { {Py_tp_methods, lock_methods}, {Py_tp_traverse, lock_traverse}, {Py_tp_members, lock_type_members}, + {Py_tp_new, lock_new}, {0, 0} }; @@ -405,7 +418,7 @@ static PyType_Spec lock_type_spec = { .name = "_thread.lock", .basicsize = sizeof(lockobject), .flags = (Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_GC | - Py_TPFLAGS_DISALLOW_INSTANTIATION | Py_TPFLAGS_IMMUTABLETYPE), + Py_TPFLAGS_IMMUTABLETYPE), .slots = lock_type_slots, }; @@ -1439,8 +1452,6 @@ A subthread can use this function to interrupt the main thread.\n\ Note: the default signal handler for SIGINT raises ``KeyboardInterrupt``." ); -static lockobject *newlockobject(PyObject *module); - static PyObject * thread_PyThread_allocate_lock(PyObject *module, PyObject *Py_UNUSED(ignored)) { @@ -1838,10 +1849,14 @@ thread_module_exec(PyObject *module) } // Lock - state->lock_type = (PyTypeObject *)PyType_FromSpec(&lock_type_spec); + state->lock_type = (PyTypeObject *)PyType_FromModuleAndSpec(module, &lock_type_spec, NULL); if (state->lock_type == NULL) { return -1; } + if (PyModule_AddType(module, state->lock_type) < 0) { + return -1; + } + // Old alias: lock -> LockType if (PyDict_SetItemString(d, "LockType", (PyObject *)state->lock_type) < 0) { return -1; } From 4b1c4f6bedb95aa16ecbf429709d415e286617b3 Mon Sep 17 00:00:00 2001 From: sobolevn Date: Tue, 23 Jan 2024 14:12:06 +0300 Subject: [PATCH 2/8] gh-114315: Make `threading.Lock` a real class --- Doc/library/threading.rst | 8 +++++--- Lib/test/test_threading.py | 14 +++++++++----- Lib/threading.py | 3 ++- .../2024-01-23-14-11-49.gh-issue-114315.KeVdzl.rst | 2 ++ Modules/_threadmodule.c | 12 ++++++++++++ 5 files changed, 30 insertions(+), 9 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2024-01-23-14-11-49.gh-issue-114315.KeVdzl.rst diff --git a/Doc/library/threading.rst b/Doc/library/threading.rst index b85b7f008d1594..7a018249fb5b6c 100644 --- a/Doc/library/threading.rst +++ b/Doc/library/threading.rst @@ -534,9 +534,11 @@ All methods are executed atomically. lock, subsequent attempts to acquire it block, until it is released; any thread may release it. - Note that ``Lock`` is actually a factory function which returns an instance - of the most efficient version of the concrete Lock class that is supported - by the platform. + .. versionchanged:: 3.13 + Prior to 3.13 ``Lock`` actually used to be a factory + function which returned an instance + of the most efficient version of the concrete Lock class that is supported + by the platform. .. method:: acquire(blocking=True, timeout=-1) diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index 3060af44fd7e3d..7c5fb003bc34ef 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -170,11 +170,15 @@ def test_args_argument(self): t.start() t.join() - @cpython_only - def test_disallow_instantiation(self): - # Ensure that the type disallows instantiation (bpo-43916) - lock = threading.Lock() - test.support.check_disallow_instantiation(self, type(lock)) + def test_lock_no_args(self): + threading.Lock() # works + self.assertRaises(TypeError, threading.Lock, 1) + self.assertRaises(TypeError, threading.Lock, a=1) + self.assertRaises(TypeError, threading.Lock, 1, 2, a=1, b=2) + + def test_lock_or_none(self): + import types + self.assertIsInstance(threading.Lock | None, types.UnionType) # Create a bunch of threads, let each do some work, wait until all are # done. diff --git a/Lib/threading.py b/Lib/threading.py index 075d7b0dce1208..9dddf595cb5bfb 100644 --- a/Lib/threading.py +++ b/Lib/threading.py @@ -36,6 +36,7 @@ _start_joinable_thread = _thread.start_joinable_thread _daemon_threads_allowed = _thread.daemon_threads_allowed _allocate_lock = _thread.allocate_lock +_LockType = _thread.LockType _set_sentinel = _thread._set_sentinel get_ident = _thread.get_ident _is_main_interpreter = _thread._is_main_interpreter @@ -107,7 +108,7 @@ def gettrace(): # Synchronization classes -Lock = type(_allocate_lock()) +Lock = _LockType def RLock(*args, **kwargs): """Factory function that returns a new reentrant lock. diff --git a/Misc/NEWS.d/next/Library/2024-01-23-14-11-49.gh-issue-114315.KeVdzl.rst b/Misc/NEWS.d/next/Library/2024-01-23-14-11-49.gh-issue-114315.KeVdzl.rst new file mode 100644 index 00000000000000..a8a19fc525d019 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2024-01-23-14-11-49.gh-issue-114315.KeVdzl.rst @@ -0,0 +1,2 @@ +Make :class:`threading.Lock` a real class, not a factory function. Add +``__new__`` to ``_thread.lock`` type. diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 4037e254d22987..b0b9b42ec087f7 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -5,6 +5,7 @@ #include "Python.h" #include "pycore_interp.h" // _PyInterpreterState.threads.count #include "pycore_moduleobject.h" // _PyModule_GetState() +#include "pycore_modsupport.h" // _PyArg_NoKeywords() #include "pycore_pylifecycle.h" #include "pycore_pystate.h" // _PyThreadState_SetCurrent() #include "pycore_sysmodule.h" // _PySys_GetAttr() @@ -354,11 +355,22 @@ static lockobject *newlockobject(PyObject *module); static PyObject * lock_new(PyTypeObject *type, PyObject *args, PyObject *kwargs) { + // convert to AC? + if (!_PyArg_NoKeywords("lock", kwargs)) { + goto error; + } + if (!_PyArg_CheckPositional("lock", PyTuple_GET_SIZE(args), 0, 0)) { + goto error; + } + PyObject *module = PyType_GetModuleByDef(type, &thread_module); if (module == NULL) { return NULL; } return (PyObject *)newlockobject(module); + +error: + return NULL; } From 427b450afe384790f83bf667d9b7af3269300bee Mon Sep 17 00:00:00 2001 From: sobolevn Date: Tue, 23 Jan 2024 14:13:51 +0300 Subject: [PATCH 3/8] Assert module instead --- Modules/_threadmodule.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index b0b9b42ec087f7..ee7aa4763a2298 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -364,9 +364,7 @@ lock_new(PyTypeObject *type, PyObject *args, PyObject *kwargs) } PyObject *module = PyType_GetModuleByDef(type, &thread_module); - if (module == NULL) { - return NULL; - } + assert(module != NULL); return (PyObject *)newlockobject(module); error: From 1a1b4e8638352ebde38b7169fa401f987cf4c5dc Mon Sep 17 00:00:00 2001 From: sobolevn Date: Tue, 23 Jan 2024 14:14:36 +0300 Subject: [PATCH 4/8] Do not add to module --- Modules/_threadmodule.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index ee7aa4763a2298..7b97fed426c0fa 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -1863,10 +1863,6 @@ thread_module_exec(PyObject *module) if (state->lock_type == NULL) { return -1; } - if (PyModule_AddType(module, state->lock_type) < 0) { - return -1; - } - // Old alias: lock -> LockType if (PyDict_SetItemString(d, "LockType", (PyObject *)state->lock_type) < 0) { return -1; } From aa896b1d24efbc02db2e616c02dcb97faa1c3acc Mon Sep 17 00:00:00 2001 From: Nikita Sobolev Date: Tue, 23 Jan 2024 14:20:21 +0300 Subject: [PATCH 5/8] Update Doc/library/threading.rst Co-authored-by: Alex Waygood --- Doc/library/threading.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Doc/library/threading.rst b/Doc/library/threading.rst index 7a018249fb5b6c..e5a02420ef5681 100644 --- a/Doc/library/threading.rst +++ b/Doc/library/threading.rst @@ -535,8 +535,8 @@ All methods are executed atomically. thread may release it. .. versionchanged:: 3.13 - Prior to 3.13 ``Lock`` actually used to be a factory - function which returned an instance + ``Lock`` is now a class. In earlier Python versions, + ``Lock`` used to be a factory function which returned an instance of the most efficient version of the concrete Lock class that is supported by the platform. From 9d30863a961b12537ea96ba35194d7b5e5e4de99 Mon Sep 17 00:00:00 2001 From: sobolevn Date: Tue, 23 Jan 2024 15:05:02 +0300 Subject: [PATCH 6/8] Add `lock` to module --- Modules/_threadmodule.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Modules/_threadmodule.c b/Modules/_threadmodule.c index 7b97fed426c0fa..ee7aa4763a2298 100644 --- a/Modules/_threadmodule.c +++ b/Modules/_threadmodule.c @@ -1863,6 +1863,10 @@ thread_module_exec(PyObject *module) if (state->lock_type == NULL) { return -1; } + if (PyModule_AddType(module, state->lock_type) < 0) { + return -1; + } + // Old alias: lock -> LockType if (PyDict_SetItemString(d, "LockType", (PyObject *)state->lock_type) < 0) { return -1; } From 3311e0abc73b85dc5f1460ef40b13ce3d843ea98 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Thu, 25 Jan 2024 11:09:43 -0800 Subject: [PATCH 7/8] Simplify the versionchanged wording. --- Doc/library/threading.rst | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/Doc/library/threading.rst b/Doc/library/threading.rst index e5a02420ef5681..5fbf9379b8202c 100644 --- a/Doc/library/threading.rst +++ b/Doc/library/threading.rst @@ -535,10 +535,9 @@ All methods are executed atomically. thread may release it. .. versionchanged:: 3.13 - ``Lock`` is now a class. In earlier Python versions, - ``Lock`` used to be a factory function which returned an instance - of the most efficient version of the concrete Lock class that is supported - by the platform. + ``Lock`` is now a class. In earlier Pythons, ``Lock`` was a factory + function which returned an instance of the underlying private lock + type. .. method:: acquire(blocking=True, timeout=-1) From b33a71ae609385d5e368ab98088ca2f0e6cf6aa9 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith [Google LLC]" Date: Thu, 25 Jan 2024 11:15:52 -0800 Subject: [PATCH 8/8] Test that threading.Lock cannot be subclassed. --- Lib/test/test_threading.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/Lib/test/test_threading.py b/Lib/test/test_threading.py index 7c5fb003bc34ef..a314f8180ebd6b 100644 --- a/Lib/test/test_threading.py +++ b/Lib/test/test_threading.py @@ -176,6 +176,12 @@ def test_lock_no_args(self): self.assertRaises(TypeError, threading.Lock, a=1) self.assertRaises(TypeError, threading.Lock, 1, 2, a=1, b=2) + def test_lock_no_subclass(self): + # Intentionally disallow subclasses of threading.Lock because they have + # never been allowed, so why start now just because the type is public? + with self.assertRaises(TypeError): + class MyLock(threading.Lock): pass + def test_lock_or_none(self): import types self.assertIsInstance(threading.Lock | None, types.UnionType)