Fixing JDK-8130493

Schaef, Martin schaef at amazon.com
Wed Jan 10 23:29:43 UTC 2018


Fwiw, in the old code, Main already catches AnnotationProcessingErrors:
http://hg.openjdk.java.net/jdk8u/jdk8u/langtools/file/989188d1a978/src/share/classes/com/sun/tools/javac/main/Main.java#l551
But they don’t make it there because of the issue we discussed that Errors get swallowed and turned into Aborts which get swallowed here:
http://hg.openjdk.java.net/jdk8u/jdk8u/langtools/file/989188d1a978/src/share/classes/com/sun/tools/javac/main/JavaCompiler.java#l863
However, AnnotationProcessingErrors do print a stacktrace, so its not perfect either.

On 1/10/18, 5:43 PM, "Jonathan Gibbons" <jonathan.gibbons at oracle.com> wrote:

    In this case, it's definitely the user's problem, because it's a bad 
    class file being provided, but I don't think there's any merit in 
    providing a stacktrace. It's not like there's an unexpected error such 
    that it helps us or the user know where in the code the problem 
    occurred.  A clear message about the offending class (file) should be 
    sufficient.    This may mean we need catch blocks for recognized 
    exceptions as well as better handling of Throwable.
    
    It's a strongly-related issue that there are instances of "throw new 
    Abort" without a message.
    
    -- Jon
    
    On 01/10/2018 12:25 PM, Jan Lahoda wrote:
    > On 10.1.2018 20:06, Jonathan Gibbons wrote:
    >> Jan,
    >>
    >> javac should not generate a stacktrace in this situation. This case is
    >> about a bad class file, and so javac should generate an informative
    >> message, in the family of "bad class file" or "class not found" 
    >> messages.
    >>
    >> javac should only resort to stacktraces when code has crashed or
    >> otherwise behaved unexpectedly, such that javac cannot provide a helpful
    >> message.  There are two sub-cases here ... the crash is in javac code or
    >> libraries that it might call, in which case the message is, "oops, our
    >> fault, please report a bug", or the crash is in user code, in which
    >> case, the message is effectively, "it's your problem , not javac"
    >
    > (Especially for the exception from JDK-8130493.) I think this case is 
    > close to the "it's your problem , not javac" - the problem happens 
    > when the ServiceLoader tries to load a user's class into the VM 
    > (putting aside that it wasn't wrapped in the 
    > ServiceConfigurationError, as I'd expect based on the ServiceLoader 
    > javadoc) and it fails. So, while the specific stacktrace in the bug 
    > does not seem particularly useful, in other cases the exception may be 
    > useful for the user to diagnose the problem. I'd assume that this 
    > situation is fairly rare, and so I personally wouldn't mind more 
    > detailed output that could help to determine what is the cause of the 
    > problem.
    >
    > Jan
    >
    >>
    >> -- Jon
    >>
    >> On 01/10/2018 09:41 AM, Jan Lahoda 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