Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't destroy roundtrip PyProxies automatically #3369

Merged
merged 6 commits into from Dec 23, 2022

Conversation

hoodmane
Copy link
Member

@hoodmane hoodmane commented Dec 18, 2022

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).

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

Checklists

  • Add a CHANGELOG entry
  • Add / update tests
  • Add new / update outdated documentation

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();
}
@rth
Copy link
Member

rth commented Dec 20, 2022

Do you want to include this in 0.22?

@hoodmane
Copy link
Member Author

I think it would be nice to include. It makes create_proxy work a bit more reasonably.

Copy link
Member

@ryanking13 ryanking13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a failing test, otherwise LGTM. Thanks @hoodmane!

@hoodmane
Copy link
Member Author

We should also add a deprecation message for use like pyproxy.destroy("msg") rather than pyproxy.destroy({msg:"msg"}). Also, maybe it should be spelled message instead of msg?

@ryanking13
Copy link
Member

We should also add a deprecation message for use like pyproxy.destroy("msg") rather than pyproxy.destroy({msg:"msg"})

Sounds good to me. (In that case, please also update the deprecation timeline!)

Also, maybe it should be spelled message instead of msg?

I'm +1 to it.

@ryanking13 ryanking13 merged commit 83ea447 into pyodide:main Dec 23, 2022
@ryanking13 ryanking13 deleted the dont-destroy-roundtrip branch December 23, 2022 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants