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

[Roger Riggs]

Hi Chris,

On 10/21/2016 6:36 AM, Chris Hegarty wrote:
> 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.
removed initializer
>
>  2) hasStaticInitializerForSerialization() can have its setAccessible
>     call moved to just before the invoke.
The method is cached in hasStaticInitializerMethod and does not need to be
set every time the method is called.  As is, setAccessible is only 
called once.
There's no need to repeat the security checks on every invocation.

>
>  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.
ok
>
>  4) s.m.RF, Return -> Returns
Right, fixed correctly

Thanks, Roger

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