search

jpy-consortium / jpy

Apache License 2.0
68 stars 16 forks source link

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

Closed jmao-denver closed 12 months ago

jmao-denver commented 1 year ago

Fixes #104

devinrsmith commented 1 year ago

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.

jmao-denver commented 1 year ago

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 commented 12 months ago

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 commented 12 months ago

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 commented 12 months ago

Good point. These are subtle issues.