Allow manually destroying borrowed attribute proxies (#1854)

docs/project/changelog.md

@@ -32,6 +32,10 @@ substitutions:
   arguments and return values are automatically destroyed when the function is
   finished. {pr}`1573`
 
+- {{Fix}} It is now possible to destroy borrowed attribute `PyProxy` of a
+  `PyProxy` (as introduced by {pr}`1636`) before destroying the root `PyProxy`.
+  {pr}`1854`
+
 ### pyodide-build
 
 - {{API}} By default only a minimal set of packages is built. To build all

pyodide-build/pyodide_build/testing.py

@@ -80,17 +80,13 @@ def run_in_pyodide(
                     selenium.run_js(
                         f"""
                         let eval_code = pyodide.pyodide_py.eval_code;
-                        try {{
-                            eval_code.callKwargs(
-                                {{
-                                    source : atob({encoded}.join("")),
-                                    globals : pyodide._module.globals,
-                                    filename : {filename!r}
-                                }}
-                            )
-                        }} finally {{
-                            eval_code.destroy();
-                        }}
+                        eval_code.callKwargs(
+                            {{
+                                source : atob({encoded}.join("")),
+                                globals : pyodide._module.globals,
+                                filename : {filename!r}
+                            }}
+                        );
                         """
                     )
                     # When invoking the function, use the default filename <eval>

src/core/pyproxy.c

@@ -235,14 +235,21 @@ finally:
 int
 _PyObject_GetMethod(PyObject* obj, PyObject* name, PyObject** method);
 
-EM_JS(int, pyproxy_mark_borrowed, (JsRef id), {
-  let proxy = Module.hiwire.get_value(id);
-  Module.pyproxy_mark_borrowed(proxy);
-});
-
 EM_JS(JsRef, proxy_cache_get, (JsRef proxyCacheId, PyObject* descr), {
   let proxyCache = Module.hiwire.get_value(proxyCacheId);
-  return proxyCache.get(descr);
+  let proxyId = proxyCache.get(descr);
+  if (!proxyId) {
+    return undefined;
+  }
+  // Okay found a proxy. Is it alive?
+  if (Module.hiwire.get_value(proxyId).$$.ptr) {
+    return proxyId;
+  } else {
+    // It's dead, tidy up
+    proxyCache.delete(descr);
+    Module.hiwire.decref(proxyId);
+    return undefined;
+  }
 })
 
 // clang-format off
@@ -294,10 +301,9 @@ _pyproxy_getattr(PyObject* pyobj, JsRef idkey, JsRef proxyCache)
   FAIL_IF_NULL(idresult);
   if (pyproxy_Check(idresult)) {
     // If a getter returns a different object every time, this could potentially
-    // fill up the cache with a lot of junk. However, there is no other option
-    // that makes sense from the point of the user.
+    // fill up the cache with a lot of junk. If this is a problem, the user will
+    // have to manually destroy the attributes.
     proxy_cache_set(proxyCache, pydescr, hiwire_incref(idresult));
-    pyproxy_mark_borrowed(idresult);
   }
 
 success:

src/core/pyproxy.js

@@ -119,7 +119,7 @@ Module.pyproxy_new = function (ptrobj, cache) {
   }
   cache.refcnt++;
   Object.defineProperty(target, "$$", {
-    value: { ptr: ptrobj, type: "PyProxy", borrowed: false, cache },
+    value: { ptr: ptrobj, type: "PyProxy", cache },
   });
   Module._Py_IncRef(ptrobj);
   let proxy = new Proxy(target, PyProxyHandlers);
@@ -189,9 +189,6 @@ Module.getPyProxyClass = function (flags) {
 
 // Static methods
 Module.PyProxy_getPtr = _getPtr;
-Module.pyproxy_mark_borrowed = function (proxy) {
-  proxy.$$.borrowed = true;
-};
 
 const pyproxy_cache_destroyed_msg =
   "This borrowed attribute proxy was automatically destroyed in the " +
@@ -336,9 +333,7 @@ class PyProxyClass {
    *        destroyed".
    */
   destroy(destroyed_msg) {
-    if (!this.$$.borrowed) {
-      Module.pyproxy_destroy(this, destroyed_msg);
-    }
+    Module.pyproxy_destroy(this, destroyed_msg);
   }
   /**
    * Make a new PyProxy pointing to the same Python object.

src/tests/test_jsproxy.py

@@ -518,6 +518,44 @@ def test_nested_attribute_access():
     assert self.Float64Array.BYTES_PER_ELEMENT == 8
 
 
+def test_destroy_attribute(selenium):
+    selenium.run_js(
+        """
+        let test = pyodide.runPython(`
+            class Test:
+                a = {}
+            test = Test()
+            test
+        `);
+        pyodide.runPython(`
+            import sys
+            assert sys.getrefcount(test) == 3
+            assert sys.getrefcount(test.a) == 2
+        `);
+        test.a;
+        pyodide.runPython(`
+            assert sys.getrefcount(test) == 3
+            assert sys.getrefcount(test.a) == 3
+        `);
+        test.a.destroy();
+        pyodide.runPython(`
+            assert sys.getrefcount(test) == 3
+            assert sys.getrefcount(test.a) == 2
+        `);
+        test.a;
+        pyodide.runPython(`
+            assert sys.getrefcount(test) == 3
+            assert sys.getrefcount(test.a) == 3
+        `);
+        test.destroy();
+        pyodide.runPython(`
+            assert sys.getrefcount(test) == 2
+            assert sys.getrefcount(test.a) == 2
+        `);
+        """
+    )
+
+
 @run_in_pyodide
 def test_window_isnt_super_weird_anymore():
     import js