Hi Michael,
After reading carefully discussion thread, let me elaborate my
investigation and conclusion.
The 2nd version of Shirish's patch tries to address your
concern that
"Would it be possible to fix this within the context of whatever
loader is currently being invoked?". The new solution sticks to
using
Loader rather than JarLoader.
The call cl.close() in the jtreg test case, according to its spec
(URLClassLoader.close) should "close any files that were opened
by it
in case of jar". Its implementation code shows it closes any
opened
resources through api such as getResourceAsStream invoked by
client
code but doesn't take care of any resources opened by
findResource(String) or findResources(String). This implies that
findResource should return any resource found but should not
leave it
in open state. The key issue for a Loader.findResource() when
searching within a jar file does not follow this rule because
the code
combination "InputStream is = url.openStream(); is.close();" (in
URLClassPath.Loader.findResource()) leaves the jar file in open
state.
As Shirish pointed out, if useCaches is set to true, the
problem is
gone. It can be easily verified from code
JarURLInputStream.close()
defined in JarURLConnection.java.
My conclusion is that Shirish's first patch is reasonable
(except the
constructor change which I have not fully understood yet) because
choosing a JarLoader avoids unclosed resources after calling
URLClassLoader.getResource() and 2nd patch also makes sense as
explained above. The ramifications of these 2 patches need
deliberate
considerations but we still have to fix the issue after
weighing their
risks. Could you please shed your light on it?
Best regards,
Frank
On 8/25/2012 12:02 AM, Shirish Kuncolienkar wrote:
On 8/24/2012 5:39 PM, Michael McMahon wrote:
On 23/08/12 18:50, Shirish Kuncolienkar wrote:
Could I get the change reviewed please
This behavior is seen on Windows.
Logic in URLClassPath.getLoader() does not take care of an URL
which looks like "jar:file:/C:/test/xyz.jar!/". The logic
ends up
choosing a FileLoader instead of a JarLoader. JarLoader has
provision for closing file handles, so choosing a JarLoader
will
solve the problem. Secondly the constructor of JarLoader
blindly
adds a prefix and suffix to the provided URL to make it look
like a
jar URL. Changed the code here to conditionally append/prepend
The change set can be found at
http://cr.openjdk.java.net/~shirishk/7183373/webrev.0/
-Shirish
Shirish,
I have a slight concern that this would modify the Loader
class to
be used in some circumstances
completely independent of the requirements of
URLClassLoader.close(). This is very sensitive code.
Would it be possible to fix this within the context of whatever
loader is currently being invoked?
- Michael
Michael,
Thanks for the review comments. The second version of the fix is
uploaded at
http://cr.openjdk.java.net/~shirishk/7183373/webrev.2/
Could you please take a look at this one ?
Description of the fix:
URLClassPath.Loader.findResource() method opens a connection
to the
provided URL to test whether the URL is good. Here the Jar
file gets
opened but does not get closed because the created stream as
setUseCaches set to true.
Just out of curiosity I would like to know bit more on "some
circumstances completely independent of the requirements of
URLClassLoader.close()". I see that the Loader classes are
private in
nature and are being used within the context of the URLClassPath.
We create an instance of JarLoader for all the jars that are
on the
extension class loader path by adding "jar" , "!/" to the file
url
which comes as the input. The reason behind the first fix was
that if
we have a url like this why not use a JarLoader instance.
- Shirish
