ConstructAccess shoud support non-static inner classes instantiation

[ghost]

From serverpe...@gmail.com on June 13, 2012 15:47:40

Look at the attachment with my proposal for adding a "T newInstance (Object enclosingInstance)" method, allowing inner classes instantiation :-)

So caller could do:

if (constructorAccess.isNonStaticMemberClass()) {
    newInstance = constructorAccess.newInstance(enclosingInstance);
}
else {
    newInstance = constructorAccess.newInstance();
}

Regards!

Attachment: ConstructorAccess.java

Original issue: http://code.google.com/p/reflectasm/issues/detail?id=8

[ghost]

From nathan.s...@gmail.com on June 13, 2012 22:20:03

This issue was closed by revision r36.

Status: Fixed

[ghost]

From nathan.s...@gmail.com on June 13, 2012 22:20:07

Thanks!!

[ghost]

From serverpe...@gmail.com on June 18, 2012 05:19:47

Oh, I'm afraid that in r41 this improvement has been removed, just a human error replacing with an older version? or did you detect any defect / incompatibility?

Thanks

[ghost]

From nathan.s...@gmail.com on June 18, 2012 05:23:17

No, I meant to reopen this issue. The code caused a Kryo test to fail. I wanted to do a Kryo release ASAP, so I rolled it back for now. 5AM so I can't look at it zzz. The test was FieldSerializerTest, testInstantiatorStrategy, something to do with HasArgumentConstructor, ah managed to save a result:

testInstantiatorStrategy(com.esotericsoftware.kryo.FieldSerializerTest):
com.esotericsoftware.kryo.FieldSerializerTest$HasArgumentConstructor:
method ()V not found

Status: Accepted

[ghost]

From serverpe...@gmail.com on June 18, 2012 09:42:44

Hi Nathan,

I think I have the solution to this issue and also to java.lang.LinkageError reported by mongonix (https://groups.google.com/forum/?fromgroups#!topic/kryo-users/gv9LE9vsEN4). Sorry for mix all this in this same comment:

1. Resolution for the race condition / LinkageError in AccessClassLoader ("java.lang.LinkageError: loader (instance of com/esotericsoftware/reflectasm/Acc
   essClassLoader): attempted duplicate class definition for name ..."):
   • I'm pretty sure that this bug has arisen when we fixed the AccessClassLoader cache. Now that different XXXAccess can actually share the same ClassLoader, two different threads could define the same accessor class at the same time.
   • See the AccessClassLoader version attached and its comments. Other solutions could by coarse-grained synchronization or even undo the AccessClassLoader cache.
     
2. Resolution for the incompatibility between the ConstructorAccess in r36 and Kryo:
   • The incompatibility was because I removed the "throw new RuntimeException(...)".
   • See the attached ConstructorAccess. Your Kryo test cases work ok...
   • And also this gives Kryo automatic compatibility with (non-static) inner classes instantiation as long as they don't use member variables or methods from tue enclosing instance. For exampe, if you remove the static modifier from all the nested classes inside FieldSerializerTest (except the nested Serializer), the Test Unit still work.
   • But in order to make Kryo full compatible with inner classes you should overload Kryo.newInstance to allow passing the enclosing instance when instantiating inner classes.
   • As you can see, I have also changed FieldAccess in order to skip synthetic fields (this$1, this$2...) in inner classes. Not sure if this is really neccessary at least to avoid a waste of time in Kryo scanning object graphs), but this adition is harmless nevertheless (I think so).
     
     P.S: mongonix reported an java.lang.OutOfMemoryError in the same thread.
     
     R,
     Jesús

Attachment: AccessClassLoader.java ConstructorAccess.java FieldAccess.java

[ghost]

From nathan.s...@gmail.com on June 18, 2012 17:28:39

This issue was closed by revision r45.

Status: Fixed

[ghost]

From nathan.s...@gmail.com on June 18, 2012 17:28:47

1. I think we can synchronize in get() for the access classes so we don't have to catch the LinkageError.

2a) I think creating an inner class without an enclosing instance should be an error. Likewise, creating a new instance with an enclosing instance of a non-inner class should be an error. It's neat to be able to create an inner class instance with a null enclosing instance, but rarely not useful since this isn't possible thru Java-the-language.

2b) Kryo avoids synthetic fields, tho has a setting to use them (someone wanted it). I think it is ok for ReflectASM to make them available, in case someone wants to access them (I personally can't think of a good reason but meh). Kryo doesn't iterate over the fields reported by ReflectASM anyway, since ReflectASM has to be optional for use on Android where we can't do bytecode generation.

Can you take a look at what I checked in?

[ghost]

From serverpe...@gmail.com on June 19, 2012 00:38:16

Your checked-in version is working perfectily for me in all cases. And the synchronized regions and monitor make perfect sense to me.

One question: do you plan to support inner classes in Kryo somehow?

PS: Maybe you shoul also think about this "esoteric" possibility ;) : what if an application has already defined classes named XXXConstructorAccess or XXXFieldAccess?

[ghost]

From nathan.s...@gmail.com on June 19, 2012 00:43:53

Inner classes in Kryo, probably not. Well, it can be done currently by using Objenesis to create classes without calling their constructor, and enabling synthetic fields for FieldSerializer. Or you could do the construction yourself with Java's funky syntax "enclosingInstance.new InnerClassType()". But I don't think this need is very common.

If they had classes with those names, I think it would work fine. AccessClassLoader tries to load the class in the same loader, but if that fails it will load the class in the AccessClassLoader, which shouldn't have any user classes in it. The user classes come from the parent loader.