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

Remove try with resource in PyDictWrapper.values() #105

Merged

Conversation

jmao-denver
Copy link
Contributor

Fixes #104

@jmao-denver jmao-denver self-assigned this Jul 5, 2023
@jmao-denver jmao-denver changed the title Remove try with resource in PDW values() Remove try with resource in PyDictWrapper.values() Jul 5, 2023
@devinrsmith
Copy link
Member

I think we need to audit all usages of asList and asDict, as it looks like there may be other callsites that use the same pattern.

@devinrsmith devinrsmith self-requested a review July 5, 2023 16:03
@devinrsmith devinrsmith added the bug Something isn't working label Jul 5, 2023
@jmao-denver
Copy link
Contributor Author

I think we need to audit all usages of asList and asDict, as it looks like there may be other callsites that use the same pattern.

I did a global search/verification and found no problem with other places that use the same pattern.

@devinrsmith devinrsmith reopened this Jul 17, 2023
@devinrsmith
Copy link
Member

I see exactly the same issues w/ org.jpy.PyDictWrapper#keySet; maybe org.jpy.PyDictWrapper#entrySet (need to look into the semantics more, org.jpy.PyDictWrapper#get(java.lang.Object) claims a borrowed reference.

@niloc132
Copy link
Contributor

I see exactly the same issues w/ org.jpy.PyDictWrapper#keySet; maybe org.jpy.PyDictWrapper#entrySet (need to look into the semantics more, org.jpy.PyDictWrapper#get(java.lang.Object) claims a borrowed reference.

I believe you're misreading (though I read it that way when I first looked at this PR): whereas PyDictWrapper.values returns PyObject.asList() (a PyListWrapper instance, wrapping the existing PyObject, and so the existing PyObject must not be closed/released), PyDictWraper.keySet() copies the returned list into a new fresh LinkedHashSet (so the old one can and must be disposed of). The entrySet() implementation does the same, but washes it through a stream, collecting instances into a new LinkedHashSet rather than returning the PyListWrapper directly.

Both keySet and entrySet have the distinction that jpy offers no Set implementation, so a copy is necessary, and a copy means that we aren't providing a proper "view" of the underlying Map, which is technically wrong. On the other hand, values is returning a List instance that reflects the changes of the underlying dict/Map, which is correct, but it does mean that the instance can't be closed. Necessarily, the caller of values() must close the object when done, but need not do so for keySet or entrySet.

PyDictWrapper.get() is a different case, but should behave like any other PyObject that is returned - the caller must close it when finished.

@devinrsmith
Copy link
Member

Good point. These are subtle issues.

@jmao-denver jmao-denver merged commit 60f2b49 into jpy-consortium:master Jul 18, 2023
46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PyDictWrapper.values() incorrectly close the underlying PyObject while it is still referenced.
3 participants