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

Delay resolving super classes until referenced #146

Merged
merged 8 commits into from
May 21, 2024

Conversation

jmao-denver
Copy link
Contributor

@jmao-denver jmao-denver commented May 1, 2024

Fixes #142

  1. be able to generate Python types from Java classes with complicated cyclic references
  2. fix a reference counting issue when a new Python type fails to finalize
  3. fix the Python tests and disable the Java tests due to previous changes (years ago) to enable auto PyObjects cleanup in Java.
  4. add a limited GH check workflow to run the Python tests on Linux
  5. add two new Python test cases

@cpwright
Copy link
Contributor

cpwright commented May 1, 2024

We should attempt to add a unit test for this. If we understand have a deep enough understanding of the underlying problem, I expect we could have simple fixture classes to demonstrate it.

@niloc132
Copy link
Contributor

niloc132 commented May 1, 2024

Worth noting that we don't run jpy tests at this time as part of CI or releases, they don't consistently pass enough to be useful. Deephaven-core has its own modified suite that we run instead.

@jmao-denver jmao-denver changed the title Delay resolving super classes till referenced Delay resolving super classes until referenced May 2, 2024
@jmao-denver jmao-denver marked this pull request as ready for review May 6, 2024 02:18
@jmao-denver jmao-denver force-pushed the 142-get_type-crash branch 6 times, most recently from b625a98 to 48aee74 Compare May 7, 2024 18:08
.github/workflows/check.yml Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
src/test/java/org/jpy/fixtures/CyclicReferenceChild1.java Outdated Show resolved Hide resolved
src/test/python/jpy_exception_test.py Outdated Show resolved Hide resolved
src/test/python/jpy_exception_test.py Outdated Show resolved Hide resolved
src/test/python/jpy_gettype_test.py Outdated Show resolved Hide resolved
src/main/c/jpy_jtype.c Show resolved Hide resolved
src/main/c/jpy_jtype.c Outdated Show resolved Hide resolved
src/main/c/jpy_jtype.c Show resolved Hide resolved
src/main/c/jpy_jobj.c Outdated Show resolved Hide resolved
src/test/java/org/jpy/fixtures/CyclicReferenceParent.java Outdated Show resolved Hide resolved
src/test/java/org/jpy/fixtures/CyclicReferenceChild2.java Outdated Show resolved Hide resolved
src/test/java/org/jpy/fixtures/CyclicReferenceChild1.java Outdated Show resolved Hide resolved
src/test/python/jpy_array_test.py Outdated Show resolved Hide resolved
src/main/c/jpy_jobj.c Outdated Show resolved Hide resolved
.github/workflows/check.yml Outdated Show resolved Hide resolved
.github/workflows/check.yml Outdated Show resolved Hide resolved
src/main/c/jpy_jobj.c Outdated Show resolved Hide resolved
}

JPy_DIAG_PRINT(JPy_DIAG_F_TYPE, "JType_GetType: javaName=\"%s\", found=%d, resolve=%d, resolved=%d, type=%p\n", type->javaName, found, resolve, type->isResolved, type);

if (!type->isResolved && resolve) {
if (JType_ResolveType(jenv, type) < 0) {
JPy_DECREF(type);
Copy link
Member

Choose a reason for hiding this comment

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

Does this need an error log?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not b/c the functions that resolves the members (methods, fields, etc.) log the specific errors. That said it wouldn't hurt to log it at this level.

Copy link
Member

@devinrsmith devinrsmith left a comment

Choose a reason for hiding this comment

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

At first, I thought "resolve" was a proxy to java Class.forName "initialize" param, but that is not the case. "resolve" here is jpy-logic for "have I reflected over the class type so I can know all the methods I can call".

It is user accessible via jpy.get_type("SomeClass", resolve=...). We might want to add some tests around this:

AClass = jpy.get_type("AClass", resolve=False)
BClass = jpy.get_type("BClass", resolve=False) # extends AClass
CClass = jpy.get_type("CClass", resolve=False) # extends BClass

# tests to show the above classes are not resolved

jpy.get_type("BClass")

# tests to show that AClass and BClass are resolved; and CClass is still not resolved

jpy.get_type("CClass")

# tests to show that AClass, BClass, CClass are resolved

src/main/c/jpy_jtype.c Show resolved Hide resolved
src/main/c/jpy_jtype.c Show resolved Hide resolved
src/main/c/jpy_jobj.c Outdated Show resolved Hide resolved
Copy link
Member

@devinrsmith devinrsmith left a comment

Choose a reason for hiding this comment

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

I'm also noticing we may be overcounting in JType_InitComponentType and JType_InitSuperType:

    if (superClassRef != NULL) {
        type->superType = JType_GetType(jenv, superClassRef, resolve);
        if (type->superType == NULL) {
            return -1;
        }
        JPy_INCREF(type->superType);
        JPy_DELETE_LOCAL_REF(superClassRef);
    }

JType_GetType should be incrementing the ref count for us, so we shouldn't need to increment it again, right?

Copy link
Member

@devinrsmith devinrsmith left a comment

Choose a reason for hiding this comment

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

Jianfeng and I went over this and improved the reference counting logic, which I think is more correct now. Practically speaking though, once a type is successfully resolved into the jpy type ref dict, there is no (official) way to remove it, so it should stay referenced forever.

@jmao-denver jmao-denver merged commit 84628ad into jpy-consortium:master May 21, 2024
52 checks passed
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.

get_type ordering crash
5 participants