From 7dcf9b04a7389e5f3eb557c596cdb56d24ad4017 Mon Sep 17 00:00:00 2001 From: Carl Meyer Date: Thu, 6 Oct 2022 10:34:59 -0700 Subject: [PATCH] Address review comments --- Doc/c-api/dict.rst | 12 +- Include/cpython/dictobject.h | 5 +- Include/internal/pycore_dict.h | 4 +- Include/internal/pycore_interp.h | 2 +- Lib/test/test_capi.py | 132 ++++++++++++++++++ Modules/_testcapimodule.c | 223 ++++++++++--------------------- Objects/dictobject.c | 26 +++- 7 files changed, 243 insertions(+), 161 deletions(-) diff --git a/Doc/c-api/dict.rst b/Doc/c-api/dict.rst index 0fedf2c7d9030c..97f83d522af2d4 100644 --- a/Doc/c-api/dict.rst +++ b/Doc/c-api/dict.rst @@ -246,6 +246,12 @@ Dictionary Objects of error (e.g. no more watcher IDs available), return ``-1`` and set an exception. +.. c:function:: int PyDict_ClearWatcher(int watcher_id) + + Clear watcher identified by *watcher_id* previously returned from + :c:func:`PyDict_AddWatcher`. Return ``0`` on success, ``-1`` on error (e.g. + if the given *watcher_id* was never registered.) + .. c:function:: int PyDict_Watch(int watcher_id, PyObject *dict) Mark dictionary *dict* as watched. The callback granted *watcher_id* by @@ -258,7 +264,7 @@ Dictionary Objects ``PyDict_EVENT_MODIFIED``, ``PyDict_EVENT_DELETED``, ``PyDict_EVENT_CLONED``, ``PyDict_EVENT_CLEARED``, or ``PyDict_EVENT_DEALLOCED``. -.. c:type:: void (*PyDict_WatchCallback)(PyDict_WatchEvent event, PyObject *dict, PyObject *key, PyObject *new_value) +.. c:type:: int (*PyDict_WatchCallback)(PyDict_WatchEvent event, PyObject *dict, PyObject *key, PyObject *new_value) Type of a dict watcher callback function. @@ -279,3 +285,7 @@ Dictionary Objects Callbacks occur before the notified modification to *dict* takes place, so the prior state of *dict* can be inspected. + + If an error occurs in the callback, it may return ``-1`` with an exception + set; this exception will be printed as an unraisable exception using + :c:func:`PyErr_WriteUnraisable`. On success it should return ``0``. diff --git a/Include/cpython/dictobject.h b/Include/cpython/dictobject.h index ea9d37f0837a6e..05c751ff4b22c3 100644 --- a/Include/cpython/dictobject.h +++ b/Include/cpython/dictobject.h @@ -98,10 +98,11 @@ typedef enum { // Callback to be invoked when a watched dict is cleared, dealloced, or modified. // In clear/dealloc case, key and new_value will be NULL. Otherwise, new_value will be the // new value for key, NULL if key is being deleted. -typedef void(*PyDict_WatchCallback)(PyDict_WatchEvent event, PyObject* dict, PyObject* key, PyObject* new_value); +typedef int(*PyDict_WatchCallback)(PyDict_WatchEvent event, PyObject* dict, PyObject* key, PyObject* new_value); -// Register a dict-watcher callback +// Register/unregister a dict-watcher callback PyAPI_FUNC(int) PyDict_AddWatcher(PyDict_WatchCallback callback); +PyAPI_FUNC(int) PyDict_ClearWatcher(int watcher_id); // Mark given dictionary as "watched" (callback will be called if it is modified) PyAPI_FUNC(int) PyDict_Watch(int watcher_id, PyObject* dict); diff --git a/Include/internal/pycore_dict.h b/Include/internal/pycore_dict.h index fda97dc0faeec3..ae4094a095d879 100644 --- a/Include/internal/pycore_dict.h +++ b/Include/internal/pycore_dict.h @@ -155,8 +155,8 @@ struct _dictvalues { extern uint64_t _pydict_global_version; #define DICT_MAX_WATCHERS 8 -#define DICT_VERSION_MASK 255 -#define DICT_VERSION_INCREMENT 256 +#define DICT_VERSION_INCREMENT (1 << DICT_MAX_WATCHERS) +#define DICT_VERSION_MASK (DICT_VERSION_INCREMENT - 1) #define DICT_NEXT_VERSION() (_pydict_global_version += DICT_VERSION_INCREMENT) diff --git a/Include/internal/pycore_interp.h b/Include/internal/pycore_interp.h index d327ead8473378..8cecd5ab3e541e 100644 --- a/Include/internal/pycore_interp.h +++ b/Include/internal/pycore_interp.h @@ -144,7 +144,7 @@ struct _is { // Initialized to _PyEval_EvalFrameDefault(). _PyFrameEvalFunction eval_frame; - void *dict_watchers[8]; + PyDict_WatchCallback dict_watchers[DICT_MAX_WATCHERS]; Py_ssize_t co_extra_user_count; freefunc co_extra_freefuncs[MAX_CO_EXTRA_USERS]; diff --git a/Lib/test/test_capi.py b/Lib/test/test_capi.py index 2c6fe34d3b788c..cb90d55941cae7 100644 --- a/Lib/test/test_capi.py +++ b/Lib/test/test_capi.py @@ -2,6 +2,7 @@ # these are all functions _testcapi exports whose name begins with 'test_'. from collections import OrderedDict +from contextlib import contextmanager import _thread import importlib.machinery import importlib.util @@ -1393,5 +1394,136 @@ def func2(x=None): self.do_test(func2) +class TestDictWatchers(unittest.TestCase): + # types of watchers testcapimodule can add: + EVENTS = 0 # appends dict events as strings to global event list + ERROR = 1 # unconditionally sets and signals a RuntimeException + SECOND = 2 # always appends "second" to global event list + + def add_watcher(self, kind=EVENTS): + return _testcapi.add_dict_watcher(kind) + + def clear_watcher(self, watcher_id): + _testcapi.clear_dict_watcher(watcher_id) + + @contextmanager + def watcher(self, kind=EVENTS): + wid = self.add_watcher(kind) + try: + yield wid + finally: + self.clear_watcher(wid) + + def assert_events(self, expected): + actual = _testcapi.get_dict_watcher_events() + self.assertEqual(actual, expected) + + def watch(self, wid, d): + _testcapi.watch_dict(wid, d) + + def test_set_new_item(self): + d = {} + with self.watcher() as wid: + self.watch(wid, d) + d["foo"] = "bar" + self.assert_events(["new:foo:bar"]) + + def test_set_existing_item(self): + d = {"foo": "bar"} + with self.watcher() as wid: + self.watch(wid, d) + d["foo"] = "baz" + self.assert_events(["mod:foo:baz"]) + + def test_clone(self): + d = {} + d2 = {"foo": "bar"} + with self.watcher() as wid: + self.watch(wid, d) + d.update(d2) + self.assert_events(["clone"]) + + def test_no_event_if_not_watched(self): + d = {} + with self.watcher() as wid: + d["foo"] = "bar" + self.assert_events([]) + + def test_del(self): + d = {"foo": "bar"} + with self.watcher() as wid: + self.watch(wid, d) + del d["foo"] + self.assert_events(["del:foo"]) + + def test_pop(self): + d = {"foo": "bar"} + with self.watcher() as wid: + self.watch(wid, d) + d.pop("foo") + self.assert_events(["del:foo"]) + + def test_clear(self): + d = {"foo": "bar"} + with self.watcher() as wid: + self.watch(wid, d) + d.clear() + self.assert_events(["clear"]) + + def test_dealloc(self): + d = {"foo": "bar"} + with self.watcher() as wid: + self.watch(wid, d) + del d + self.assert_events(["dealloc"]) + + def test_error(self): + d = {} + unraisables = [] + def unraisable_hook(unraisable): + unraisables.append(unraisable) + with self.watcher(kind=self.ERROR) as wid: + self.watch(wid, d) + orig_unraisable_hook = sys.unraisablehook + sys.unraisablehook = unraisable_hook + try: + d["foo"] = "bar" + finally: + sys.unraisablehook = orig_unraisable_hook + self.assert_events([]) + self.assertEqual(len(unraisables), 1) + unraisable = unraisables[0] + self.assertIs(unraisable.object, d) + self.assertEqual(str(unraisable.exc_value), "boom!") + + def test_two_watchers(self): + d1 = {} + d2 = {} + with self.watcher() as wid1: + with self.watcher(kind=self.SECOND) as wid2: + self.watch(wid1, d1) + self.watch(wid2, d2) + d1["foo"] = "bar" + d2["hmm"] = "baz" + self.assert_events(["new:foo:bar", "second"]) + + def test_watch_non_dict(self): + with self.watcher() as wid: + with self.assertRaisesRegex(ValueError, r"Cannot watch non-dictionary"): + self.watch(wid, 1) + + def test_watch_out_of_range_watcher_id(self): + d = {} + with self.assertRaisesRegex(ValueError, r"Invalid dict watcher ID -1"): + self.watch(-1, d) + with self.assertRaisesRegex(ValueError, r"Invalid dict watcher ID 8"): + self.watch(8, d) # DICT_MAX_WATCHERS = 8 + + def test_unassigned_watcher_id(self): + d = {} + with self.assertRaisesRegex(ValueError, r"No dict watcher set for ID 1"): + self.watch(1, d) + + if __name__ == "__main__": unittest.main() diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c index 10c133c602d60b..624aaed4ba918a 100644 --- a/Modules/_testcapimodule.c +++ b/Modules/_testcapimodule.c @@ -5171,8 +5171,9 @@ test_tstate_capi(PyObject *self, PyObject *Py_UNUSED(args)) // Test dict watching static PyObject *g_dict_watch_events; +static int g_dict_watchers_installed; -static void +static int dict_watch_callback(PyDict_WatchEvent event, PyObject *dict, PyObject *key, @@ -5201,191 +5202,106 @@ dict_watch_callback(PyDict_WatchEvent event, default: msg = PyUnicode_FromString("unknown"); } - assert(PyList_Check(g_dict_watch_events)); - PyList_Append(g_dict_watch_events, msg); -} - -static void -dict_watch_callback_2(PyDict_WatchEvent event, - PyObject *dict, - PyObject *key, - PyObject *new_value) -{ - PyObject *msg = PyUnicode_FromString("second"); - PyList_Append(g_dict_watch_events, msg); -} - -static int -dict_watch_assert(Py_ssize_t expected_num_events, - const char *expected_last_msg) -{ - char buf[512]; - Py_ssize_t actual_num_events = PyList_Size(g_dict_watch_events); - if (expected_num_events != actual_num_events) { - snprintf(buf, - 512, - "got %d dict watch events, expected %d", - (int)actual_num_events, - (int)expected_num_events); - raiseTestError("test_watch_dict", (const char *)&buf); + if (!msg) { return -1; } - PyObject *last_msg = PyList_GetItem(g_dict_watch_events, - PyList_Size(g_dict_watch_events)-1); - if (PyUnicode_CompareWithASCIIString(last_msg, expected_last_msg)) { - snprintf(buf, - 512, - "last event is '%s', expected '%s'", - PyUnicode_AsUTF8(last_msg), - expected_last_msg); - raiseTestError("test_watch_dict", (const char *)&buf); + assert(PyList_Check(g_dict_watch_events)); + if (PyList_Append(g_dict_watch_events, msg) < 0) { + Py_DECREF(msg); return -1; } return 0; } static int -try_watch(int watcher_id, PyObject *obj) { - if (PyDict_Watch(watcher_id, obj)) { - raiseTestError("test_watch_dict", "PyDict_Watch() failed on dict"); +dict_watch_callback_second(PyDict_WatchEvent event, + PyObject *dict, + PyObject *key, + PyObject *new_value) +{ + PyObject *msg = PyUnicode_FromString("second"); + if (!msg) { + return -1; + } + if (PyList_Append(g_dict_watch_events, msg) < 0) { return -1; } return 0; } static int -dict_watch_assert_error(int watcher_id, PyObject *obj, const char *fail_msg) +dict_watch_callback_error(PyDict_WatchEvent event, + PyObject *dict, + PyObject *key, + PyObject *new_value) { - if (!PyDict_Watch(watcher_id, obj)) { - raiseTestError("test_watch_dict", fail_msg); - return -1; - } else if (!PyErr_Occurred()) { - raiseTestError("test_watch_dict", "PyDict_Watch() returned error code without exception set"); - return -1; - } else { - PyErr_Clear(); - } - return 0; + PyErr_SetString(PyExc_RuntimeError, "boom!"); + return -1; } static PyObject * -test_watch_dict(PyObject *self, PyObject *Py_UNUSED(args)) +add_dict_watcher(PyObject *self, PyObject *kind) { - PyObject *watched = PyDict_New(); - PyObject *unwatched = PyDict_New(); - PyObject *one = PyLong_FromLong(1); - PyObject *two = PyLong_FromLong(2); - PyObject *key1 = PyUnicode_FromString("key1"); - PyObject *key2 = PyUnicode_FromString("key2"); - - g_dict_watch_events = PyList_New(0); - - int wid = PyDict_AddWatcher(dict_watch_callback); - if (try_watch(wid, watched)) { - return NULL; - } - - PyDict_SetItem(unwatched, key1, two); - PyDict_Merge(watched, unwatched, 1); - - if (dict_watch_assert(1, "clone")) { - return NULL; - } - - PyDict_SetItem(watched, key1, one); - PyDict_SetItem(unwatched, key1, one); - - if (dict_watch_assert(2, "mod:key1:1")) { - return NULL; - } - - PyDict_SetItemString(watched, "key1", two); - PyDict_SetItemString(unwatched, "key1", two); - - if (dict_watch_assert(3, "mod:key1:2")) { - return NULL; - } - - PyDict_SetItem(watched, key2, one); - PyDict_SetItem(unwatched, key2, one); - - if (dict_watch_assert(4, "new:key2:1")) { - return NULL; - } - - _PyDict_Pop(watched, key2, Py_None); - _PyDict_Pop(unwatched, key2, Py_None); - - if (dict_watch_assert(5, "del:key2")) { - return NULL; - } - - PyDict_DelItemString(watched, "key1"); - PyDict_DelItemString(unwatched, "key1"); - - if (dict_watch_assert(6, "del:key1")) { - return NULL; - } - - PyDict_SetDefault(watched, key1, one); - PyDict_SetDefault(unwatched, key1, one); - - if (dict_watch_assert(7, "new:key1:1")) { - return NULL; - } - - int wid2 = PyDict_AddWatcher(dict_watch_callback_2); - if (try_watch(wid2, unwatched)) { - return NULL; + int watcher_id; + assert(PyLong_Check(kind)); + long kind_l = PyLong_AsLong(kind); + if (kind_l == 2) { + watcher_id = PyDict_AddWatcher(dict_watch_callback_second); + } else if (kind_l == 1) { + watcher_id = PyDict_AddWatcher(dict_watch_callback_error); + } else { + watcher_id = PyDict_AddWatcher(dict_watch_callback); } - - PyDict_Clear(watched); - - if (dict_watch_assert(8, "clear")) { + if (watcher_id < 0) { return NULL; } - - PyDict_Clear(unwatched); - - if (dict_watch_assert(9, "second")) { - return NULL; + if (!g_dict_watchers_installed) { + assert(!g_dict_watch_events); + if (!(g_dict_watch_events = PyList_New(0))) { + return NULL; + } } + g_dict_watchers_installed++; + return PyLong_FromLong(watcher_id); +} - PyObject *copy = PyDict_Copy(watched); - // copied dict is not watched, so this does not add an event - Py_CLEAR(copy); - - Py_CLEAR(watched); - - if (dict_watch_assert(10, "dealloc")) { +static PyObject * +clear_dict_watcher(PyObject *self, PyObject *watcher_id) +{ + if (PyDict_ClearWatcher(PyLong_AsLong(watcher_id))) { return NULL; } - - // it is an error to try to watch a non-dict - if (dict_watch_assert_error(wid, one, "PyDict_Watch() succeeded on non-dict")) { - return NULL; + g_dict_watchers_installed--; + if (!g_dict_watchers_installed) { + assert(g_dict_watch_events); + Py_CLEAR(g_dict_watch_events); } + Py_RETURN_NONE; +} - // It is an error to pass an out-of-range watcher ID - if (dict_watch_assert_error(-1, unwatched, "PyDict_Watch() succeeded on negative watcher ID")) { +static PyObject * +watch_dict(PyObject *self, PyObject *args) +{ + PyObject *dict; + int watcher_id; + if (!PyArg_ParseTuple(args, "iO", &watcher_id, &dict)) { return NULL; } - if (dict_watch_assert_error(8, unwatched, "PyDict_Watch() succeeded on too-large watcher ID")) { + if (PyDict_Watch(watcher_id, dict)) { return NULL; } + Py_RETURN_NONE; +} - // It is an error to pass a never-registered watcher ID - if (dict_watch_assert_error(7, unwatched, "PyDict_Watch() succeeded on unused watcher ID")) { +static PyObject * +get_dict_watcher_events(PyObject *self, PyObject *Py_UNUSED(args)) +{ + if (!g_dict_watch_events) { + PyErr_SetString(PyExc_RuntimeError, "no watchers active"); return NULL; } - - Py_CLEAR(unwatched); - Py_CLEAR(g_dict_watch_events); - Py_DECREF(one); - Py_DECREF(two); - Py_DECREF(key1); - Py_DECREF(key2); - Py_RETURN_NONE; + Py_INCREF(g_dict_watch_events); + return g_dict_watch_events; } @@ -5982,7 +5898,10 @@ static PyMethodDef TestMethods[] = { {"settrace_to_record", settrace_to_record, METH_O, NULL}, {"test_macros", test_macros, METH_NOARGS, NULL}, {"clear_managed_dict", clear_managed_dict, METH_O, NULL}, - {"test_watch_dict", test_watch_dict, METH_NOARGS, NULL}, + {"add_dict_watcher", add_dict_watcher, METH_O, NULL}, + {"clear_dict_watcher", clear_dict_watcher, METH_O, NULL}, + {"watch_dict", watch_dict, METH_VARARGS, NULL}, + {"get_dict_watcher_events", get_dict_watcher_events, METH_NOARGS, NULL}, {NULL, NULL} /* sentinel */ }; diff --git a/Objects/dictobject.c b/Objects/dictobject.c index 395b8a77c5fafd..f85385017b199d 100644 --- a/Objects/dictobject.c +++ b/Objects/dictobject.c @@ -5747,7 +5747,7 @@ PyDict_AddWatcher(PyDict_WatchCallback callback) for (int i = 0; i < DICT_MAX_WATCHERS; i++) { if (!interp->dict_watchers[i]) { - interp->dict_watchers[i] = (void*)callback; + interp->dict_watchers[i] = callback; return i; } } @@ -5756,6 +5756,22 @@ PyDict_AddWatcher(PyDict_WatchCallback callback) return -1; } +int +PyDict_ClearWatcher(int watcher_id) +{ + if (watcher_id < 0 || watcher_id >= DICT_MAX_WATCHERS) { + PyErr_Format(PyExc_ValueError, "Invalid dict watcher ID %d", watcher_id); + return -1; + } + PyInterpreterState *interp = _PyInterpreterState_GET(); + if (!interp->dict_watchers[watcher_id]) { + PyErr_Format(PyExc_ValueError, "No dict watcher set for ID %d", watcher_id); + return -1; + } + interp->dict_watchers[watcher_id] = NULL; + return 0; +} + void _PyDict_SendEvent(int watcher_bits, PyDict_WatchEvent event, @@ -5766,9 +5782,13 @@ _PyDict_SendEvent(int watcher_bits, PyInterpreterState *interp = _PyInterpreterState_GET(); for (int i = 0; i < DICT_MAX_WATCHERS; i++) { if (watcher_bits & 1) { - PyDict_WatchCallback cb = (PyDict_WatchCallback)interp->dict_watchers[i]; + PyDict_WatchCallback cb = interp->dict_watchers[i]; if (cb) { - cb(event, (PyObject*)mp, key, value); + if (cb(event, (PyObject*)mp, key, value) < 0) { + // some dict modification paths (e.g. PyDict_Clear) can't raise, so we + // can't propagate exceptions from dict watchers. + PyErr_WriteUnraisable((PyObject *)mp); + } } } watcher_bits >>= 1;