Hi Daniil,

On 12/05/2019 3:14 am, Daniil Titov wrote:
Hi David,

There are two ways how these reference types (for the classes that become 
unloaded later) could appear in the collection 
com.sun.tools.jdi.VirtualMachineImpl maintains  and stores in  
VirtualMachineImpl.typesBySignature instance variable:
1) Initial load when all classes are requested from the debuggee
2) Or added later when ClassPrepare event for a specific class is posted and 
handled

The reference types are removed from VirtualMachineImpl.typesBySignature  when 
ClassUnload event is processed. However, additional tracing showed ( pleases 
see below the sample output) that in some cases these ClassUnload events are 
received after we entered definedClasses() method and  started iterating over 
the copy of the collection of the classes returned by vm.allClasses() method.

Thanks for clarifying that for me. I was thinking in this case that definedClasses() was directly querying the target VM, but it isn't it's iterating the existing set of known classes cached in the client (vm.allClasses()).

Though it seems that whether or not we will hit this ObjectCollectedException depends on what we have already done with a particular ReferenceType. In this case we hit the exception when invoking classLoader() but that will only throw the exception if we do not already have the classLoader cached and have to go and seek it from the target VM.

I do wonder why this issue should suddenly appear now? Encountering an unloaded class, like a generated reflection accessor, should always have been possible and so we should have seen this before. Something must have changed recently. ??

I'm also concerned about other code that iterates through vm.allClasses() and which does not seem to be aware of the possibility of CollectedObjectException. For example in ClassTypeImpl.java we have:

   public List<ClassType> subclasses() {
        List<ClassType> subs = new ArrayList<>();
        for (ReferenceType refType : vm.allClasses()) {
            if (refType instanceof ClassType) {
                ClassType clazz = (ClassType)refType;
                ClassType superclass = clazz.superclass();
                if ((superclass != null) && superclass.equals(this)) {
                    subs.add((ClassType)refType);
                }
            }
        }

        return subs;
    }

If the superclass is already cached then this will work, but if it has to call to the target VM over JDWP then we will have the same bug I think. Which again raises the question for me as to why we are not seeing tests fail here?

Based on the output below it seems as all unloaded classes are the generated 
ones or lambda forms.
--> debugger: ......getting: List definedClasses = clRef.definedClasses();

Received Unload Event for Ljdk/internal/reflect/GeneratedConstructorAccessor1;
Handled Unload Event for Ljdk/internal/reflect/GeneratedConstructorAccessor1;

This is fine - generated reflection accessor are loaded in a custom classloader specifically so they can be unloaded promptly. But ...

Received Unload Event for 
Lorg/graalvm/compiler/lir/alloc/lsra/LinearScanLifetimeAnalysisPhase$$Lambda$93/1045689388;
Handled Unload Event for 
Lorg/graalvm/compiler/lir/alloc/lsra/LinearScanLifetimeAnalysisPhase$$Lambda$93/1045689388;

... these are I believe definitions of VM anonymous classes (they have names of the form foo/<number> which are not legal class names). Should these even be visible to JDI?

Thanks,
David
-----

Received Unload Event for Ljava/lang/invoke/LambdaForm$DMH/733189374;
Handled Unload Event for Ljava/lang/invoke/LambdaForm$DMH/733189374;
Received Unload Event for 
Lorg/graalvm/compiler/lir/alloc/lsra/LinearScanLifetimeAnalysisPhase$$Lambda$94/454340234;
Handled Unload Event for 
Lorg/graalvm/compiler/lir/alloc/lsra/LinearScanLifetimeAnalysisPhase$$Lambda$94/454340234;
Received Unload Event for Ljava/lang/invoke/LambdaForm$DMH/1780167778;
Handled Unload Event for Ljava/lang/invoke/LambdaForm$DMH/1780167778;
Received Unload Event for 
Lorg/graalvm/compiler/lir/alloc/lsra/LinearScanLifetimeAnalysisPhase$$Lambda$86/323001064;
Handled Unload Event for 
Lorg/graalvm/compiler/lir/alloc/lsra/LinearScanLifetimeAnalysisPhase$$Lambda$86/323001064;

Exception while getting classloader for type 
jdk.internal.reflect.GeneratedConstructorAccessor1
# ERROR: ##> debugger: ERROR: Exception : com.sun.jdi.ObjectCollectedException


Thanks!
--Daniil



On 5/11/19, 3:39 AM, "David Holmes" <[email protected]> wrote:

     On 11/05/2019 2:14 pm, Jean Christophe Beyler wrote:
     > Isn't that the point? The list returned could have unloaded classes  and
     > we can catch it via this exception (from the comment above
     > the ReferenceType interface):
     >
     >   * Any method on <code>ReferenceType</code> or which directly or
     > indirectly takes
     >   * <code>ReferenceType</code> as parameter may throw
     >   * {@link ObjectCollectedException} if the mirrored type has been 
unloaded.
     >
     > Turns out that even in the definedClasses, we can get that exception so
     > we should check for it while walking the reference types as well?
I understand that the list returned to the "debugger" process may
     contain ReferenceTypes for types that have actually been unloaded by the
     time it queries them (unless the debuggee is suspended of course). But I
     don't see how we can encounter those types while compiling the list in
     the debuggee in the first place.
Something seems amiss here ... possibly just my understanding ... David > Jc
     >
     > *From: *Chris Plummer <[email protected]
     > <mailto:[email protected]>>
     > *Date: *Fri, May 10, 2019 at 9:09 PM
     > *To: *David Holmes, Daniil Titov, OpenJDK Serviceability
     >
     >     On 5/10/19 9:03 PM, Chris Plummer wrote:
     >      > On 5/10/19 8:59 PM, David Holmes wrote:
     >      >> Hi Daniil,
     >      >>
     >      >> On 11/05/2019 12:10 pm, Daniil Titov wrote:
     >      >>> Please review the change that fixes an intermittent failure of 
the
     >      >>> test.
     >      >>>
     >      >>> The tests checks the implementation of  the
     >      >>> com.sun.tools.jdi.ClassLoaderReference class. The problem here 
is
     >      >>> that while
     >      >>> com.sun.tools.jdi.ClassLoaderReferenceImpl.definedClasses()
     >     iterates
     >      >>> over all loaded classes to retrieve a classloader and compares
     >     it to
     >      >>> the current one, some of the classes might become unloaded and
     >      >>> garbage collected (e.g.
     >      >>> org.graalvm.compiler.nodes.InliningLog$$Lambda$41.899832640 or
     >      >>> jdk.internal.reflect.GeneratedConstructorAccessor1, etc.). If 
this
     >      >>> happens then the attempt to retrieve a classloader for the
     >     collected
     >      >>> class results in com.sun.jdi.ObjectCollectedException being 
thrown.
     >      >>
     >      >> That seems odd to me. If you have a reference to the Class then 
it
     >      >> can't be unloaded. I would not expect allClasses() to have
     >      >> weak-references, so a class should not be unloadable while you 
are
     >      >> examining it. Unless it is finding VM anonymous classes (which it
     >      >> should not!).
     >      >>
     >      > I was just typing up something similar. Shouldn't the test do a
     >      > vm.suspend() and then call disableCollection() on each class
     >     returned
     >      > by vm.allClasses()?
     >     Oh wait, this isn't a test. It's part of JDI. I guess it would be up 
to
     >     the user of ClassLoaderReference.definedClasses() to do the suspend. 
In
     >     fact I'm not sure there's much purpose in calling
     >     ClassLoaderReference.definedClasses() without suspending first. Even
     >     with your changes, the list returned can end up with references to
     >     unloaded classes.
     >
     >     Chris
     >      >
     >      > Chris
     >      >> David
     >      >> -----
     >      >>
     >      >>> The fix catches this com.sun.jdi.ObjectCollectedException and
     >      >>> continues iterating over the rest of the classes.
     >      >>>
     >      >>> Webrev: http://cr.openjdk.java.net/~dtitov/8222422/webrev.01
     >      >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8222422
     >      >>>
     >      >>> Thanks!
     >      >>> --Daniil
     >      >>>
     >      >>>
     >      >
     >      >
     >
     >
     >
     >
     > --
     >
     > Thanks,
     > Jc

Reply via email to