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

[Chris Hegarty]

Roger,

On 20/10/16 18:57, Roger Riggs wrote:
> 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/

This looks very good Roger. A few very minor comments:

  1) The explicit initialization of hasStaticInitializerMethod to null
     is unnecessary.

  2) hasStaticInitializerForSerialization() can have its setAccessible
     call moved to just before the invoke.

  3) Remove the @since 9 from s.m.RF. This class exists on previous
     JDK's. 8137058 moved it originally and added a new file in its
     place to help preserve the history of the file.

  4) s.m.RF, Return -> Returns

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

No specific comments.

-Chris.

> 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
>
>