8222422: vmTestbase/nsk/jdi/ClassLoaderReference/definedClasses tests failed with Unexpected Exception: null
Jean Christophe Beyler
jcbeyler at google.com
Thu May 16 02:14:08 UTC 2019
LGTM too :)
*From: *Chris Plummer <chris.plummer at oracle.com>
*Date: *Wed, May 15, 2019 at 6:55 PM
*To: *Daniil Titov, David Holmes, Jean Christophe Beyler
*Cc: *OpenJDK Serviceability
LGTM
>
> Chris
>
> On 5/15/19 12:15 PM, Daniil Titov wrote:
> > Hi David, Chris, and JC,
> >
> > Please review a new version of the change that also fixes the similar
> problems in ClassTypeImpl.subclasses(), InterfaceTypeImpl.subinterfaces(),
> and InterfaceTypeImpl.implementors() methods. The fix moves the common code
> that iterates over all loaded types while ignoring ObjectCollectedException
> in a new method VirtualMachineImpl.forEachClass(Consumer<ReferenceType>).
> >
> > Method ReferenceTypeImpl.nestedTypes() doesn't have this problem (since
> the only method it calls on a reference type is name() that cannot throw
> ObjectCollectedException()). However, to avoid potential pitfalls in the
> future if someone decides to change this code, I modified this method to
> use the same pattern as in the cases listed above.
> >
> > All vmTestbase/nsk/jdi tests as well as tier1, tier2, and tier3 tests
> succeeded.
> >
> > Webrev: http://cr.openjdk.java.net/~dtitov/8222422/webrev.02
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8222422
> >
> > Thanks,
> > Daniil
> >
> > On 5/13/19, 2:59 PM, "David Holmes" <david.holmes at oracle.com> wrote:
> >
> > Hi Daniil,
> >
> > On 14/05/2019 5:56 am, Daniil Titov wrote:
> > > Hi David,
> > >
> > > It seems as the chances that class unloading happens during the
> life-time of these tests are extremely low and switching Graal on increases
> these chances. At least without Graal I could not locate any test result
> that showed that ClassUnload event was posted. With Graal on 1 of 500
> tests fails (at least on Windows platform, on other platforms it must be
> more rare if not zero) and I could not find any successful test showing
> that ClassUnload event was ever posted for any class.
> >
> > Interesting that only Graal exposes this ... though it may be that
> the
> > reflection accessor actually relates to Graal code, hence the
> reason the
> > unloading only shows up with Graal. It may mean there is a hole in
> our
> > test coverage with JDI - may need some tests that involve executing
> > reflection code with accessors eagerly generated so that we do see
> > class-unload events. Though timing would be problematic.
> >
> > > You are right, the similar problem exists for ClassTypeImpl.
> subclasses() method and there is a separate issue for that: JDK-8223492.
> And while I was not able to reproduce it so far the logs provided in
> JDK-8223492 show that the problem here is the same. Initially I planned to
> keep these issues separated and proceed with JDK-8223492 after the current
> review is completed. But if you think it makes sense I could update the
> current webrev to include changes for ClassTypeImpl. subclasses() and then
> close JDK-8223492 as a duplicate.
> >
> > Entirely up to you. But I would be suspicious of all of the code
> that
> > iterates vm.allClasses().
> >
> > > I am also curious whether lambda forms, e.g.
> org/graalvm/compiler/lir/alloc/lsra/LinearScanLifetimeAnalysisPhase$$Lambda$93/1045689388,
> are supposed to be visible in JDI but I could not locate any discussion
> about this.
> >
> > I'm trying to find that out too - but we can deal with that as a
> > separate issue.
> >
> > Thanks,
> > David
> >
> > > Thanks!
> > > --Daniil
> > >
> > >
> > > Per tests data and additional tracing it seems as with Graal on
> the chances that class unloading happens during the test run are about
> 1/200 and only on Windows platform. Without Graal there were no single
> occurrence of the test when any ClassUnload event was posted.
> > >
> > > On 5/12/19, 3:19 PM, "David Holmes" <david.holmes at oracle.com>
> wrote:
> > >
> > > 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" <
> david.holmes at oracle.com> 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 <chris.plummer at oracle.com
> > > > > <mailto:chris.plummer at oracle.com>>
> > > > > *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
> > > >
> > > >
> > > >
> > >
> > >
> > >
> >
> >
> >
>
>
>
--
Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190515/90dc5987/attachment-0001.html>
More information about the serviceability-dev
mailing list