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
