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

fix(#1156): illegal reflective access on java 9 #1180

Merged
merged 2 commits into from
Feb 1, 2018

Conversation

loicmathieu
Copy link
Contributor

@loicmathieu loicmathieu commented Jan 30, 2018

Hi,

This pull request allow to workaround the java 9 illegal access warning generated by mybatis.

When mybatis search for existing methods on objects, it goes to the object hierarchy including the top java.lang.Object class.

As there is no point to go to the java.lang.Object class as it didn't contains any getter/setter, a trivial workaround is to skip this class.
I tested this approach and with this patch there is no more illegall access warning on Java 9.

Regards,

Loïc

#1156

@harawata
Copy link
Member

Thank you, @loicmathieu !

But, this is not the right fix.
Even after applied the patch, I see similar warnings when running BaseTest.

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by org.apache.ibatis.reflection.Reflector (file:/..../mybatis-3/target/classes/) to method java.lang.Class.checkPackageAccess(java.lang.SecurityManager,java.lang.ClassLoader,boolean)
WARNING: Please consider reporting this to the maintainers of org.apache.ibatis.reflection.Reflector
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release

The real cause is the call to setAccessible() method, but simply removing the calls will break backward compatibility.
We haven't discussed this yet, but netty, for example, added a new option as a workaround.

@loicmathieu
Copy link
Contributor Author

Hello,

Yes, my patch is not a correction of the issue but a workaround that works on my use case, when you only use mybatis on your own object with not private method (why is a setter or a getter private anyway), there is no issue as the setAccessible Method is not used.

I don't use mapper annotation, and apparently the illegall access warnings on BaseTest are due to the mapper annotation processing, I didn't have time to check this now. But if you launch the test with --illegal-access=warn with and without the patch you should see that the fixe corrected some warnings.

The netty way is to don't do setAccessible (this method should not have been used on the first place) on java 9 and allow to force it via a system property. This solution will break existing applications on Java 9 and they will be able to optin via a system property to works again.
Alternative is to use VarHandles but this is a hard path and will need to compile against java 9.

I agree that a more complete solution should be implemented, anyway, my fix will improve the situation as it will fix the issue in some cases (maybe a majority of cases) and it will also improve starting performance a little as it will avoid some reflection calls.

Regards,

Loïc

@harawata
Copy link
Member

Well, the change itself is good as an optimization, but I'm not comfortable with treating this as a workaround for the warnings.
Would you consider removing the comment line?
Then I have no problem with merging this.

@loicmathieu
Copy link
Contributor Author

Hello,
I remove the comment.
Regards,
Loïc

@harawata harawata merged commit 9a19aed into mybatis:master Feb 1, 2018
@harawata
Copy link
Member

harawata commented Feb 1, 2018

Thank you, @loicmathieu !

@kazuki43zoo kazuki43zoo added the enhancement Improve a feature or add a new feature label Feb 1, 2018
@kazuki43zoo kazuki43zoo added this to the 3.4.6 milestone Feb 1, 2018
@loicmathieu loicmathieu deleted the fix_1156 branch February 2, 2018 08:15
pulllock pushed a commit to pulllock/mybatis-3 that referenced this pull request Oct 19, 2023
As a side effect, this might reduce the chance of 'illegal reflective access' warnings on Java 9. mybatis#1156
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.

3 participants