Fixing JDK-8130493
Jonathan Gibbons
jonathan.gibbons at oracle.com
Wed Jan 10 18:53:32 UTC 2018
Martin,
Your first question is the key question, although maybe not in the sense
you mean.
> My first question would be, what is the use case where iterating through annotation processors should Abort the compiler/ and exit without error message/?
Note the emphasis is on "exiting without an error message". That is the
bug that needs to be fixed. In general, Abort should be used to halt
the compilation/after reporting an informative error message/. I see a
number of use-sites in the JavacProcessingEnvironment class where this
pattern is not followed correctly, and that hints at the correct
solution. If an error message is reported, that will (a) not surprise
the user that the compiler seems to ignore the issue , and (b) will
cause a non-zero exit code from javac.
-- Jon
On 01/10/2018 10:43 AM, Schaef, Martin wrote:
> My first question would be, what is the use case where iterating through annotation processors should Abort the compiler and exit without error message?
> Currently, I can’t think of a case where this is expected.
> I would assume that the idea behind the lazy iterator in “hasNext” is that some annotation processors can be discovered but not loaded. But for this case, the “next” method shouldn’t just throw all the way up to Main. Otherwise, you don’t really need lazy loading.
>
> Can you think of a scenario where a processor is discovered but cannot be loaded? What’s the expected behavior?
>
>
> On 1/10/18, 1:32 PM, "Jonathan Gibbons" <jonathan.gibbons at oracle.com> wrote:
>
> 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?
> > >>
> > >>
> > >>
> > >>
> > >>
> > >>
> > >>
> > >
> >
> >
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/compiler-dev/attachments/20180110/46d6eaea/attachment-0001.html>
More information about the compiler-dev
mailing list