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

Reflect class (#534) #539

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

tshirtman
Copy link
Member

No description provided.

AndreMiras
AndreMiras previously approved these changes May 24, 2020
Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

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

LGTM

jnius/jnius_conversion.pxi Outdated Show resolved Hide resolved
jnius/reflect.py Outdated


# NOTE: See also comments on autoclass()
def reflect_class(c, include_protected=True, include_private=True):
Copy link
Member Author

Choose a reason for hiding this comment

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

better call it cls.
Also a docstring would be nice.

tests/test_reflect_class.py Outdated Show resolved Hide resolved
@tshirtman
Copy link
Member Author

hm, i though the tests would be ran if i did the PR, but apparently not, because the branch is outside this repository.

@cmacdonald
Copy link
Contributor

hm, i though the tests would be ran if i did the PR, but apparently not, because the branch is outside this repository.

See https://github.com/terrier-org/pyterrier/blob/master/.github/workflows/push.yml#L6 for an example

@cmacdonald
Copy link
Contributor

cmacdonald commented May 25, 2020

Actually:

jnius_conversion.pxi, line 203:

# r == 'java/lang/Character'
if r not in jclass_register:

I think this is a bug. in jnius_exportclass.pxi, jclass_register is keyed using a tuple, not just the string of the classname.

Also, for MetaJavaClass, the get_javaclass() defaults don't match the constructor defaults for classparams. I think these need to be a constant (cc @hx2A).

@cmacdonald
Copy link
Contributor

Further WIP committed.

  • This appears to address the RecursionError, but I want to test more.
  • Fixed access to jclass_register in conversion.pxi
  • Addresses review feedback

I will return to this branch later this week to address the key part about getting the Class object

@hx2A
Copy link
Member

hx2A commented May 25, 2020

It seems I missed a change I should have made when I modified jnius_export_class.pxi. :( Looks like it was an easy fix. I don't see any other missing calls to jclass_register in the code.

@cmacdonald
Copy link
Contributor

cmacdonald commented May 25, 2020

hi @hx2A . Sure yes. Can you comment on the discrepancy in MetaJavaClass: constructor vs the get_javaclass(). I think having constants that we could refer to in autoclass() and elsewhere should fix this all.

@hx2A
Copy link
Member

hx2A commented May 26, 2020

Can you comment on the discrepancy in MetaJavaClass: constructor vs the get_javaclass()

Yes. I reviewed the code and I see I made this mistake. When I changed the default params in the MetaJavaClass.__new__ method I should have also made a change to the get_javaclass method. Better yet I should've coded it with constants so both places in jnius_export_class.pxiand the autoclass function all referred to the same variable so problems like this wouldn't happen in the first place.

The default values are supposed to be (True, True). I can make the fix but I am not sure if I can push to @cmacdonald 's branch.

@tshirtman
Copy link
Member Author

you probably can't, but you can push your branch and he can pull from it.

@hx2A
Copy link
Member

hx2A commented May 26, 2020

I made the changes and the unit tests pass. It wasn't a big change. I apologize for not coding it this way in the first place.

I'm a little bit confused with the git commands I need to do to properly collaborate here. Below is what I did; perhaps you can tell me what I need to do next.

First I checked out @cmacdonald 's branch so there wouldn't be merge issues.

git remote add cmacdonald [email protected]:cmacdonald/pyjnius.git
git checkout cmacdonald/reflect_class_issue534

Then I did the coding changes and ran the unit tests. I pushed the changes to my forked repo with this:

git push origin HEAD:refs/cmacdonald/reflect_class_issue534

But on github I don't see a branch, or anything else to suggest this has been pushed to my origin. Do I need to create a new branch or something? This is my current status:

$ git status
HEAD detached from cmacdonald/reflect_class_issue534
nothing to commit, working tree clean

Obviously I'm not a pro at using git.

@tshirtman
Copy link
Member Author

git push origin HEAD should be enough, when you are on the branch on which you did the change, it'll create a branch with the same name on your own origin.

@hx2A
Copy link
Member

hx2A commented May 26, 2020

That didn't work because I was in a detached head state. I had to create a branch first.

git branch fix_20200526
git checkout fix_20200526
git push --set-upstream origin fix_20200526

Now there is a branch on github you can pull from.

@cmacdonald
Copy link
Contributor

Thanks @hx2A, I have some feedback on fix_20200526:

  • did you commit the file that _metajavaclass_default_classparams is defined in?
  • _DEFAULT_INCLUDE_PROTECTED and _DEFAULT_INCLUDE_PRIVATE would be more intuitive names for me?

@hx2A
Copy link
Member

hx2A commented May 26, 2020

I changed them to _DEFAULT_INCLUDE_PROTECTED and _DEFAULT_INCLUDE_PRIVATE.

These are defined in jnius/jnius_export_class.pxi. Is there a better place for them?

@cmacdonald
Copy link
Contributor

These are defined in jnius/jnius_export_class.pxi. Is there a better place for them?

Sorry, I missed them.

I think we should have a test ensuring that this behaves as expected autoclass() an object, and check that the MetaJavaClass.get_javaclass(cls_name) function returns.

cls_name = "bla.something.relatively.rare.in.jvm"
if MetaJavaClass.get_javaclass(cls_name) is not None:
            self.skipTest("%s already loaded - has this test run more than once?" % cls_name)
self.assertIsNotNone( autoclass(cls_name) )
self.assertIsNotNone( MetaJavaClass.get_javaclass(cls_name) )

@hx2A
Copy link
Member

hx2A commented May 26, 2020

Unittest added to tests/test_reflect.py. The code looks like this:

    def test_autoclass_default_params(self):
        cls_name = 'javax.crypto.Cipher'
        if MetaJavaClass.get_javaclass(cls_name.replace('.', '/')) is not None:
                    self.skipTest("%s already loaded - has this test run more than once?" % cls_name)
        self.assertIsNotNone(autoclass(cls_name))
        self.assertIsNotNone(MetaJavaClass.get_javaclass(cls_name.replace('.', '/')))

@cmacdonald
Copy link
Contributor

Thank you @hx2A, I will merge into this branch.

@hx2A
Copy link
Member

hx2A commented May 26, 2020

Wait, I just made a minor change to the branch

@hx2A
Copy link
Member

hx2A commented May 26, 2020

Slight improvement:

    def test_autoclass_default_params(self):
        cls_name = 'javax.crypto.Cipher'
        jni_name = cls_name.replace('.', '/')
        if MetaJavaClass.get_javaclass(jni_name) is not None:
                    self.skipTest("%s already loaded - has this test run more than once?" % cls_name)
        self.assertIsNotNone(autoclass(cls_name))
        self.assertIsNotNone(MetaJavaClass.get_javaclass(jni_name))

@cmacdonald
Copy link
Contributor

cmacdonald commented May 27, 2020

Hi @tshirtman and @AndreMiras.
Here is my working patch. Please see the test case for the underlying use case.

Some notes:

  • One side-benefit of this patch is that it should result in one less JNI FindClass() call each time that autoclass() is invoked.
  • I think also that the proper checking of jclass_register should make help to improve speed compared to 1.3.0
  • Most tricky part: working out JavaClass.copy_storage(classDict['_class'], jcs)
  • Part I am least happy with: jnius_conversion.pxi checking for find_javaclass() raising an exception - I think we should pass find_javaclass() a flag about exception handling.
  • Also in jnius_conversion.pxi, could we do a object.getClass()wholly using the proper JNI method, rather than fully instantiating an Object, then throwing it away once we have called getClass()? (And does this code leak a LocalRef?)

I want to test this a bit further, but now would be good time for re-review.

@cmacdonald
Copy link
Contributor

PS: A demanding suite of tests that can be use for timings might be a Good Thing.

@tshirtman tshirtman marked this pull request as ready for review June 1, 2020 00:22
Copy link
Member Author

@tshirtman tshirtman left a comment

Choose a reason for hiding this comment

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

Part I am least happy with: jnius_conversion.pxi checking for find_javaclass() raising an exception - I think we should pass find_javaclass() a flag about exception handling.

Indeed, it would be better if at least the exception we want to handle there was explicit, instead of catching any exception, bare excepts are a common source of bugs.

Also in jnius_conversion.pxi, could we do a object.getClass() wholly using the proper JNI method, rather than fully instantiating an Object, then throwing it away once we have called getClass()? (And does this code leak a LocalRef?)

I think that would be great, yes, not sure why/if the current way is needed, but that does indeed look suboptimal.

jnius/reflect.py Outdated Show resolved Hide resolved
jnius/reflect.py Outdated
Auto-reflects a class based on an instance of its class.

Parameters:
cls_object (str): a Python instance of a java.lang.Class object.
Copy link
Member Author

Choose a reason for hiding this comment

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

it's not an str.

self.assertIsNotNone(instance)


def test_dynamic_jar(self):
Copy link
Member Author

Choose a reason for hiding this comment

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

that's pretty cool :)

@tshirtman
Copy link
Member Author

Looks really cool, looking forward to merging :)

@cmacdonald
Copy link
Contributor

This patch addresses the feedback. Salient commentary:

  • 17e264c uses JNI calls, as discussed. Is there somewhere we can store the method id of Object.getClass()?
  • 4ef1593 - there was some regression here. Two things appeared here - we seemed to have multiple methods when we shouldn't, and as a result I conflated them back down; the terminated property wasn't appropriate, java.util.concurrent.ThreadPoolExecutor has a terminated() protected method. I changed that to a valid property, terminating.

Hopefully one more round should close this.

@lerela
Copy link

lerela commented Jun 25, 2020

I've tried out this branch to try to see if it would fix an issue I have (autoclass finding MyFirstClass.java but not MySecondClass.java despite them being next to each other), but my application crashes on launch if I use pyjnius from @cmacdonald's branch. It might be coming from my side, but just thought I'd let you know:

06-25 17:34:46.325 29210 29255 I python  : [INFO   ] [Logger      ] Record log in /data/user/0/com.company.package/files/app/.kivy/logs/kivy_20-06-25_2.txt
06-25 17:34:46.325 29210 29255 I python  : [INFO   ] [Kivy        ] v1.11.1
06-25 17:34:46.325 29210 29255 I python  : [INFO   ] [Kivy        ] Installed at "/data/user/0/com.company.package/files/app/_python_bundle/site-packages/kivy/__init__.pyc"
06-25 17:34:46.325 29210 29255 I python  : [INFO   ] [Python      ] v3.8.1 (default, Jun 25 2020, 15:08:54)
06-25 17:34:46.325 29210 29255 I python  : [Clang 8.0.2 (https://android.googlesource.com/toolchain/clang 40173bab62ec7462
06-25 17:34:46.325 29210 29255 I python  : [INFO   ] [Python      ] Interpreter at ""
06-25 17:34:46.327 29210 29255 I python  : [INFO   ] [Factory     ] 184 symbols loaded
06-25 17:34:47.051 29210 29255 E linker  : library "/system/lib/libc.so" ("/apex/com.android.runtime/lib/bionic/libc.so") needed or dlopened by "/data/data/com.company.package/files/app/_python_bundle/modules/_ctypes.cpython-38.so" is not accessible for the namespace: [name="(default)", ld_library_paths="", default_library_paths="/system/lib64:/system/product/lib64", permitted_paths="/system/lib64/drm:/system/lib64/extractors:/system/lib64/hw:/system/product/lib64:/system/framework:/system/app:/system/priv-app:/vendor/framework:/vendor/app:/vendor/priv-app:/system/vendor/framework:/system/vendor/app:/system/vendor/priv-app:/odm/framework:/odm/app:/odm/priv-app:/oem/app:/system/product/framework:/system/product/app:/system/product/priv-app:/data:/mnt/expand:/apex/com.android.runtime/lib64/bionic:/system/lib64/bootstrap"]
06-25 17:34:47.142 29210 29255 F myapp: runtime.cc:630]   native: #14 pc 0000000000028b84  /data/data/com.company.package/files/app/_python_bundle/site-packages/jnius/jnius.so (???)
06-25 17:34:47.143 29210 29255 F libc    : Fatal signal 6 (SIGABRT), code -1 (SI_QUEUE) in tid 29255 (SDLThread), pid 29210 (company.package)
06-25 17:34:47.260 29263 29263 F DEBUG   : *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
06-25 17:34:47.260 29263 29263 F DEBUG   : Build fingerprint: 'OnePlus/OnePlus6T/...:user/release-keys'
06-25 17:34:47.260 29263 29263 F DEBUG   : Revision: '0'
06-25 17:34:47.260 29263 29263 F DEBUG   : ABI: 'arm64'
06-25 17:34:47.260 29263 29263 F DEBUG   : Timestamp: 2020-06-25 17:34:47+0200
06-25 17:34:47.260 29263 29263 F DEBUG   : pid: 29210, tid: 29255, name: SDLThread  >>> com.company.package <<<
06-25 17:34:47.260 29263 29263 F DEBUG   : uid: 10617
06-25 17:34:47.260 29263 29263 F DEBUG   : signal 6 (SIGABRT), code -1 (SI_QUEUE), fault addr --------
06-25 17:34:47.260 29263 29263 F DEBUG   : Abort message: 'JNI DETECTED ERROR IN APPLICATION: use of deleted global reference 0x7a72
06-25 17:34:47.260 29263 29263 F DEBUG   :     from int org.libsdl.app.SDLActivity.nativeRunMain(java.lang.String, java.lang.String, java.lang.Object)'
06-25 17:34:47.260 29263 29263 F DEBUG   :     x0  0000000000000000  x1  0000000000007247  x2  0000000000000006  x3  00000073900ce280
06-25 17:34:47.260 29263 29263 F DEBUG   :     x4  fefeff7384323f97  x5  fefeff7384323f97  x6  fefeff7384323f97  x7  7f7f7f7fff7f7fff
06-25 17:34:47.260 29263 29263 F DEBUG   :     x8  00000000000000f0  x9  56c2ad083c849bcd  x10 0000000000000001  x11 0000000000000000
06-25 17:34:47.260 29263 29263 F DEBUG   :     x12 fffffff0fffffbdf  x13 ffffffffffffffff  x14 0000000000000004  x15 ffffffffffffffff
06-25 17:34:47.260 29263 29263 F DEBUG   :     x16 0000007485eff8c0  x17 0000007485edb900  x18 000000739041db78  x19 000000000000721a
06-25 17:34:47.260 29263 29263 F DEBUG   :     x20 0000000000007247  x21 00000000ffffffff  x22 0000007391316a40  x23 00000073ffec0665
06-25 17:34:47.260 29263 29263 F DEBUG   :     x24 00000073ffee21e1  x25 0000000000000001  x26 00000074005bc258  x27 00000074868357c0
06-25 17:34:47.260 29263 29263 F DEBUG   :     x28 00000074003ec338  x29 00000073900ce320
06-25 17:34:47.260 29263 29263 F DEBUG   :     sp  00000073900ce260  lr  0000007485e8d0c4  pc  0000007485e8d0f0
06-25 17:34:47.262 29263 29263 F DEBUG   : 
06-25 17:34:47.262 29263 29263 F DEBUG   : backtrace:
06-25 17:34:47.262 29263 29263 F DEBUG   :     NOTE: Function names and BuildId information is missing for some frames due
06-25 17:34:47.262 29263 29263 F DEBUG   :     NOTE: to unreadable libraries. For unwinds of apps, only shared libraries
06-25 17:34:47.262 29263 29263 F DEBUG   :     NOTE: found under the lib/ directory are readable.
06-25 17:34:47.262 29263 29263 F DEBUG   :       #00 pc 00000000000830f0  /apex/com.android.runtime/lib64/bionic/libc.so (abort+160) (BuildId: a6a4a6a4e20240bbe3173fe560b161af)
06-25 17:34:47.262 29263 29263 F DEBUG   :       #01 pc 00000000004b977c  /apex/com.android.runtime/lib64/libart.so (art::Runtime::Abort(char const*)+2280) (BuildId: a7b90c4876fad384278d03bbb11492c6)
06-25 17:34:47.262 29263 29263 F DEBUG   :       #02 pc 000000000000b458  /system/lib64/libbase.so (android::base::LogMessage::~LogMessage()+580) (BuildId: 1efae6b19d52bd307d764e26e1d0e2c9)
06-25 17:34:47.262 29263 29263 F DEBUG   :       #03 pc 000000000037825c  /apex/com.android.runtime/lib64/libart.so (art::JavaVMExt::JniAbort(char const*, char const*)+1584) (BuildId: a7b90c4876fad384278d03bbb11492c6)
06-25 17:34:47.262 29263 29263 F DEBUG   :       #04 pc 000000000037856c  /apex/com.android.runtime/lib64/libart.so (art::JavaVMExt::JniAbortF(char const*, char const*, ...)+176) (BuildId: a7b90c4876fad384278d03bbb11492c6)
06-25 17:34:47.262 29263 29263 F DEBUG   :       #05 pc 00000000004ff474  /apex/com.android.runtime/lib64/libart.so (art::Thread::DecodeJObject(_jobject*) const+680) (BuildId: a7b90c4876fad384278d03bbb11492c6)
06-25 17:34:47.262 29263 29263 F DEBUG   :       #06 pc 0000000000369db4  /apex/com.android.runtime/lib64/libart.so (art::(anonymous namespace)::ScopedCheck::CheckInstance(art::ScopedObjectAccess&, art::(anonymous namespace)::ScopedCheck::InstanceKind, _jobject*, bool)+96) (BuildId: a7b90c4876fad384278d03bbb11492c6)
06-25 17:34:47.262 29263 29263 F DEBUG   :       #07 pc 0000000000369148  /apex/com.android.runtime/lib64/libart.so (art::(anonymous namespace)::ScopedCheck::CheckPossibleHeapValue(art::ScopedObjectAccess&, char, art::(anonymous namespace)::JniValueType)+580) (BuildId: a7b90c4876fad384278d03bbb11492c6)
06-25 17:34:47.262 29263 29263 F DEBUG   :       #08 pc 0000000000368764  /apex/com.android.runtime/lib64/libart.so (art::(anonymous namespace)::ScopedCheck::Check(art::ScopedObjectAccess&, bool, char const*, art::(anonymous namespace)::JniValueType*)+652) (BuildId: a7b90c4876fad384278d03bbb11492c6)
06-25 17:34:47.262 29263 29263 F DEBUG   :       #09 pc 000000000036b548  /apex/com.android.runtime/lib64/libart.so (art::(anonymous namespace)::CheckJNI::DeleteRef(char const*, _JNIEnv*, _jobject*, art::IndirectRefKind)+672) (BuildId: a7b90c4876fad384278d03bbb11492c6)
06-25 17:34:47.262 29263 29263 F DEBUG   :       #10 pc 0000000000028b84  /data/data/com.company.package/files/app/_python_bundle/site-packages/jnius/jnius.so

Only difference with a working application is the pyjnius version (I'm now back to master and all is well except for the issue mentioned earlier).

@cmacdonald
Copy link
Contributor

cmacdonald commented Jun 25, 2020

Thanks. I'm not an Android person, so testing on Android is a problem for me. Is there more of the stacktrace that can show where in Jnius it failed? You said it fails "on launch" - what was the python code?

@lerela
Copy link

lerela commented Jun 26, 2020

There's also a stack trace of all threads but nothing relates to pyjnius and there is unfortunately no more information concerning the exact exception location.

Unfortunately that is a pretty big application which makes a few pyjnius references and I cannot share its source code, but it's failing before the app has even loaded (there are some globals defined with pyjnius as well so just loading the Python file could trigger the issue?).

As you can see from the log this is happening right after "184 symbols loaded" which is still deep inside Kivy's initialization routines. A normal log looks like this:

06-25 17:51:05.856 31234 31291 I python  : [INFO   ] [Logger      ] Record log in /data/user/0/com.company.package/files/app/.kivy/logs/kivy_20-06-25_0.txt
06-25 17:51:05.857 31234 31291 I python  : [INFO   ] [Kivy        ] v1.11.1
06-25 17:51:05.857 31234 31291 I python  : [INFO   ] [Kivy        ] Installed at "/data/user/0/com.company.package/files/app/_python_bundle/site-packages/kivy/__init__.pyc"
06-25 17:51:05.857 31234 31291 I python  : [INFO   ] [Python      ] v3.8.1 (default, Jun 25 2020, 15:08:54) 
06-25 17:51:05.857 31234 31291 I python  : [Clang 8.0.2 (https://android.googlesource.com/toolchain/clang 40173bab62ec7462
06-25 17:51:05.857 31234 31291 I python  : [INFO   ] [Python      ] Interpreter at ""
06-25 17:51:05.859 31234 31291 I python  : [INFO   ] [Factory     ] 184 symbols loaded
06-25 17:51:06.077 31234 31291 I python  : [INFO   ] [Image       ] Providers: img_tex, img_dds, img_sdl2, img_gif (img_pil, img_ffpyplayer ignored)
06-25 17:51:06.136 31234 31291 I python  : [INFO   ] [Window      ] Provider: sdl2
06-25 17:51:06.150 31234 31291 E libEGL  : validate_display:91 error 3008 (EGL_BAD_DISPLAY)
06-25 17:51:06.150 31234 31291 V SDL     : setOrientation() orientation=-1 width=800 height=600 resizable=true hint=
06-25 17:51:06.153 31234 31291 I python  : [INFO   ] [GL          ] Using the "OpenGL ES 2" graphics system
06-25 17:51:06.154 31234 31291 I python  : [INFO   ] [GL          ] Backend used <sdl2>
06-25 17:51:06.154 31234 31291 I python  : [INFO   ] [GL          ] OpenGL version <b'OpenGL ES 3.2 [email protected] (GIT@35556ba, I9ca166462c, 1565208506) (Date:08/07/19)'>
06-25 17:51:06.154 31234 31291 I python  : [INFO   ] [GL          ] OpenGL vendor <b'Qualcomm'>
06-25 17:51:06.154 31234 31291 I python  : [INFO   ] [GL          ] OpenGL renderer <b'Adreno (TM) 630'>
06-25 17:51:06.155 31234 31291 I python  : [INFO   ] [GL          ] OpenGL parsed version: 3, 2
06-25 17:51:06.155 31234 31291 I python  : [INFO   ] [GL          ] Texture max size <16384>
06-25 17:51:06.155 31234 31291 I python  : [INFO   ] [GL          ] Texture max units <16>

See that no application code has ran yet at the point where it crashed with your branch. Sorry this is not very helpful!

@hx2A
Copy link
Member

hx2A commented Jul 15, 2020

I did some testing on this branch with my project and everything looked OK. This branch lacks the pass_by_reference=False kwarg we added a while ago, which caused some problems for me at first, but once I took that out everything seemed to work correctly.

Is there anything I can do to help move this along?

@cmacdonald
Copy link
Contributor

I can rebase this week. Could you review some of the other outstanding PRs Jim?

@hx2A
Copy link
Member

hx2A commented Jul 16, 2020

@cmacdonald I will do that

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.

5 participants