Fixing JDK-8130493

Wed Jan 10 17:38:09 UTC 2018

Mostly because I cannot see if there are other Exceptions that should be wrapped into Abort. I saw other places that throw AnnotationProcessingError, so I thought this would be a good way forward.

Do you think that I can safely wrap all exceptions into an Error at this point?

If so, I’ll adjust my patch.

From: Jonathan Gibbons <jonathan.gibbons at oracle.com>
Date: Wednesday, January 10, 2018 at 12:24 PM
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

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><mailto:jonathan.gibbons at oracle.com>
Date: Wednesday, January 10, 2018 at 11:39 AM
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>
Cc: "Hohensee, Paul" <hohensee at amazon.com><mailto: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/9681e57e/attachment-0001.html>