Address review comments

python · markshannon · Oct 7, 2022 · Oct 3, 2022 · Oct 4, 2022 · Oct 4, 2022
commit 7dcf9b04a7389e5f3eb557c596cdb56d24ad4017
diff --git 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
@@ -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
@@ -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)
 

@@ -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
@@ -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()