On Fri, 4 Dec 2020 20:12:11 GMT, Chris Plummer <[email protected]> wrote:
>> This PR replaces the withdrawn PR #1348. This PR tries to fix the underlying >> problem, rather than fix the tests. >> >> The problem is that a number of JDI tests create objects on the debugger >> side with calls to `newInstance()`. However, on the debugee side, these new >> instances will only be held on to by a `JNIGlobalWeakRef`, which means they >> could be collected at any time, even before `newInstace()` returns. A number >> of JDI tests get spurious `ObjectCollectedException` thrown at them, which >> results in test failures. To make these objects stick around, a call to >> `disableCollection()` is typically needed. >> >> However, as pointer out by @plummercj in >> [JDK-8255987](https://bugs.openjdk.java.net/browse/JDK-8255987): >> >>> Going back to the spec, ObjectReference.disableCollection() says: >>> >>> "By default all ObjectReference values returned by JDI may be collected at >>> any time the target VM is running" >>> >>> and >>> >>> "Note that while the target VM is suspended, no garbage collection will >>> occur because all threads are suspended." >>> >>> But no where does is say what is meant by the VM running or being >>> suspended, or how to get it in that state. One might assume that this ties >>> in with VirtualMachine.suspend(), but it says: >>> >>> "Suspends the execution of the application running in this virtual machine. >>> All threads currently running will be suspended." >>> >>> No mention of suspending the VM, but that certainly seems to be what is >>> implied by the method name and also by the loose wording in >>> disableCollection(). >> >> Most of our spuriously failing tests do actually make a call to >> `VirtualMachine.suspend()`, presumably to prevent objects from being garbage >> collected. However, the current implementation of `VirtualMachine.suspend()` >> will only suspend all Java threads. That is not enough to prevent objects >> from being garbage collected. The GC can basically run at any time, and >> there is no relation to whether all Java threads are suspended or not. >> >> However, as suggested by @plummercj, we could emulate the behaviour implied >> by the spec by letting a call to `VirtualMachine.suspend()` also convert all >> existing JDI objects references to be backed by a (strong) `JNIGlobalRef` >> rather than a (weak) `JNIGlobalWeakRef`. That will not prevent the GC from >> running, but it will prevent any object visible to a JDI client from being >> garbage collected. Of course, a call to `VirtualMachine.resume()` would >> convert all references back to being weak again. >> >> This patch introduces the needed functions in `libjdwp` to "pin" and "unpin" >> all objects. These new functions are then used by the underpinnings of >> `VirtualMachine.suspend()` and `VirtualMachine.resume()` to implement the >> behaviour described above. >> >> Note that there are still a few tests that needed adjustments to guard >> against `ObjectCollectionException`. These are: >> - *vmTestbase/nsk/jdi/ArrayType/newInstance/newinstance004.java* - This >> test seems to have been forgotten by >> [JDK-8203174](https://bugs.openjdk.java.net/browse/JDK-8203174), which did a >> similar fix in the other `ArrayType/newinstance` tests. >> - >> *vmTestbase/nsk/jdi/VMOutOfMemoryException/VMOutOfMemoryException001/VMOutOfMemoryException001.java* >> - We just want to allocate as much as we can, so catching an ignoring >> `ObjectCollectedException` seems reasonable here. >> - *vmTestbase/nsk/share/jdi/sde/SDEDebuggee.java* - We still want to >> prevent `TestClassLoader` from being unloaded to avoid invalidating code >> locations. >> - >> *vmTestbase/nsk/jdi/ReferenceType/instances/instances002/instances002.java* >> - This test keeps the VM suspended, and then expects objects to be garbage >> collected, which they now won't. >> >> Testing: >> - More than 50 iterations of the `vmTestbase/nsk/jdi` and >> `vmTestbase/nsk/jdwp` test suites, using various GC, both in mach5 and >> locally. > > test/hotspot/jtreg/vmTestbase/nsk/jdi/ReferenceType/instances/instances002/instances002.java > line 194: > >> 192: debuggee.resume(); >> 193: checkDebugeeAnswer_instances(className, >> baseInstances); >> 194: debuggee.suspend(); > > Before the changes in this PR, what was triggering the (expected) collection > of the objects? @plummercj Nothing was explicitly triggering collection of these objects. However, the test is explicitly checking the number of objects "reachable for the purposes of garbage collection" in `checkDebugeeAnswer_instances()`. The tests sets up a breakpoint (with SUSPEND_ALL), which suspends the VM. Then it creates a number of new instances and expects these to be weakly reachable. However, with this change, suspending the VM will make all objects "reachable for the purposes of garbage collection". So, to let the test continue to create objects which are weakly reachable we need to first resume the VM, create the new instances, and then suspend it again. @dholmes-ora I have no idea why these tests are so different. The VM suspend is implicit in the breakpoint in this test, which is set up using SUSPEND_ALL. > test/hotspot/jtreg/vmTestbase/nsk/jdi/VMOutOfMemoryException/VMOutOfMemoryException001/VMOutOfMemoryException001.java > line 85: > >> 83: array.disableCollection(); >> 84: } catch (ObjectCollectedException e) { >> 85: continue; > > Maybe add a comment: "Since the VM is not suspended, the object may have been > collected before disableCollection() could be called on it. Just ignore and > continue doing allocations until we run out of memory." Sounds good, will fix. ------------- PR: https://git.openjdk.java.net/jdk/pull/1595
