On Tue, 7 Dec 2021 06:23:22 GMT, Peter Levart <plev...@openjdk.org> wrote:
>> Roman Kennke has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix cast; add testcase to cover memory pressure
>
> test/jdk/java/io/ObjectStreamClass/TestOSCClassLoaderLeak.java line 84:
>
>> 82:
>> 83:
>> assertNotNull(ObjectStreamClass.lookup(TestClass.class).getFields());
>> 84: }
>
> I don't quite get this test. It loads ObjectStreamClass_MemoryLeakExample
> class from child class loader, constructs an instance from it and calls
> .toString() on an instance. This is just to indicate that the class
> initializer of that class did lookup an ObjectStreamClass instance for Test
> class loaded by the same child loader. OK so far...
> Then there is this loop that tries to exhibit some memory pressure while
> constantly looking up OSC for another Test class (this time loaded by parent
> class loader) presumably to trigger clearing the SoftReference(s) of both
> classes loaded by child ClassLoader.... Is this what the loop was supposed to
> do?
> And finally there is an assertNotNull that does another lookup for OSC of
> Test class loaded by parent class loader, retrive its fields and check that
> the returned OSC instance as well as the field array are not null. This will
> always succeed regardless of what you do before the assertion.
>
> I don't think you need any custom class loading to verify the correctness of
> caching. The following two tests pass on old implementation of OSC. Do they
> pass on the new one too?
>
>
> public class ObjectStreamClassCaching {
>
> @Test
> public void testCachingEffectiveness() throws Exception {
> var ref = lookupObjectStreamClass(TestClass.class);
> System.gc();
> Thread.sleep(100L);
> // to trigger any ReferenceQueue processing...
> lookupObjectStreamClass(AnotherTestClass.class);
> Assertions.assertFalse(ref.refersTo(null),
> "Cache lost entry although memory was not
> under pressure");
> }
>
> @Test
> public void testCacheReleaseUnderMemoryPressure() throws Exception {
> var ref = lookupObjectStreamClass(TestClass.class);
> pressMemoryHard(ref);
> System.gc();
> Thread.sleep(100L);
> Assertions.assertTrue(ref.refersTo(null),
> "Cache still has entry although memory was
> pressed hard");
> }
>
> // separate method so that the looked-up ObjectStreamClass is not kept on
> stack
> private static WeakReference<?> lookupObjectStreamClass(Class<?> cl) {
> return new WeakReference<>(ObjectStreamClass.lookup(cl));
> }
>
> private static void pressMemoryHard(Reference<?> ref) {
> try {
> var list = new ArrayList<>();
> while (!ref.refersTo(null)) {
> list.add(new byte[1024 * 1024 * 64]); // 64 MiB chunks
> }
> } catch (OutOfMemoryError e) {
> // release
> }
> }
> }
>
> class TestClass implements Serializable {
> }
>
> class AnotherTestClass implements Serializable {
> }
The test was a rather crude (but successful) attempt to demonstrate the
ClassCastException. Thanks for providing the better testcase. I verified that
it succeeds with this PR, and also demonstrates the ClassCastException if I
revert my previous change in ClassCache. I pushed this new test, and removed my
old one.
-------------
PR: https://git.openjdk.java.net/jdk/pull/6375
