Fixing JDK-8130493
Schaef, Martin
schaef at amazon.com
Wed Jan 10 18:27:31 UTC 2018
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