Bug 57574 - javax.el.ImportHandler.importPackage() does not work in Equinox OSGi
Summary: javax.el.ImportHandler.importPackage() does not work in Equinox OSGi
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: EL (show other bugs)
Version: 8.0.x-trunk
Hardware: All All
: P2 normal (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
Depends on:
Reported: 2015-02-12 08:53 UTC by Jan Bartel
Modified: 2015-02-13 11:08 UTC (History)
0 users

ImportHandler proposed patch (593 bytes, patch)
2015-02-12 08:53 UTC, Jan Bartel
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jan Bartel 2015-02-12 08:53:08 UTC
Created attachment 32462 [details]
ImportHandler proposed patch

The code in javax.el.ImportHandler.importPackage(String name) tries to load a package, first via java.lang.Package.getPackage(name), and if that fails, via the thread context classloader getResource(name) instead.

The call to Package.getPackage(name) appears to return null under Equinox, not sure about other osgi environments.

Unfortunately the call to the TCCL.getResource(name) also returns null, because the name does not have a trailing '/'. Eg "javax/servlet" should be "javax/servlet/".

The attached patch always adds a trailing '/' to the package name. The fix has been tested on a recent version of Equinox. 

Comment 1 Mark Thomas 2015-02-12 09:05:49 UTC
So why isn't this a bug in Equinox? If you can point to a specification that says the trailing / is required then we can add it but otherwise this is going to get resolved as invalid.
Comment 2 Konstantin Kolinko 2015-02-12 15:51:06 UTC
This is some error-handling path in case if Package.getPackage(name) returned null. Is there a valid reason for getPackage() method to return null? From the comment in the code it looks that there is.

> String path = name.replace('.', '/');
> URL url = cl.getResource(path);

I think that both approaches (with and without terminating '/') are invalid.

I see no saying in ClassLoader.getResource() javadoc that this operation is applicable to directories.  The support of ending '/' may be dependent on the underlying file system. I.e.:

- I think a file system is likely to behave independently of the trailing slash.

- I think a JAR is likely to expect a trailing '/' if it was packed with explicit archive entries for directories. If there are no such entries in the archive then it is likely to return false 'null' answers.

I propose to postpone the check until one actually tries to locate the class. It is for performance's sake, like it is already done with ImportHandler.importClass() method.
Comment 3 Jan Bartel 2015-02-13 00:23:30 UTC
The behaviour of Package.getPackage(name) is to use the classloader of the caller of the package to try and retrieve the package info. In osgi-land the classloader of the caller may not be allowed to see that package because of strict osgi rules about class visbility. The caller was loaded from the javax.el bundle or the jasper bundle. Neither of those bundles can possibly have manifest dependencies on the packages that will be dynamically loaded by the jsp being compiled.

As far as the trailing '/' is concerned, I think Konstantin is on-the-money as the bundles are probably packed jars and I can see code in the Equinox impl that uses ZipFile/ZipEntry to look into the bundle, and IIRC those methods tend to like a trailing '/' to signify a directory.

Comment 4 Konstantin Kolinko 2015-02-13 11:08:31 UTC
I removed the package existence check. The fix will be in 8.0.19 onwards.

r1659505 in trunk, r1659506 in 8.0.