Hi Per,
On 3/12/2020 11:19 pm, Per Liden wrote:
This PR replaces the withdraw 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().
I think we can quite reasonably infer that "suspending a VM" means
calling VirtualMachine.suspend to suspend all the threads of the target VM.
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.
You can imagine though that 25 years ago it was not an unreasonable
assumption that GC only runs in response to (failed) allocation requests
from running application threads - i.e. that it is a synchronous
response to application code execution. Hence all threads suspended
implies no allocation and thus no GC. (Someone can correct me if I'm
wrong but way way back didn't running in JDI debug mode force use of
SerialGC?)
I'm somewhat surprised that it has taken this long to discover that our
GC's are no longer operating in a way that JDI requires them to.
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.
I assume that the GC folk would be horrified if I were to suggest a
global flag to enable/disable GC? ;-)
Doing what is suggested sounds reasonable, from a functional
perspective, to get the desired effect of not collecting any objects of
interest. But I do have to wonder how many objects we are typically
dealing with and what the performance impact of this might be if we have
to iterate through each all the objects?
Thanks,
David
-----
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.
-------------
Commit messages:
- 8255987: JDI tests fail with com.sun.jdi.ObjectCollectedException
Changes: https://git.openjdk.java.net/jdk/pull/1595/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1595&range=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8255987
Stats: 161 lines in 8 files changed: 132 ins; 0 del; 29 mod
Patch: https://git.openjdk.java.net/jdk/pull/1595.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/1595/head:pull/1595
PR: https://git.openjdk.java.net/jdk/pull/1595
