RFR 9: 8164908: ReflectionFactory support for IIOP and custom serialization

[Roger Riggs]

Hi,

Thanks for the comments..

Webrev's updated in place with comments so far.  (Including David's and 
Amy's)

http://cr.openjdk.java.net/~rriggs/webrev-reflection-factory-8164908/

http://cr.openjdk.java.net/~rriggs/webrev-reflection-factory-iiop-8164908/

On 10/20/2016 8:21 AM, Alan Bateman wrote:
> On 19/10/2016 20:59, Roger Riggs wrote:
>
>> The support in sun.reflect.ReflectionFactory for custom 
>> serialization, such as IIOP input
>> and output streams, is being expanded beyond the necessary 
>> constructor of a serializable
>> class to include access to the private methods readObject, 
>> writeObject, readResolve,
>> writeReplace, etc.
>>
>> The IIOP implementation is updated to use a combination of 
>> ReflectionFactory and
>> Unsafe to serialize and deserialize objects and no longer rely on 
>> setAccessible.
>> Tests are included for ReflectionFactory and the affected IIOP classes.
>>
>> Please review and comment,
>>
>> jdk repo webrev:
>> http://cr.openjdk.java.net/~rriggs/webrev-reflection-factory-8164908/
>> corba repo webrev :
>> http://cr.openjdk.java.net/~rriggs/webrev-reflection-factory-iiop-8164908/ 
>>
>>
> I skimmed through the changes.
>
> I assume findReadWriteObjectForSerialization should throw 
> InternalError, rather than return null, if IllegalAccessException is 
> thrown (as IAE is not possible here).
fixed
>
> You've added @since 9 to sun.reflect.ReflectionFactory but that class 
> is not new.
It was new in 9, but did not previously have @since. Its internal so it 
may not be important.
Added by 8137058: Clear out all non-Critical APIs from sun.reflect

>
> The javadoc for 
> sun.reflect.ReflectionFactory.newConstructorForSerialization doesn't 
> say that it returns null when the Class is not Serializable.
The current implementation does not check that the argument is Serializable
and there are tests for that.  I will update the documentation to not 
specify a Serializable class.
I would need to track down all the uses to understand that adding the 
check would not break something.

It also does not check that the requested constructor is for a supertype.
I think the semantics of newConstructorForSerialization should include 
the search for
the super-type's noarg constructor instead of IIOP doing that search.

>
> For the MH returning methods then I assume the javadoc should say that 
> it returns a direct method handle.
ok
>
> The synchronization in the IIOP ObjectStreamClass isn't very clear. 
> Are the invoke*, read*, write* methods all invoked by the same thread 
> that creates the ObjectStreamClass with the lookup method?
ObjectStreamClass instances are cached and re-used across all threads.
ObjectStreamClass.init() handles the synchronization of the initialization.
Any thread using the ObjectStreamClass will use the same method handle 
regardless of the
thread calling the method.

Thanks, Roger