From b50f206afa08828edc9887048f79025f0dc221d8 Mon Sep 17 00:00:00 2001 From: jianfengmao Date: Sun, 12 May 2024 14:29:25 -0600 Subject: [PATCH] Split the changes to keep the issue relevant ones --- src/main/c/jpy_jobj.c | 2 +- src/main/c/jpy_jtype.c | 14 ++++++++ .../jpy/fixtures/CyclicReferenceChild1.java | 30 +++++++++++++++++ .../jpy/fixtures/CyclicReferenceChild2.java | 17 ++++++++++ .../fixtures/CyclicReferenceGrandParent.java | 33 +++++++++++++++++++ .../jpy/fixtures/CyclicReferenceParent.java | 32 ++++++++++++++++++ src/test/python/jpy_gettype_test.py | 15 +++++++++ 7 files changed, 142 insertions(+), 1 deletion(-) create mode 100644 src/test/java/org/jpy/fixtures/CyclicReferenceChild1.java create mode 100644 src/test/java/org/jpy/fixtures/CyclicReferenceChild2.java create mode 100644 src/test/java/org/jpy/fixtures/CyclicReferenceGrandParent.java create mode 100644 src/test/java/org/jpy/fixtures/CyclicReferenceParent.java diff --git a/src/main/c/jpy_jobj.c b/src/main/c/jpy_jobj.c index bb4d3ad6..d67ff684 100644 --- a/src/main/c/jpy_jobj.c +++ b/src/main/c/jpy_jobj.c @@ -726,7 +726,7 @@ int JType_InitSlots(JPy_JType* type) typeObj = JTYPE_AS_PYTYPE(type); - // This is hacky to make the caller JType_GetType be able to call JPy_INCREF regardless if the type was already created, + // This is hacky to make the caller JType_GetType able to call JPy_INCREF regardless if the type was already created, // but it will cause a ref counting error when this function fails, and we need to remove the type from the registry // #if defined(JPY_COMPAT_39P) // Py_SET_REFCNT(typeObj, 1); diff --git a/src/main/c/jpy_jtype.c b/src/main/c/jpy_jtype.c index 69d60a05..fc0c089f 100644 --- a/src/main/c/jpy_jtype.c +++ b/src/main/c/jpy_jtype.c @@ -171,8 +171,17 @@ JPy_JType* JType_GetType(JNIEnv* jenv, jclass classRef, jboolean resolve) //printf("T2: type->tp_init=%p\n", ((PyTypeObject*)type)->tp_init); // ... before we can continue processing the super type ... + // Note, at this point, the Python type has been registered-but-yet-finalized (as done in JType_InitSlots). + // We need to delay resolving the super classes to when they are actually referenced, so that in a cyclic + // reference scenario, the super classes can still be finalized (reference-able by child classes), but NOT RESOLVED + // (its methods/fields remain absent in the type object). This is to avoid the case where its members reference some other + // registered-but-yet-finalized classes in the same class hierarchy, and these classes in turn have + // registered-but-yet-finalized super classes, causing JType_InitSlots to fail for one of them because its super + // class is not finalized. When this happens, the affected classes will not be properly resolved. if (JType_InitSuperType(jenv, type, JNI_FALSE) < 0) { PyDict_DelItem(JPy_Types, typeKey); + JPy_DECREF(typeKey); + JPy_DECREF(type); return NULL; } @@ -180,7 +189,10 @@ JPy_JType* JType_GetType(JNIEnv* jenv, jclass classRef, jboolean resolve) // ... and processing the component type. if (JType_InitComponentType(jenv, type, JNI_FALSE) < 0) { + JPy_DIAG_PRINT(JPy_DIAG_F_TYPE, "JType_GetType: error: JType_InitComponentType() failed for javaName=\"%s\"\n", type->javaName); PyDict_DelItem(JPy_Types, typeKey); + JPy_DECREF(typeKey); + JPy_DECREF(type); return NULL; } @@ -190,6 +202,8 @@ JPy_JType* JType_GetType(JNIEnv* jenv, jclass classRef, jboolean resolve) if (JType_InitSlots(type) < 0) { JPy_DIAG_PRINT(JPy_DIAG_F_TYPE, "JType_GetType: error: JType_InitSlots() failed for javaName=\"%s\"\n", type->javaName); PyDict_DelItem(JPy_Types, typeKey); + JPy_DECREF(typeKey); + JPy_DECREF(type); return NULL; } diff --git a/src/test/java/org/jpy/fixtures/CyclicReferenceChild1.java b/src/test/java/org/jpy/fixtures/CyclicReferenceChild1.java new file mode 100644 index 00000000..4cc6712b --- /dev/null +++ b/src/test/java/org/jpy/fixtures/CyclicReferenceChild1.java @@ -0,0 +1,30 @@ +// +// Copyright 2024 jpy-consortium +// + +package org.jpy.fixtures; + +import java.lang.reflect.Array; +import java.lang.reflect.Method; + +/** + * Used as a test class for the test cases in jpy_gettype_test.py + * + * @author Jianfeng Mao + */ +@SuppressWarnings("UnusedDeclaration") +public class CyclicReferenceChild1 extends CyclicReferenceParent { + private int x; + + private CyclicReferenceChild1(int x) { + this.x = x; + } + + public static CyclicReferenceChild1 of(int x) { + return new CyclicReferenceChild1(x); + } + + public int get_x() { + return this.x; + } +} diff --git a/src/test/java/org/jpy/fixtures/CyclicReferenceChild2.java b/src/test/java/org/jpy/fixtures/CyclicReferenceChild2.java new file mode 100644 index 00000000..7a127056 --- /dev/null +++ b/src/test/java/org/jpy/fixtures/CyclicReferenceChild2.java @@ -0,0 +1,17 @@ +// +// Copyright 2024 jpy-consortium +// + +package org.jpy.fixtures; + +import java.lang.reflect.Array; +import java.lang.reflect.Method; + +/** + * Used as a test class for the test cases in jpy_gettype_test.py + * + * @author Jianfeng Mao + */ +@SuppressWarnings("UnusedDeclaration") +public class CyclicReferenceChild2 extends CyclicReferenceParent { +} diff --git a/src/test/java/org/jpy/fixtures/CyclicReferenceGrandParent.java b/src/test/java/org/jpy/fixtures/CyclicReferenceGrandParent.java new file mode 100644 index 00000000..92c2fe62 --- /dev/null +++ b/src/test/java/org/jpy/fixtures/CyclicReferenceGrandParent.java @@ -0,0 +1,33 @@ +// +// Copyright 2024 jpy-consortium +// + +package org.jpy.fixtures; + +import java.lang.reflect.Array; +import java.lang.reflect.Method; + +/** + * Used as a test class for the test cases in jpy_gettype_test.py + * + * @author Jianfeng Mao + */ +@SuppressWarnings("UnusedDeclaration") +public class CyclicReferenceGrandParent { + private int x; + public int z = 100; + + public CyclicReferenceGrandParent() { + } + + public void refChild2(CyclicReferenceChild2 child2) { + } + + public int grandParentMethod() { + return 888; + } + + public int get_x() { + return this.x; + } +} diff --git a/src/test/java/org/jpy/fixtures/CyclicReferenceParent.java b/src/test/java/org/jpy/fixtures/CyclicReferenceParent.java new file mode 100644 index 00000000..1b544ccb --- /dev/null +++ b/src/test/java/org/jpy/fixtures/CyclicReferenceParent.java @@ -0,0 +1,32 @@ +// +// Copyright 2024 jpy-consortium +// + +package org.jpy.fixtures; + +import java.lang.reflect.Array; +import java.lang.reflect.Method; + +/** + * Used as a test class for the test cases in jpy_gettype_test.py + * + * @author Jianfeng Mao + */ +@SuppressWarnings("UnusedDeclaration") +public abstract class CyclicReferenceParent extends CyclicReferenceGrandParent { + private int x; + public int y = 10; + + public static CyclicReferenceChild1 of(int x) { + return CyclicReferenceChild1.of(x); + } + + public int parentMethod() { + return 88; + } + + public int get_x() { + return this.x; + } + +} diff --git a/src/test/python/jpy_gettype_test.py b/src/test/python/jpy_gettype_test.py index f9b58b1c..a7233526 100644 --- a/src/test/python/jpy_gettype_test.py +++ b/src/test/python/jpy_gettype_test.py @@ -71,6 +71,21 @@ def test_issue_74(self): for i in range(200): jpy.get_type(java_type) + def test_cyclic_reference(self): + """ + Test if delaying resolving super classes doesn't break existing class reference pattern. + """ + j_child1_class = jpy.get_type("org.jpy.fixtures.CyclicReferenceChild1") + j_child2_class = jpy.get_type("org.jpy.fixtures.CyclicReferenceChild2") + j_child2 = j_child2_class() + + j_child1 = j_child1_class.of(8) + self.assertEqual(88, j_child1.parentMethod()) + self.assertEqual(888, j_child1.grandParentMethod()) + self.assertIsNone(j_child1.refChild2(j_child2)) + self.assertEqual(8, j_child1.get_x()) + self.assertEqual(10, j_child1.y) + self.assertEqual(100, j_child1.z) if __name__ == '__main__':