RFR: 8333796: Add missing serialization functionality to sun.reflect.ReflectionFactory [v4]

[David M. Lloyd]

On Tue, 16 Jul 2024 18:17:00 GMT, Roger Riggs <rriggs at openjdk.org> wrote:

>> David M. Lloyd has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Address review feedback
>
> src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line 491:
> 
>> 489:             return 0;
>> 490:         }
>> 491:     }
> 
> How is this different than `ObjectStreamClass.lookup(clazz).getSerialVersionUID()` ?
> 
> And BTW, this misses the computation of the default SVUID and the SVUID of Records.

The `ObjectStreamClass` computes a default value if there is no field, so serialization frameworks do not have any way to know whether a default value or explicitly specified value is in use. That said - I'm looking over the custom serializer code that we have and I think we switched to using `ObjectStreamClass` a while ago so I can just remove this whole method.

> src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java line 505:
> 
>> 503:     );
>> 504: 
>> 505:     private static boolean isValidSerializable(Class<?> cl) {
> 
> "Valid" is too broad to clearly identify the purpose. 
> You might flip it to return true for all classes that use the default serialized object format.
> 
> You might find that using the pattern language feature and "instanceof" would be effective and not need the small set.

Sure, I can rename this and invert it.

I haven't used the pattern feature much so far, but after reading JEP 441 again and looking for other examples, while I see how an object can be matched with `instanceof`, I can't figure out a way to use it to switch on an actual `Class` object in this way. Am I missing something?

> test/jdk/sun/reflect/ReflectionFactory/ReflectionFactoryTest.java line 379:
> 
>> 377:         @SuppressWarnings("removal")
>> 378:         ObjectOutputStream oos = AccessController.doPrivileged((PrivilegedExceptionAction<ObjectOutputStream>) () -> new ObjectOutputStream() {
>> 379:             protected void writeObjectOverride(final Object obj) throws IOException {
> 
> The alternate invocation of the replacement defaultWriteObject should be hooked in here, to see how the whole mechanism works smoothly. Especially for nested serializations.

So far I'm unit testing each method to show that it works to its own specification; in order to fully integrate with multiple classes and nested objects, it seems as though I'd have to implement an entire custom serialization framework for the test, which does not seem feasible; the generated method does not relate in any way to nested serializations (that would be solely the job of the framework which uses it).

The only difference I can see between directly using the generated `default(Read|Write)Object` and one which uses them indirectly by way of a `Ser#(read|write)Object()` calling through `Object*Stream.default(Read|Write)Object()` is the call path through the test classes and mocks. But, I could possibly add a couple more tests with another pair of slightly different mocked streams which "tracks" the "current" object (of which there would just be one) and makes sure that `default(Read|Write)Object` also calls through the handle, just to show that it works, though I don't think this adds any surety or even code coverage to the overall tests since all of the additional code would exist only in the unit test (i.e. I'd be writing a test that proves that the test works, not that the feature works).

I do believe that this feature is fully covered by the existing tests, but if I am missing some subtle detail we can discuss further of course.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1769274124
PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1769278913
PR Review Comment: https://git.openjdk.org/jdk/pull/19702#discussion_r1769300945