Fixing JDK-8130493
Jonathan Gibbons
jonathan.gibbons at oracle.com
Wed Jan 10 22:43:00 UTC 2018
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