From 83ea44728acf93fe1ec1999a540947861aa98460 Mon Sep 17 00:00:00 2001 From: Hood Chatham Date: Thu, 22 Dec 2022 16:24:58 -0800 Subject: [PATCH] Don't destroy roundtrip PyProxies automatically (#3369) * Don't destroy roundtrip PyProxies automatically There's a bunch of places where we want to destroy a `PyProxy` if it is being handed back into Python. This way, if the user wants to keep the proxy around they can return a copy() of it (whereas otherwise they would have no way to destroy it after the JavaScript execution is ended). ```js function new_dict(){ const dict = pyodide.globals.get("dict"); const result = dict([[1,2],[3,4]]); return result; // Proxy is destroyed after it is returned! } ``` If the `PyProxy` has roundtrip set, however, it is generally a bad idea to destroy it in these cases. In many cases, this means that the object returned to Python is actually unusable (because we get a destroyed double proxy). One slightly annoying question is how to deal with the case where (1) we want the return value NOT to be destroyed and (2) it may or may not be roundtrip ``` function f(px){ // We don't want px to be destroyed so we have to return a copy (the copy // gets destroyed instead). But if px is roundtrip then the copy is also // roundtrip and neither gets destroyed so there is a leak. What to do??? return px.copy(); } --- docs/project/changelog.md | 2 +- docs/project/deprecation-timeline.md | 10 +++++--- src/core/js2python.js | 2 +- src/core/jsproxy.c | 4 +-- src/core/pyproxy.c | 9 +++++-- src/core/pyproxy.ts | 32 +++++++++++++++++++---- src/core/python2js.c | 7 +++++ src/test-js/index.test-d.ts | 3 ++- src/tests/test_pyproxy.py | 38 ++++++++++++++++++++++++++++ 9 files changed, 91 insertions(+), 16 deletions(-) diff --git a/docs/project/changelog.md b/docs/project/changelog.md index cbeff9b94..9e77d99f9 100644 --- a/docs/project/changelog.md +++ b/docs/project/changelog.md @@ -73,7 +73,7 @@ substitutions: If this is set to `True`, then when the proxy is converted back to Python, it is converted back to the same double proxy. This allows the proxy to be destroyed from Python even if no reference is retained. - {pr}`3163` + {pr}`3163`, {pr}`3369` - {{ Enhancement }} A `JsProxy` of a function now has a `__get__` descriptor method, so it's possible to use a JavaScript function as a Python method. When diff --git a/docs/project/deprecation-timeline.md b/docs/project/deprecation-timeline.md index ccd1b69f3..755d53a4b 100644 --- a/docs/project/deprecation-timeline.md +++ b/docs/project/deprecation-timeline.md @@ -12,13 +12,15 @@ deprecation warnings. More details about each item can often be found in the ## 0.24.0 -- The `messageCallback` and `errorCallback` argument to `loadPackage` and `loadPackagesFromImports` will be passed as a - named argument only. +- The `messageCallback` and `errorCallback` argument to `loadPackage` and + `loadPackagesFromImports` will be passed as a named argument only. ## 0.23.0 -Names that used to be in the root `pyodide` module and were moved to submodules -will no longer be available in the root module. +- Names that used to be in the root `pyodide` module and were moved to submodules + will no longer be available in the root module. +- The "message" argument to `PyProxy.destroy` method will no longer be accepted + as a positional argument. ## 0.21.0 diff --git a/src/core/js2python.js b/src/core/js2python.js index a800bccc3..4fd19cc8e 100644 --- a/src/core/js2python.js +++ b/src/core/js2python.js @@ -326,7 +326,7 @@ JS_FILE(js2python_init, () => { ); result = js2python_convertImmutable(result_js); if (API.isPyProxy(result_js)) { - result_js.destroy(); + Module.pyproxy_destroy(result_js, "", false); } if (result !== undefined) { return result; diff --git a/src/core/jsproxy.c b/src/core/jsproxy.c index e5c9f5146..d20cbacc2 100644 --- a/src/core/jsproxy.c +++ b/src/core/jsproxy.c @@ -2772,10 +2772,10 @@ EM_JS_REF(JsRef, get_async_js_call_done_callback, (JsRef proxies_id), { "at the end of an asynchronous function call. Try " + "using create_proxy or create_once_callable."; for (let px of proxies) { - Module.pyproxy_destroy(px, msg); + Module.pyproxy_destroy(px, msg, false); } if (API.isPyProxy(result)) { - Module.pyproxy_destroy(result, msg); + Module.pyproxy_destroy(result, msg, false); } }); }); diff --git a/src/core/pyproxy.c b/src/core/pyproxy.c index 288dd53ae..4cb24fe79 100644 --- a/src/core/pyproxy.c +++ b/src/core/pyproxy.c @@ -53,16 +53,21 @@ EM_JS(void, destroy_proxies, (JsRef proxies_id, char* msg_ptr), { } let proxies = Hiwire.get_value(proxies_id); for (let px of proxies) { - Module.pyproxy_destroy(px, msg); + Module.pyproxy_destroy(px, msg, false); } }); EM_JS(void, destroy_proxy, (JsRef proxy_id, char* msg_ptr), { + let px = Module.hiwire.get_value(proxy_id); + if (px.$$props.roundtrip) { + // Don't destroy roundtrip proxies! + return; + } let msg = undefined; if (msg_ptr) { msg = UTF8ToString(msg_ptr); } - Module.pyproxy_destroy(Module.hiwire.get_value(proxy_id), msg); + Module.pyproxy_destroy(px, msg, false); }); static PyObject* asyncio; diff --git a/src/core/pyproxy.ts b/src/core/pyproxy.ts index cbf549352..94f794505 100644 --- a/src/core/pyproxy.ts +++ b/src/core/pyproxy.ts @@ -332,16 +332,23 @@ function pyproxy_decref_cache(cache: PyProxyCache) { for (let proxy_id of cache_map.values()) { const cache_entry = Hiwire.pop_value(proxy_id); if (!cache.leaked) { - Module.pyproxy_destroy(cache_entry, pyproxy_cache_destroyed_msg); + Module.pyproxy_destroy(cache_entry, pyproxy_cache_destroyed_msg, true); } } } } -Module.pyproxy_destroy = function (proxy: PyProxy, destroyed_msg: string) { +Module.pyproxy_destroy = function ( + proxy: PyProxy, + destroyed_msg: string, + destroy_roundtrip: boolean, +) { if (proxy.$$.ptr === 0) { return; } + if (!destroy_roundtrip && proxy.$$props.roundtrip) { + return; + } let ptrobj = _getPtr(proxy); Module.finalizationRegistry.unregister(proxy.$$); destroyed_msg = destroyed_msg || "Object has already been destroyed"; @@ -428,6 +435,10 @@ Module.callPyObject = function (ptrobj: number, jsargs: any) { export type PyProxy = PyProxyClass & { [x: string]: any }; +const DESTROY_MSG_POSITIONAL_ARG_DEPRECATED = + "Using a positional argument for the message argument for 'destroy' is deprecated and will be removed in v0.23"; +let DESTROY_MSG_POSITIONAL_ARG_WARNED = false; + export class PyProxyClass { $$: { ptr: number; @@ -489,11 +500,22 @@ export class PyProxyClass { * collected, however there is no guarantee that the finalizer will be run in * a timely manner so it is better to ``destroy`` the proxy explicitly. * - * @param destroyed_msg The error message to print if use is attempted after + * @param options + * @param options.message The error message to print if use is attempted after * destroying. Defaults to "Object has already been destroyed". + * */ - destroy(destroyed_msg?: string) { - Module.pyproxy_destroy(this, destroyed_msg); + destroy(options: { message?: string; destroyRoundtrip?: boolean } = {}) { + if (typeof options === "string") { + if (!DESTROY_MSG_POSITIONAL_ARG_WARNED) { + DESTROY_MSG_POSITIONAL_ARG_WARNED = true; + console.warn(DESTROY_MSG_POSITIONAL_ARG_DEPRECATED); + } + options = { message: options }; + } + options = Object.assign({ message: "", destroyRoundtrip: true }, options); + const { message: m, destroyRoundtrip: d } = options; + Module.pyproxy_destroy(this, m, d); } /** * Make a new PyProxy pointing to the same Python object. diff --git a/src/core/python2js.c b/src/core/python2js.c index 8e09e84ba..2e4ba99da 100644 --- a/src/core/python2js.c +++ b/src/core/python2js.c @@ -852,6 +852,13 @@ finally: return py_result; } +// As contrasts `destroy_proxies` defined in pyproxy.c and declared in +// pyproxy.h: +// 1. This handles JavaScript errors, for the other one JS errors are fatal. +// 2. This calls `proxy.destroy`, so if it is some other object with a `destroy` +// method, that will get called (is this a good thing??) +// 3. destroy_proxies won't destroy proxies with roundtrip set to true, this +// will. EM_JS_NUM(errcode, destroy_proxies_js, (JsRef proxies_id), { for (let proxy of Hiwire.get_value(proxies_id)) { proxy.destroy(); diff --git a/src/test-js/index.test-d.ts b/src/test-js/index.test-d.ts index 978761e78..b50d82a3a 100644 --- a/src/test-js/index.test-d.ts +++ b/src/test-js/index.test-d.ts @@ -85,7 +85,8 @@ async function main() { expectType(px.x); expectType(px.copy()); - expectType(px.destroy("blah")); + expectType(px.destroy({ message: "blah" })); + expectType(px.destroy({ destroyRoundtrip: false })); expectType(px.destroy()); expectType(px.toJs()); expectType(px.toJs({})); diff --git a/src/tests/test_pyproxy.py b/src/tests/test_pyproxy.py index d73f4a93c..4b38f4103 100644 --- a/src/tests/test_pyproxy.py +++ b/src/tests/test_pyproxy.py @@ -1380,3 +1380,41 @@ async def test_async_gen_throw(selenium): """ )(g3()) assert p.to_py() == [{"done": False, "value": 1}, {"done": True, "value": None}] + + +@run_in_pyodide +def test_roundtrip_no_destroy(selenium): + from pyodide.code import run_js + from pyodide.ffi import create_proxy + + def isalive(p): + return getattr(p, "$$").ptr != 0 + + p = create_proxy({1: 2}) + run_js("(x) => x")(p) + assert isalive(p) + run_js( + """ + (p) => { + p.destroy({destroyRoundtrip : false}); + } + """ + )(p) + assert isalive(p) + run_js( + """ + (p) => { + p.destroy({destroyRoundtrip : true}); + } + """ + )(p) + assert not isalive(p) + p = create_proxy({1: 2}) + run_js( + """ + (p) => { + p.destroy(); + } + """ + )(p) + assert not isalive(p)