[JENKINS-37566] - Annotate URLish methods and handle NPE in RemoteClassLoader.

[oleg-nenashev]

The NPE seems to be non-realistic (why would RemoteClassloader construct URL from File?), but it’s better to fix it just in case.

https://issues.jenkins-ci.org/browse/JENKINS-37566

@reviewbybees

[ghost]

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

[jglick]

Probably not introducing a bug, but does not seem to be correctly solving it either.

[jglick]

This is internal, so why not make it @Nonnull? Then toURL could also be @Nonnull, as could toURLs. If you want to signal an error, throw an exception.

[oleg-nenashev]

makes sense

[jglick]

Why not throw, say, FileNotFoundException in such a case, so this can be @Nonnull?

[oleg-nenashev]

We could, of course.
Can do it though now we are rather in the code refactoring area than in the NPE warning fix one

[oleg-nenashev]

@jglick After some consideration I feel returning null is a valid behavior. E.g. this code uses abstract URLish, which may be both URL and File pointers. I could refactor all the code to do the same with exceptions, but I do not feel it improves the code much. WDYT?

public URL findResource(String name) {
        // first attempt to load from locally fetched jars
        URL url = super.findResource(name);
        final Channel channel = channel();
        if(url!=null || channel == null || !channel.isRemoteClassLoadingAllowed())   return url;

        try {
            if(resourceMap.containsKey(name)) {
                URLish f = resourceMap.get(name);
                if(f==null) return null;    // no such resource
                URL u = f.toURL();
                if (u!=null)    return u;
            }

[jglick]

IIUC it is the line

if (u!=null)    return u;

that is under discussion. I think it makes sense to have toURL throw an exception if it is definitely broken, and for the caller to decide what to do with that—log it, in this case (whatever the catch block does).

Or fix this method to be @Nonnull, which seems more natural, and leave it up to the consumer of the resulting URL to deal with missing resources—they would anyway need to be handling FileNotFoundException, so it seems pointless to be prechecking that condition.

IOW lines 46–50 seem stupid.

[jglick]

Does not make things any worse, though it misses an opportunity to make the code clearer and more robust.

[jglick]

IIUC it is the line

if (u!=null)    return u;

that is under discussion. I think it makes sense to have toURL throw an exception if it is definitely broken, and for the caller to decide what to do with that—log it, in this case (whatever the catch block does).

Or fix this method to be @Nonnull, which seems more natural, and leave it up to the consumer of the resulting URL to deal with missing resources—they would anyway need to be handling FileNotFoundException, so it seems pointless to be prechecking that condition.

IOW lines 46–50 seem stupid.

[oleg-nenashev]

Does not make things any worse, though it misses an opportunity to make the code clearer and more robust.

I will consider it for the next refactoring phases. It is definitely not the biggest issue in Remoting