-
Notifications
You must be signed in to change notification settings - Fork 66
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 InaccessibleObjectException on JDK16+ #73
Conversation
Hi @reta Can you keep the public constructors please? It is used by extensions/custom codes.
|
Hey @rmannibucau , thanks for taking a look:
Could drop it but replace with
Build with JDK16 or run test with JDK16, I could help with that on Jenkins fe.
This part is unclear to me in a sense, those are internal JDK classes, how would build time generation help? |
Overall null check is not needed, entry is only added if accessible. |
@rmannibucau comments addressed, the only regression I have seen so far across the test suite is this guy:
Essentially, with |
771498b
to
26b029c
Compare
The Travis build failed but issue seems to be unrelated:
|
I care more of a regression out of the test suite, guess running tck is an option. Test suite will get some priviledged access. If no regression, not sure the change is useful too so we should refine it i think. |
Hi @reta, had a quick look. Seems it is only an issue if in a not yet opened module so we should be able yo skip fields in this case no - and not have to catch it as a fallback. Wdyt? Can help on code side in 7days. |
It seems like a viable option as well, probably would need some reflection in order to use the APIs available in JDK9+, I could try it for sure. |
Running TCK would be good, the regressions should be unlikely (🤞 ), from the access checks I have run into, |
@reta tested with master (plain master, not patched) and it works well on java 16 so guess you went into a particular case where we try to do reflection on jdk classes (java.util.ArrayList.size it seems) which is something we can avoid with a "if known() then do something clever" earlier in introspection process so I think we don't need to do anything about setAccessible but rather we should fix the cause of this arraylist introspection (maybe hashmap too). Any way to write a reproducer test? edit: can be interesting to have the frame before |
@rmannibucau Yes, I do have a test but I am very surprised with your outcomes, I saw a number (but not many) of test failures. Just to double check, you used latest JDK-16 EA build, right? (the JEP in question was integrated ~ month ago). Could you also try JDK17-ea? Thanks. Edit: Just a number of different failures in
I will shortly extract the test for |
@@ -92,6 +94,14 @@ public void list() { | |||
assertEquals("[\"a\",\"b\"]", JsonbProvider.provider().create().build().toJson(map)); | |||
} | |||
|
|||
@Test | |||
public void listOfSimple() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rmannibucau Here is the test, consistently fails (for me) on the master (unpatched) on JDK16-ea+28 and above:
[INFO] -------------------------------------------------------
[INFO] T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.apache.johnzon.jsonb.JsonbWriteTest
[ERROR] Tests run: 12, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 0.27 s <<< FAILURE! - in org.apache.johnzon.jsonb.JsonbWriteTest
[ERROR] listOfSimple(org.apache.johnzon.jsonb.JsonbWriteTest) Time elapsed: 0.016 s <<< ERROR!
java.lang.reflect.InaccessibleObjectException: Unable to make field private int java.util.ArrayList.size accessible: module java.base does not "opens java.util" to unnamed module @6b71769e
at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:357)
at java.base/java.lang.reflect.AccessibleObject.checkCanSetAccessible(AccessibleObject.java:297)
at java.base/java.lang.reflect.Field.checkCanSetAccessible(Field.java:177)
at java.base/java.lang.reflect.Field.setAccessible(Field.java:171)
at org.apache.johnzon.mapper.access.FieldAccessMode$FieldDecoratedType.<init>(FieldAccessMode.java:105)
at org.apache.johnzon.mapper.access.FieldAccessMode$FieldReader.<init>(FieldAccessMode.java:169)
at org.apache.johnzon.mapper.access.FieldAccessMode.doFindReaders(FieldAccessMode.java:49)
at org.apache.johnzon.mapper.access.BaseAccessMode.findReaders(BaseAccessMode.java:82)
at org.apache.johnzon.mapper.access.FieldAndMethodAccessMode.doFindReaders(FieldAndMethodAccessMode.java:68)
at org.apache.johnzon.mapper.access.BaseAccessMode.findReaders(BaseAccessMode.java:82)
at org.apache.johnzon.jsonb.JsonbAccessMode.findReaders(JsonbAccessMode.java:468)
at org.apache.johnzon.mapper.Mappings.createClassMapping(Mappings.java:475)
at org.apache.johnzon.mapper.Mappings.doFindOrCreateClassMapping(Mappings.java:437)
at org.apache.johnzon.mapper.Mappings.findOrCreateClassMapping(Mappings.java:411)
at org.apache.johnzon.mapper.Mapper.isDeduplicateObjects(Mapper.java:215)
at org.apache.johnzon.mapper.Mapper.writeObjectWithGenerator(Mapper.java:209)
at org.apache.johnzon.mapper.Mapper.writeObject(Mapper.java:203)
at org.apache.johnzon.mapper.Mapper.writeObjectAsString(Mapper.java:252)
at org.apache.johnzon.jsonb.JohnzonJsonb.toJson(JohnzonJsonb.java:342)
at org.apache.johnzon.jsonb.JsonbWriteTest.listOfSimple(JsonbWriteTest.java:102)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, just read on the phone but our list detection is something along "is parameterized and raw type is a collection or is an array" so passing List.class was moving it to a fallback handling and lead to the unexpected stack you get.
Guess it is what we must fix and probably for maps too.
Hope it makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, in this particular case for ArrayList it could be certainly done, I run into it by accindent ;-). I am wondering if you were able to see the other failures, ReadPrimitiveTest
& CircularExceptionTest
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didnt run it but looks like it is the same issue of "too late type handling" so guess fix is to move it earlier trying to limit runtime impact.
If not urgent can work on it next week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rmannibucau no, it is not urgent, please let me know if I could help in scope of this PR (or I can just close it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep it open until we review the final solution and agree on it. Please ping me if you dont get news in 10 days ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @rmannibucau, I gave it a second thought, particularly when you mentioned the presence of the extensions and completely refactored the approach to eliminate flows driven by exceptions. I think it is more clean but let us discuss it when you have time, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @reta,
I like this version more but it still fixes the consequence instead of the source of the issue which is an unexpected branch in the mapper (class for collection/primitive/map/throwable/jdk-class) so even if this flavor can be kept I still think we should fix first the cause then maybe review this flavor.
Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rmannibucau This very correct, I will continue digging in so we could be proactive in introspection (we are indeed reactive now), will be back on that :) Thanks for the feedback.
4c82dd8
to
fbff720
Compare
…ssible and added accessibility property to readers and writers
fbff720
to
155558f
Compare
@reta can you review https://issues.apache.org/jira/browse/JOHNZON-331 and the related commit (15a4cc6). Does it solve your issue or do we need "more" - note that it can need some more love from cxf side is type is unexpectedly erased like using getType instead of getGenericType somewhere maybe? |
@rmannibucau for my issue with |
@reta question is: can it still happen in a valid setup (mapping a not opened/imported model is assumed wrong i guess). I didnt have more issues on java 16 so for me this exception is gone but if you see other cases we must fix them. Maybe exception case? Didnt check this last one but fix is likely to map it directly and not by reflection. Wdyt? |
@rmannibucau Mapping a not opened/imported model is certainly not advisable, probably the question would be: should |
Well my reasoning is:
So overall your impl could be a toggle but my feeling is that it shouldnt be the default if we want to support jpms correctly - but once again jpms can be dead today so maybe not a big deal. Side note: no logging is likely desired so failling or swallowing probably? Anyone else has an opinion on it? |
@rmannibucau thanks, let me just keep it open for a few days if someone has opinion on the matter, otherwise let us just close it as not needed, wdyt? |
Closing as not needed |
Since JEP-396 [1] integration into JDK16, the JDK internals become much less accessible.
Johnzon
usessetAccessible
in several places to relax the visibility rules but it now throwsInaccessibleObjectException
on JDK16 and above (for a number of JDK classes). For example, we run into following issues with Apache CXF while running JSON-B tests on JDK16/JDK17:The suggested fix in this PR is to ignore the properties which are not accessible anymore (basically when
setAccessible
ends up withInaccessibleObjectException
exception). Alternatively, it is still possible to use--add-opens java.base/java.util=ALL-UNNAMED
but arguably this not considered acceptable.[1] https://openjdk.java.net/jeps/396