Fixing JDK-8130493
Jonathan Gibbons
jonathan.gibbons at oracle.com
Wed Jan 10 18:31:49 UTC 2018
I would still like to examine this problem further. I would like to
understand the right solution, and not just go for the minimal solution.
-- Jon
On 01/10/2018 10:27 AM, Schaef, Martin wrote:
> Tested it, and it works for my use case and the example in JDK-8130493. See patch below.
> Want to go forward with this?
>
>
> diff --new-file --unified --recursive --exclude='*.autotools' --exclude='*.cproject' --exclude='*.project' ./langtools/src/share/classes/com/sun/tools/javac/processing/JavacProcessingEnvironment.java ./langtools/src/share/classes/com/sun/tools/javac/processing/JavacProcessingEnvironment.java
> --- ./langtools/src/share/classes/com/sun/tools/javac/processing/JavacProcessingEnvironment.java 2017-10-20 20:28:37.000000000 +0000
> +++ ./langtools/src/share/classes/com/sun/tools/javac/processing/JavacProcessingEnvironment.java 2018-01-10 16:46:52.798017039 +0000
> a 2018-01-10 17:50:25.093338055 +0000
> @@ -339,7 +339,7 @@
> }
> } catch (Throwable t) {
> log.error("proc.service.problem");
> - throw new Abort(t);
> + throw new AnnotationProcessingError(t);
> }
> }
>
> @@ -361,7 +361,7 @@
> log.error("proc.bad.config.file", sce.getLocalizedMessage());
> throw new Abort(sce);
> } catch (Throwable t) {
> - throw new Abort(t);
> + throw new AnnotationProcessingError(t);
> }
> }
>
>
> On 1/10/18, 12:42 PM, "Jan Lahoda" <jan.lahoda at oracle.com> wrote:
>
> For a minimal change, I think it should be enough to change (in
> ServiceIterator) the existing:
> ---
> } catch (Throwable t) {
> throw new Abort(t);
> }
> ---
> to
> ---
> } catch (Throwable t) {
> throw new AnnotationProcessingError(t);
> }
> ---
>
> AnnotationProcessingError is handled both in JavacTaskImpl and
> com.sun.tools.javac.main.Main. For command line javac, this would get an
> output like:
> ---
> An annotation processor threw an uncaught exception.
> Consult the following stack trace for details.
> java.lang.ClassFormatError: Truncated class file
> [stacktrace]
> ---
>
> Which is in line with the behavior when -processor <name> is used
> (NameProcessIterator).
>
> But I guess it would be good to improve consistency of error handling in
> JavacProcessingEnvironment at some point.
>
> Jan
>
> On 10.1.2018 18:23, Jonathan Gibbons wrote:
> > Why would you not just change
> >
> >
> > from
> >
> > } catch (Throwable t) {
> >
> > log.error("proc.service.problem");
> >
> > throw new Abort(t);
> >
> > to
> >
> > } catch (Throwable t) {
> >
> > log.error("proc.service.problem");
> >
> > throw new AnnotationProcessingError(t);
> >
> >
> > and then briefly, concisely, handle AnnotationProcessingError in
> > JavaCompiler.
> >
> > -- Jon
> >
> >
> > On 1/10/18 9:13 AM, Schaef, Martin wrote:
> >>
> >> I understand. The bare minimum that fixes the test cases would be to
> >> change the exception handling in the constructor of ServiceIterator:
> >>
> >> http://hg.openjdk.java.net/jdk8u/jdk8u/langtools/file/989188d1a978/src/share/classes/com/sun/tools/javac/processing/JavacProcessingEnvironment.java#l340
> >>
> >> and in ServiceIterator.next():
> >>
> >> http://hg.openjdk.java.net/jdk8u/jdk8u/langtools/file/989188d1a978/src/share/classes/com/sun/tools/javac/processing/JavacProcessingEnvironment.java#l363
> >>
> >> from
> >>
> >> } catch (Throwable t) {
> >>
> >> log.error("proc.service.problem");
> >>
> >> throw new Abort(t);
> >>
> >> to
> >>
> >> } catch (Exception t) {
> >>
> >> log.error("proc.service.problem");
> >>
> >> throw new Abort(t);
> >>
> >> } catch (Throwable t) {
> >>
> >> log.error("Some error text");
> >>
> >> throw new AnnotationProcessingError (t);
> >>
> >> With this change, the exit code of javac is 3 instead of 0.
> >>
> >> Does that sound like something you could support? If so, what should
> >> go in the log.error messages?
> >>
> >> Cheers,
> >>
> >> Martin
> >>
> >> *From: *Jonathan Gibbons <jonathan.gibbons at oracle.com>
> >> *Date: *Wednesday, January 10, 2018 at 11:39 AM
> >> *To: *"Schaef, Martin" <schaef at amazon.com>,
> >> "compiler-dev at openjdk.java.net" <compiler-dev at openjdk.java.net>
> >> *Cc: *"Hohensee, Paul" <hohensee at amazon.com>
> >> *Subject: *Re: Fixing JDK-8130493
> >>
> >> Martin,
> >>
> >> I have not read your patch, but changing the semantics of the iterator
> >> sounds like a step too far.
> >>
> >> Without trying to implement the following yet, I would expect the
> >> general direction of a solution to be to one of the following:
> >>
> >> 1. if the exception should be fatal, allow it to propagate up to
> >> JavaCompiler, (similar to as now) but maybe with a custom new wrapper
> >> exception
> >>
> >> 2. if the exception should not be fatal, translate it to an error
> >> message (log.error(...)) and continue.
> >>
> >> It probably needs more analysis and discussion to determine which of
> >> those two directions to take.
> >>
> >> -- Jon
> >>
> >> On 1/10/18 8:17 AM, Schaef, Martin wrote:
> >>
> >> Let me revise that:
> >>
> >> In JavacProcessingEnvironment.initProcessorIterator
> >> <http://hg.openjdk.java.net/jdk8u/jdk8u/langtools/file/989188d1a978/src/share/classes/com/sun/tools/javac/processing/JavacProcessingEnvironment.java#l256>
> >> the code chooses one of two types of iterators:
> >>
> >> The NameProcessIterator
> >> <http://hg.openjdk.java.net/jdk8u/jdk8u/langtools/file/989188d1a978/src/share/classes/com/sun/tools/javac/processing/JavacProcessingEnvironment.java#l384>
> >> if ther -processor flag is given or the ServiceIterator
> >> <http://hg.openjdk.java.net/jdk8u/jdk8u/langtools/file/989188d1a978/src/share/classes/com/sun/tools/javac/processing/JavacProcessingEnvironment.java#l325>.
> >>
> >> If we look at the NameProcessIterator.hasNext()
> >> <http://hg.openjdk.java.net/jdk8u/jdk8u/langtools/file/989188d1a978/src/share/classes/com/sun/tools/javac/processing/JavacProcessingEnvironment.java#l396>
> >> implementation, it checks if there is a next name AND checks if
> >> this class name can be loaded. That is, if the class is not
> >> present, the hasNext() method already returns false (or, in our
> >> case, throws the NoClassDefFoundError which gets wrapped into an
> >> AnnotationProcessingError).
> >>
> >> If we look at the ServiceIterator together with the stack trace
> >> from my previous email, we see that the .hasNext() and .next()
> >> calls get forwarded into the ServiceLoader.LazyIterator
> >> <http://hg.openjdk.java.net/jdk8u/jdk8u/langtools/file/989188d1a978/src/share/classes/com/sun/tools/javac/util/ServiceLoader.java#l257>.
> >> This behaves different than the NameProcessIterator: hasNext()
> >> <http://hg.openjdk.java.net/jdk8u/jdk8u/langtools/file/989188d1a978/src/share/classes/com/sun/tools/javac/util/ServiceLoader.java#l222>
> >> only checks if a next name exists and would not throw a
> >> NoClassDefFoundError. Then next()
> >> <http://hg.openjdk.java.net/jdk8u/jdk8u/langtools/file/989188d1a978/src/share/classes/com/sun/tools/javac/util/ServiceLoader.java#l256>
> >> throws the NoClassDefFoundError which gets caught by the catch-all
> >> in JavaProcessingEnvironment.ServiceIterator.next()
> >> <http://hg.openjdk.java.net/jdk8u/jdk8u/langtools/file/989188d1a978/src/share/classes/com/sun/tools/javac/processing/JavacProcessingEnvironment.java#l363>
> >> and wrapped into an Abort.
> >>
> >> So, to summarize:
> >>
> >> For a NameProcessIterator, a missing class causes a
> >> AnnotationProcessingError thrown from
> >> NameProcessIterator.hasNext()
> >> <http://hg.openjdk.java.net/jdk8u/jdk8u/langtools/file/989188d1a978/src/share/classes/com/sun/tools/javac/processing/JavacProcessingEnvironment.java#l396>
> >> and for ServiceIterator, it causes a Abert thrown by
> >> JavaProcessingEnvironment.ServiceIterator.next()
> >> <http://hg.openjdk.java.net/jdk8u/jdk8u/langtools/file/989188d1a978/src/share/classes/com/sun/tools/javac/processing/JavacProcessingEnvironment.java#l363>.
> >>
> >> I could fix my problem and JDK-8130493
> >> <https://bugs.openjdk.java.net/browse/JDK-8130493> by checking if
> >> the class can be loaded in the hasNext method and changing the
> >> exception handling in
> >> JavaProcessingEnvironment.ServiceIterator.next()
> >> <http://hg.openjdk.java.net/jdk8u/jdk8u/langtools/file/989188d1a978/src/share/classes/com/sun/tools/javac/processing/JavacProcessingEnvironment.java#l363>.
> >> The problem with that is that the lazy iterator is not lazy
> >> anymore. Is there a reason why this Iterator has to be lazy?
> >>
> >> I attached a patch that solves the issue for me. Feedback would be
> >> great.
> >>
> >> Cheers,
> >>
> >> Martin
> >>
> >> *From: *compiler-dev <compiler-dev-bounces at openjdk.java.net>
> >> <mailto:compiler-dev-bounces at openjdk.java.net> on behalf of
> >> "Schaef, Martin" <schaef at amazon.com> <mailto:schaef at amazon.com>
> >> *Date: *Tuesday, January 9, 2018 at 3:37 PM
> >> *To: *Jonathan Gibbons <jonathan.gibbons at oracle.com>
> >> <mailto:jonathan.gibbons at oracle.com>,
> >> "compiler-dev at openjdk.java.net"
> >> <mailto:compiler-dev at openjdk.java.net>
> >> <compiler-dev at openjdk.java.net> <mailto:compiler-dev at openjdk.java.net>
> >> *Subject: *Re: Fixing JDK-8130493
> >>
> >> It’s a user class. See full stack trace below.
> >>
> >> The problem is that the ClassNotFound is already caught at
> >> com.sun.tools.javac.util.ServiceLoader$LazyIterator.next(ServiceLoader.java:255)
> >> <http://hg.openjdk.java.net/jdk8u/jdk8u/langtools/file/989188d1a978/src/share/classes/com/sun/tools/javac/util/ServiceLoader.java#l257>
> >> which already wraps it in a ServiceConfigurationError
> >> <http://hg.openjdk.java.net/jdk8u/jdk8u/langtools/file/989188d1a978/src/share/classes/com/sun/tools/javac/util/ServiceLoader.java#l100>
> >> in the fail method. That is, the exception gets wrapped twice.
> >> First, LazyIterator.next wraps it in a ServieConfigurationError,
> >> then ServiceIterator.next wraps it in an Abort.
> >>
> >> Is it safe to change both wrappings?
> >>
> >> com.sun.tools.javac.util.Abort: java.lang.NoClassDefFoundError:
> >> com/amazon/coral/util/reflection/AnnotatedManifestAnnotationProcessor
> >>
> >> at
> >> com.sun.tools.javac.processing.JavacProcessingEnvironment$ServiceIterator.next(JavacProcessingEnvironment.java:364)
> >>
> >> at
> >> com.sun.tools.javac.processing.JavacProcessingEnvironment$ServiceIterator.next(JavacProcessingEnvironment.java:325)
> >>
> >> at
> >> com.sun.tools.javac.processing.JavacProcessingEnvironment$DiscoveredProcessors$ProcessorStateIterator.next(JavacProcessingEnvironment.java:597)
> >>
> >> at
> >> com.sun.tools.javac.processing.JavacProcessingEnvironment.discoverAndRunProcs(JavacProcessingEnvironment.java:690)
> >>
> >> at
> >> com.sun.tools.javac.processing.JavacProcessingEnvironment.access$1800(JavacProcessingEnvironment.java:91)
> >>
> >> at
> >> com.sun.tools.javac.processing.JavacProcessingEnvironment$Round.run(JavacProcessingEnvironment.java:1035)
> >>
> >> at
> >> com.sun.tools.javac.processing.JavacProcessingEnvironment.doProcessing(JavacProcessingEnvironment.java:1176)
> >>
> >> at
> >> com.sun.tools.javac.main.JavaCompiler.processAnnotations(JavaCompiler.java:1170)
> >>
> >> at
> >> com.sun.tools.javac.main.JavaCompiler.compile(JavaCompiler.java:856)
> >>
> >> at com.sun.tools.javac.main.Main.compile(Main.java:523)
> >>
> >> at com.sun.tools.javac.main.Main.compile(Main.java:381)
> >>
> >> at com.sun.tools.javac.main.Main.compile(Main.java:370)
> >>
> >> at com.sun.tools.javac.main.Main.compile(Main.java:361)
> >>
> >> at com.sun.tools.javac.Main.compile(Main.java:56)
> >>
> >> at com.sun.tools.javac.Main.main(Main.java:42)
> >>
> >> Caused by: java.lang.NoClassDefFoundError:
> >> com/amazon/coral/util/reflection/AnnotatedManifestAnnotationProcessor
> >>
> >> at java.lang.ClassLoader.defineClass1(Native Method)
> >>
> >> at java.lang.ClassLoader.defineClass(ClassLoader.java:763)
> >>
> >> at
> >> java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142)
> >>
> >> at java.net.URLClassLoader.defineClass(URLClassLoader.java:467)
> >>
> >> at java.net.URLClassLoader.access$100(URLClassLoader.java:73)
> >>
> >> at java.net.URLClassLoader$1.run(URLClassLoader.java:368)
> >>
> >> at java.net.URLClassLoader$1.run(URLClassLoader.java:362)
> >>
> >> at java.security.AccessController.doPrivileged(Native Method)
> >>
> >> at java.net.URLClassLoader.findClass(URLClassLoader.java:361)
> >>
> >> at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
> >>
> >> at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
> >>
> >> at java.lang.Class.forName0(Native Method)
> >>
> >> at java.lang.Class.forName(Class.java:348)
> >>
> >> at
> >> com.sun.tools.javac.util.ServiceLoader$LazyIterator.next(ServiceLoader.java:255)
> >>
> >> at
> >> com.sun.tools.javac.util.ServiceLoader$1.next(ServiceLoader.java:337)
> >>
> >> at
> >> com.sun.tools.javac.processing.JavacProcessingEnvironment$ServiceIterator.next(JavacProcessingEnvironment.java:359)
> >>
> >> ... 14 more
> >>
> >> Caused by: java.lang.ClassNotFoundException:
> >> com.amazon.coral.util.reflection.AnnotatedManifestAnnotationProcessor
> >>
> >> at java.net.URLClassLoader.findClass(URLClassLoader.java:381)
> >>
> >> at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
> >>
> >> at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
> >>
> >> *From: *Jonathan Gibbons <jonathan.gibbons at oracle.com>
> >> <mailto:jonathan.gibbons at oracle.com>
> >> *Date: *Tuesday, January 9, 2018 at 1:19 PM
> >> *To: *"Schaef, Martin" <schaef at amazon.com>
> >> <mailto:schaef at amazon.com>, "compiler-dev at openjdk.java.net"
> >> <mailto:compiler-dev at openjdk.java.net>
> >> <compiler-dev at openjdk.java.net> <mailto:compiler-dev at openjdk.java.net>
> >> *Subject: *Re: Fixing JDK-8130493
> >>
> >> What is the class triggering the ClassNotFoundException?
> >>
> >> If it is a user class not being found, it should be wrapped in a
> >> ClientCodeException. If it is a javac class not being found, it
> >> is reasonable to wrap it in an Abort.
> >>
> >> -- Jon
> >>
> >> On 1/9/18 9:52 AM, Schaef, Martin wrote:
> >>
> >> Hi,
> >>
> >> We have some users suffering from JDK-8130493
> >> <https://bugs.openjdk.java.net/browse/JDK-8130493> (their
> >> builds succeed, but the compiler actually failed). I did some
> >> digging and the following sequence happens:
> >>
> >> A ClassNotFoundException is thrown in
> >> JavaProcessingEnvironment.ServiceIterator.next()
> >> <http://hg.openjdk.java.net/jdk8u/jdk8u/langtools/file/989188d1a978/src/share/classes/com/sun/tools/javac/processing/JavacProcessingEnvironment.java#l364>
> >> and re-thrown as an Abort.
> >>
> >> This Abort reaches JavaCompiler.processAnnotations()
> >> <http://hg.openjdk.java.net/jdk8u/jdk8u/langtools/file/989188d1a978/src/share/classes/com/sun/tools/javac/main/JavaCompiler.java#l1170>
> >> and is finally caught in JavaCompiler.compile()
> >> <http://hg.openjdk.java.net/jdk8u/jdk8u/langtools/file/989188d1a978/src/share/classes/com/sun/tools/javac/main/JavaCompiler.java#l856> by
> >> the following snippet:
> >>
> >> try {
> >>
> >> initProcessAnnotations(processors);
> >>
> >> // These method calls must be chained to avoid
> >> memory leaks
> >>
> >> delegateCompiler =
> >>
> >> processAnnotations( //<<<<<< ABORT COMES OUT HERE
> >>
> >> enterTrees(stopIfError(CompileState.PARSE,
> >> parseFiles(sourceFileObjects))),
> >>
> >> classnames);
> >>
> >> …
> >>
> >> } catch (Abort ex) {
> >>
> >> if (devVerbose)
> >>
> >> ex.printStackTrace(System.err);
> >>
> >> } finally {
> >>
> >> and swallowed ... but it's printed if XDev is set.
> >>
> >> What is a proper way to fix this? Is it correct to wrap all
> >> exceptions in JavaProcessingEnvironmentServiceIterator into
> >> aborts? Or would it be better to distinguish different Aborts
> >> in JavaCompiler.java?
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >
>
>
More information about the compiler-dev
mailing list