Guard against NullPointerException in AnnotationParser

[guusdk]

We've got a broken setup, that is unlikely to be caused by the problem described herein. Via the stacktrace, however, we did find a possible improvement for Jetty 12.0.14 using EE8. No wars are going to be won by this, but still, it might be a useful improvement.

Given this stacktrace:

org.eclipse.jetty.ee8.webapp.WebAppContext - Failed startup of context oeje8w.WebAppContext@310ac970{ROOT,/,file:///home/guus/SourceCode/IgniteRealtime/Openfire/distribution/target/distribution-base/plugins/admin/webapp/,false}{/home/guus/SourceCode/IgniteRealtime/Openfire/distribution/target/distribution-base/plugins/admin/webapp}
java.lang.NullPointerException: Cannot invoke "java.nio.file.Path.getFileName()" because "path" is null
        at org.eclipse.jetty.util.FileID.isClassFile(FileID.java:369) ~[?:?]
        at org.eclipse.jetty.ee8.annotations.AnnotationParser.parse(AnnotationParser.java:489) ~[?:?]
        at org.eclipse.jetty.ee8.annotations.AnnotationConfiguration$ParserTask.call(AnnotationConfiguration.java:170) ~[?:?]
        at org.eclipse.jetty.util.ExceptionUtil.call(ExceptionUtil.java:333) ~[?:?]
        at org.eclipse.jetty.util.ExceptionUtil$MultiException.callAndCatch(ExceptionUtil.java:270) ~[?:?]
        at org.eclipse.jetty.ee8.annotations.AnnotationConfiguration.lambda$scanForAnnotations$0(AnnotationConfiguration.java:488) ~[?:?]
        at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:979) ~[?:?]
        at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.doRunJob(QueuedThreadPool.java:1209) ~[?:?]
        at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1164) ~[?:?]
        at java.lang.Thread.run(Thread.java:840) [?:?]

It appears that there's room for improvement in AnnotationParser:

    public void parse(final Set<? extends Handler> handlers, Resource r) throws Exception {
        if (r == null)
            return;
        if (FileID.isJavaArchive(r.getPath())) {
            parseJar(handlers, r);
            return;
        }
        if (r.isDirectory()) {
            parseDir(handlers, r);
            return;
        }
        if (FileID.isClassFile(r.getPath())) {
            parseClass(handlers, null, r.getPath());
        }
        if (LOG.isDebugEnabled())
            LOG.warn("Resource not able to be scanned for classes: {}", r);
    }

If the Resource argument is null, the method immediately short-circuits to false. If the Resource is not null, but has a null path, then a NPE is thrown in only the last check (in FileID.isClassFile()). FileID.isJavaArchive() isn't bothered by having a null argument, but isClassFile is.

I'd suggest to make this behavior consistent, either by short-circuiting in the first check, or by adjusting the implementation of FileID.isClassFile()

[joakime]

What version of Jetty?

[guusdk]

Jetty 12.0.14 using EE8

[joakime]

What Resource type are you using?
I'd like to setup a unit test for the same conditions.

[joakime]

Any fix for this wont make it into this months release. (too late for that)

[guusdk]

To be honest: I don't know yet. We're trying to do a forklift upgrade from 10.0.18 to 12.0.14 and everything is broken :). I've only just now started investigating why. This caught my eye and I'd thought to fire off a quick issue lest I'd forget.

I was not counting on a response within three minutes! /me waves angry first :)

[joakime]

@guusdk a WIP PR for this is already opened at #12454

It's missing a testcase, something to prevent regression.
It would help if you can put a breakpoint in that AnnotationPaser.parse() method.
Stopping if r.getPath() is null.
Then telling us what your r (the Resource object) type is.
Even the details of that Resource object, as a screenshot of your debug window or a copy/paste would be helpful.

[guusdk]

The offender seems to be an instance of org.eclipse.jetty.util.resource.URLResourceFactory.URLResource

[joakime]

Looks like OpenFire is setting the scheme file to the URLResourceFactory. That's not a great idea, it will cause all kinds of strange file locking issues with the deprecated (in the JVM) URL classes.

[joakime]

Huh, OpenFire seems to only support Jetty 10.0.x
Are you upgrading it?

[joakime]

Ya, looks like you are developer with OpenFire.
Do you have a branch that you are doing this work in?

Don't set / use the URLResourceFactory for the default schemes (file, jrt, jar, etc).
If you build against graalvm, definitely do not use URLResourceFactory, stick with the defaults.

Only use it for oddball schemes like bundle (found in OSGi land).

[guusdk]

Yeah, like I wrote earlier, we ran into this while working to get Jetty upgraded in our project.

Thanks for the pointers!

[guusdk]

@joakime The upgrade is being worked on here: https://github.com/surevine/Openfire/tree/feature/jetty-12.0.14-ee8-upgrade

What we're upgrading is old code that itself might not have been in the best state. We've been using Jetty for ages. Most of the upgrades were a 'hit it until it runs' kind of effort.

[joakime]

I tossed a few quick comments in your commits that might help you.

[guusdk]

That's super helpful, thanks!