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

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

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
 

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;

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);
     }
   });
 });

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;

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.

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

src/test-js/index.test-d.ts

@@ -85,7 +85,8 @@ async function main() {
 
   expectType<any>(px.x);
   expectType<PyProxy>(px.copy());
-  expectType<void>(px.destroy("blah"));
+  expectType<void>(px.destroy({ message: "blah" }));
+  expectType<void>(px.destroy({ destroyRoundtrip: false }));
   expectType<void>(px.destroy());
   expectType<any>(px.toJs());
   expectType<any>(px.toJs({}));

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)