On Tue, 28 May 2024 23:20:48 GMT, Chris Plummer <[email protected]> wrote:
> Fixed "double-checked-locking" bug in `ReferenceImplType.classObject()`.
> Details in the first comment. Tested with the following:
> - com/sun/jdi
> - nsk/jdi
> - nsk/jdwp
> - nsk/jdb
The bug is the classic "double-checked-locking" flaw. In general
"double-checked-locking" does not work, but in this case, based on how it is
used and the java memory model, it can be made to work by making the field
volatile. Although the issue could be fixed by making the classObject field
volatile, I decided to get rid of the double-checked-locking instead. There is
little benefit to using it here (it simply avoids fetching the info twice when
there is a race, which is very rare), and leaving it out simplifies the code
and reduces overhead for the usual case (when there is no race). Regarding the
pre-existing comment:
// Are classObjects unique for an Object, or
// created each time? Is this spec'ed?
Yes, they are unique. No they are not created each time. Probably this lack of
understanding is why double-checked-locking was used here. The ObjectReference
spec is a bit lose on the wording, referring to an ObjectReference as "An
object that currently exists in the target VM". However, the implementation is
not. VirtualMachineImpl.objectMirror() is used to create all ObjectReference
instances, and it only creates one ObjectReference per JDWP objectID. I tested
this by making classObject() fetch the ObjectReference on every call and
compare the result to the cached value, and they always match. Also, the JDWP
spec calls out that each java Object instance has 1 unique objectID. The
following is the JDWP spec description of the objectID type.
"Uniquely identifies an object in the target VM. A particular object will be
identified by exactly one objectID in JDWP commands and replies throughout its
lifetime (or until the objectID is explicitly disposed). An ObjectID is not
reused to identify a different object unless it has been explicitly disposed,
regardless of whether the referenced object has been garbage collected. An
objectID of 0 represents a null object. Note that the existence of an object ID
does not prevent the garbage collection of the object. Any attempt to access a
a garbage collected object with its object ID will result in the INVALID_OBJECT
error code. Garbage collection can be disabled with the DisableCollection
command, but it is not usually necessary to do so."
While looking into this bug I also ran across the similar classLoader() API,
and noticed that it did not use synchronized and explained why in a comment.
That is where I got the motivation to remove synchronized from classObject().
Then I found a bunch more similar APIs. I cleaned up their comment to be
consistent.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/19439#issuecomment-2136257466
