search

odnoklassniki / one-nio

Unconventional I/O library for Java
Apache License 2.0
655 stars 97 forks source link

Emit checkcast to avoid crash on fastdebug vm #52

Closed genuss closed 4 years ago

genuss commented 4 years ago

At first I must say that there was an unrelated bug I inspected in VM but also found this one. If you run this minimal example on fastdebug vm (any vm will go, not just jdk8u232) you will get a crash like this. As far as I understand this has no impact on release VM and will never happen there. However I suppose this is important to fix to reduce amount of VM crashes you get when you debug it.

apangin commented 4 years ago

Thank you for the report! This is indeed an issue that needs to be fixed. However, the suggested fix looks incomplete and not very optimal.

  1. generateToJson also seems to miss checkcast;
  2. the fix does not work for inlined serializers (where fd.parentField() != null);
  3. if the target class has writeObject method, checkcast may be also required before calling the method;
  4. there is no need to invoke checkcast every time before writing each individual field (especially with different declared class). Instread, it might be a good idea to cast the argument to the target class just once at the method entrance, and update the local variable 1 to reuse it throughout the code.

I'll be glad to accept the fix with the above comments addressed. Would you like to update the PR yourself, or let me do it? Both options are fine.

genuss commented 4 years ago

I would try to implement fixes you mentioned maybe with your help. Could you clarify what is an inlined serializer and how could I create it to be able to test? Unfortunately there is a lack of tests for this generated code, so I couldn't replicate these issues you mentioned. I'l try to add them.

apangin commented 4 years ago

The classes marked with INLINE option are serialized as a part of the outer instance with no extra reference (i.e. the fields of the inline class are "embedded" directly into the parent). Currently there are two standard classes serialized with INLINE option: https://github.com/odnoklassniki/one-nio/blob/1361da62da1630f6263e4ad0a8ff74beffb8c9ad/src/one/nio/serial/Repository.java#L152-L155

Anyway, casting the passed Object to the target class right at the beginning of the method, should solve all of the mentioned issues.

genuss commented 4 years ago

Made it work with local variable (squashed with previous commit). Please review.

apangin commented 4 years ago

Yep, looks fine. I also think there is no need to introduce new local variable, we can just reuse slot 1. Have you verified that JVM no longer crashes with this fix, and all unit tests pass?

genuss commented 4 years ago

Removed local variable. Actually didn't know it's possible because you can't express such bytecode in plain java code. Yes I checked that on fastdebug build and it's no longer crashes. Unit tests pass however this test fails. https://github.com/odnoklassniki/one-nio/blob/1361da62da1630f6263e4ad0a8ff74beffb8c9ad/test/one/nio/net/SocketTest.java#L128-L130 I suppose that's because IP_TOS is not supported on windows, but it's unrelated.

apangin commented 4 years ago

Thank you! The fix went into master.