RFR: 8275338: Add JFR events for notable serialization situations [v4]

Tue Dec 19 16:03:55 UTC 2023

On Tue, 19 Dec 2023 14:39:47 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

> Is it per class for each classloader that loads it? Or is it per class per JVM? It's more out of curiosity than anything else because I don't think it makes a big difference (I don't expect too many classloaders that would lead to the case of extremely large streams of events).

The checks are done on the `Class<?>` instance, that is, per each defined (as per JVMS) and _used_ serializable class, on first usage in serialization. If enabled at all, they are invoked by the private `ObjectStreamClass` constructor.

> src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java line 94:
> 
>> 92:         if (!isPrivate(f)) {
>> 93:             commitEvent(SUID_PRIVATE,
>> 94:                     SUID_NAME + " should be declared private");
> 
> Rest of the event messages use "... to be effective", should this one too use a similar text?

The field needs not be declared `private` to be effective, although it is recommended to do so. In this sense, "should" is just a recommendation, while "must" is really needed to make something effective.

> src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java line 108:
> 
>> 106:                     SUID_NAME + " should be declared of type long");
>> 107:         }
>> 108:         if (!isStatic(f)) {
> 
> Nit - perhaps save the return value from the previous call to `isStatic(f)` a few lines above and reuse it here?

Assuming that most serializable classes are declared correctly, I don't think that caching the value of `isStatic()` makes a measurable difference.
If at all, then I'd prefer to cache the modifiers and "inline" the masking logic of the individual is*() methods.

> src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java line 112:
> 
>> 110:         }
>> 111:         f.setAccessible(true);
>> 112:         if (getLong(f) == null) {
> 
> Is this check and the `setAccessible()` needed if a few lines above, the call to `f.getType()` returns `Long.TYPE` (i.e. primitive type)? Perhaps we can conditionally do these additional checks and calls, only if `f.getType()` isn't a primitive?

Right, will be addressed in the next commit.

> src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java line 185:
> 
>> 183:             commitEvent(PRIV_METH_NON_STATIC,
>> 184:                     m + " must be non-static to be effective");
>> 185:         }
> 
> Should there also be a check to see if this `private` method is (misdeclared) as a `native` method?

I'm not sure that a `native` method is not considered effective by serialization. I have to check.

> src/java.base/share/classes/java/io/SerializationMisdeclarationChecker.java line 276:
> 
>> 274:     }
>> 275: 
>> 276:     private static Object getObject(Field f) {
> 
> Should we call this method `getStaticFieldValue(...)`, because that's what the implementation is.

Since the only parameter is `Field`, it has to be a static field almost for sure.
Further, there's a `getLong()` method down below that operates on a static field as well and that one would also need a renaming.
If I can come up with better names they will be in the next commit.

> src/java.base/share/classes/jdk/internal/event/SerializationMisdeclarationEvent.java line 94:
> 
>> 92: 
>> 93:     /*
>> 94:      * These constants are not final on purpose.
> 
> Just curious - what's the purpose? I don't see them being changed/updated anywhere (not even in tests).

Declaring a `public static int` field `final` means that the value is then stored in the client class. If a value is changed, then the client needs to be recompiled as well, but this is not enforced by the language, so it might lead to inconsistencies.

The clean solution would be to use an enum class, but that's not supported by JFR.

> test/jdk/jdk/jfr/event/io/TestSerializationMisdeclarationEvent.java line 50:
> 
>> 48:  * @requires vm.hasJFR
>> 49:  * @library /test/lib
>> 50:  * @run junit/othervm jdk.jfr.event.io.TestSerializationMisdeclarationEvent
> 
> Is the use of "othervm" needed here because of the use of `jdk.jfr.consumer.RecordingStream`? Does `RecordingStream` do JVM wide state changes? I did a quick look at that class but couldn't come to a conclusion.

Not sure, I have to check.

> test/jdk/jdk/jfr/event/io/TestSerializationMisdeclarationEvent.java line 112:
> 
>> 110:     @MethodSource
>> 111:     public void testGoodClass(Class<?> cls) {
>> 112:         assertEquals(0, getEventsFor(cls).size());
> 
> Can this and other assertions in this test be updated to include the class name which failed? You can still use `assertEquals(...)`, it takes an optional message as a third parameter which you could use to include the failing class name. It becomes a bit more easier to debug (unexpected) failures when the assertion includes these additional details.

Thanks for the suggestion, will take a look.

> test/jdk/jdk/jfr/event/io/TestSerializationMisdeclarationEvent.java line 144:
> 
>> 142:     }
>> 143: 
>> 144:     static {
> 
> It's a bit odd for a test case to be doing this in a static block. Could this instead be done in a `org.junit.jupiter.api.BeforeAll` method:
> 
> 
> import org.junit.jupiter.api.BeforeAll;
> ...
> @BeforeAll
> static void recordEvents() {
>   .... // that static block's code here
> }
> 
> Any (unexpected) failures in that method will then be reported in a better way by the testing framework instead of an `ExceptionInInitializer` that would be reported from a failure in static block and the test class itself failing to load.

Yes, yours is a cleaner way to go. Will do.

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

PR Comment: https://git.openjdk.org/jdk/pull/17129#issuecomment-1863031458
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431613663
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431612831
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431613065
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431614036
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431615365
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431616847
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431618143
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431617706
PR Review Comment: https://git.openjdk.org/jdk/pull/17129#discussion_r1431617951