From 3c4199036a7e8caa2eb26b715ab00cdf63a01422 Mon Sep 17 00:00:00 2001 From: Hood Chatham Date: Tue, 28 Sep 2021 11:34:24 -0700 Subject: [PATCH] Allow manually destroying borrowed attribute proxies (#1854) --- docs/project/changelog.md | 4 +++ pyodide-build/pyodide_build/testing.py | 18 +++++------- src/core/pyproxy.c | 24 ++++++++++------ src/core/pyproxy.js | 9 ++---- src/tests/test_jsproxy.py | 38 ++++++++++++++++++++++++++ 5 files changed, 66 insertions(+), 27 deletions(-) diff --git a/docs/project/changelog.md b/docs/project/changelog.md index d55a31933..6cd223bab 100644 --- a/docs/project/changelog.md +++ b/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 diff --git a/pyodide-build/pyodide_build/testing.py b/pyodide-build/pyodide_build/testing.py index a200ef28b..0adad2362 100644 --- a/pyodide-build/pyodide_build/testing.py +++ b/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 diff --git a/src/core/pyproxy.c b/src/core/pyproxy.c index fd8676e52..f9c490163 100644 --- a/src/core/pyproxy.c +++ b/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: diff --git a/src/core/pyproxy.js b/src/core/pyproxy.js index 0e98225b1..79f04c386 100644 --- a/src/core/pyproxy.js +++ b/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. diff --git a/src/tests/test_jsproxy.py b/src/tests/test_jsproxy.py index 2e603e7d0..f566496a0 100644 --- a/src/tests/test_jsproxy.py +++ b/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