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

Avoid unnecessary setAccessible() calls. #1321

Merged
merged 1 commit into from
Oct 14, 2018

Conversation

harawata
Copy link
Member

@harawata harawata commented Aug 1, 2018

As a fix for #1156 .

I used retry-on-failure approach as isAccessible() is useless (always returns false on the first call).
It would be appreciated if someone can test this with named modules.

@harawata harawata added the enhancement Improve a feature or add a new feature label Aug 1, 2018
@harawata harawata requested a review from kazuki43zoo August 1, 2018 16:02
@dawwin
Copy link
Contributor

dawwin commented Aug 2, 2018

I have tested your changes on JDK10 (without named modules) and looks like this change solves problem with "Illegal reflective access" warnings. I'll try to test it with named modules if I find time to do it :).

I have one question though. You have moved calls to setAccessible() to Invoker implementations. Isn't it now possible to concurrently call setAccessible() on the same Field object from different threads?

@harawata
Copy link
Member Author

harawata commented Aug 3, 2018

@dawwin ,

Thank you very much for testing!

Regarding setAccessible(), it is indeed possible that multiple threads call the method concurrently.
As those Invokers always pass true to setAccessible(), I think it's fine, but please let me know in case I am missing something.

@dawwin
Copy link
Contributor

dawwin commented Sep 9, 2018

I did some tests with modules and it seems that this change works fine with JDK10. Here is the example app I used for testing: https://github.com/dawwin/mybatis.jdk10test .
Model classes have to be exported to mybatis if all classes/getters/setters are public. If there are some not public fields - for example setters, then opens has to be used, otherwise it will fail (See module-info.java in repository I mentioned). So it seems to be ok.

I found one unrelated issue with JDK10, but I will report it separately

@harawata
Copy link
Member Author

Hi @dawwin ,
Thank you so much for the example app! I haven't had time to try it yet, but I sure will.
The behavior you described matches my expectation, so this seems to be the right approach. :)

@harawata
Copy link
Member Author

I am going to merge this so that users can test it with snapshot.
I'll revert it if there is any unfixable issue.

@harawata harawata merged commit fd3b1fa into mybatis:master Oct 14, 2018
pulllock pushed a commit to pulllock/mybatis-3 that referenced this pull request Oct 19, 2023
…-access

Avoid unnecessary `setAccessible()` calls.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve a feature or add a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants