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)