You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ant.apache.org by an...@apache.org on 2006/09/20 05:51:10 UTC
svn commit: r448049 -
/ant/core/trunk/src/main/org/apache/tools/ant/types/resources/URLResource.java
Author: antoine
Date: Tue Sep 19 20:51:10 2006
New Revision: 448049
URL: http://svn.apache.org/viewvc?view=rev&rev=448049
Log:
add a close method for JarURLConnection, idea found in
http://svn.apache.org/repos/asf/jakarta/velocity/engine/trunk/src/java/org/apache/velocity/runtime/resource/loader/JarHolder.java
Modified:
ant/core/trunk/src/main/org/apache/tools/ant/types/resources/URLResource.java
Modified: ant/core/trunk/src/main/org/apache/tools/ant/types/resources/URLResource.java
URL: http://svn.apache.org/viewvc/ant/core/trunk/src/main/org/apache/tools/ant/types/resources/URLResource.java?view=diff&rev=448049&r1=448048&r2=448049
==============================================================================
--- ant/core/trunk/src/main/org/apache/tools/ant/types/resources/URLResource.java (original)
+++ ant/core/trunk/src/main/org/apache/tools/ant/types/resources/URLResource.java Tue Sep 19 20:51:10 2006
@@ -25,6 +25,8 @@
import java.net.URL;
import java.net.URLConnection;
import java.net.MalformedURLException;
+import java.net.JarURLConnection;
+import java.util.jar.JarFile;
import org.apache.tools.ant.Project;
import org.apache.tools.ant.BuildException;
@@ -202,7 +204,9 @@
}
try {
connect();
- return conn.getContentLength();
+ long contentlength = conn.getContentLength();
+ close();
+ return contentlength;
} catch (IOException e) {
return UNKNOWN_SIZE;
}
@@ -300,11 +304,29 @@
}
}
+ private void close() {
+ if (conn != null) {
+ try {
+ if (conn instanceof JarURLConnection) {
+ JarURLConnection juc = (JarURLConnection) conn;
+ JarFile jf = juc.getJarFile();
+ jf.close();
+ jf = null;
+ }
+ } catch (IOException exc) {
+
+ } finally {
+ conn = null;
+ }
+ }
+ }
+
/**
* Finalize this URLResource.
* @throws Throwable on error.
*/
protected void finalize() throws Throwable {
+ close();
conn = null;
super.finalize();
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: svn commit: r448049 - /ant/core/trunk/src/main/org/apache/tools/ant/types/resources/URLResource.java
Posted by Xavier Hanin <xa...@gmail.com>.
On 9/20/06, Antoine Levy-Lambert <an...@gmx.de> wrote:
>
> Hello,
Hello,
thanks for the patch, I have submitted parts of it.
>
> What I have submitted is closing the HttpURLConnection in close().
>
> The close method has become synchronised (not sure if it was necessary).
I think it's important to make the change to conn (conn=null) in a
synchronized block to avoid NPE in a multithreading environment.
When reading the contentlength, the connection is not closed after
> calling isExists(false).
>
> Calling close() after getContentLength() causes problems when running
> under JDK 1.5. Not under JDK 1.4.
> I did not look in the bugparade of Sun to try to understand why.
I should have a look at that, because I think it's a problem to keep the
connection opened. On resource intensive scripts using http urls there will
be failures due to that, at least that's what we've experimented with Ivy.
What are the tests to run? With which java version? I will try to see if I
can better figure out why it's failing, if you think it's worth my time :-)
Xavier
So the new version of URLResource does not call close after
> getContentLength() and after getLastModified().
> Nor after getOutputStream and getInputStream.
> Closing after getOutputStream made me fail a test where
> http://ant.apache.org is downloaded twice. I do not know why. This time
> the problem was under JDK 1.4
>
> Regards,
> Antoine
>
> Xavier Hanin wrote:
> > On 9/20/06, Matt Benson <gu...@yahoo.com> wrote:
> >>
> >> --- Antoine Levy-Lambert <an...@gmx.de> wrote:
> >>
> >> > Hello Xavier,
> >> >
> >> > the patch is welcome, but I do not see the
> >> > attachment ? maybe it
> >> > comes from my email reader.
> >>
> >> Nor did I. :)
> >
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
> For additional commands, e-mail: dev-help@ant.apache.org
>
>
Re: svn commit: r448049 - /ant/core/trunk/src/main/org/apache/tools/ant/types/resources/URLResource.java
Posted by Antoine Levy-Lambert <an...@gmx.de>.
Hello,
thanks for the patch, I have submitted parts of it.
What I have submitted is closing the HttpURLConnection in close().
The close method has become synchronised (not sure if it was necessary).
When reading the contentlength, the connection is not closed after
calling isExists(false).
Calling close() after getContentLength() causes problems when running
under JDK 1.5. Not under JDK 1.4.
I did not look in the bugparade of Sun to try to understand why.
So the new version of URLResource does not call close after
getContentLength() and after getLastModified().
Nor after getOutputStream and getInputStream.
Closing after getOutputStream made me fail a test where
http://ant.apache.org is downloaded twice. I do not know why. This time
the problem was under JDK 1.4
Regards,
Antoine
Xavier Hanin wrote:
> On 9/20/06, Matt Benson <gu...@yahoo.com> wrote:
>>
>> --- Antoine Levy-Lambert <an...@gmx.de> wrote:
>>
>> > Hello Xavier,
>> >
>> > the patch is welcome, but I do not see the
>> > attachment ? maybe it
>> > comes from my email reader.
>>
>> Nor did I. :)
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: svn commit: r448049 - /ant/core/trunk/src/main/org/apache/tools/ant/types/resources/URLResource.java
Posted by Xavier Hanin <xa...@gmail.com>.
On 9/20/06, Matt Benson <gu...@yahoo.com> wrote:
>
> --- Antoine Levy-Lambert <an...@gmx.de> wrote:
>
> > Hello Xavier,
> >
> > the patch is welcome, but I do not see the
> > attachment ? maybe it
> > comes from my email reader.
>
> Nor did I. :)
This is weird, I see it in gmail... Is it using some fancy thing to attach
files?
Anyway, here is the patch in plain text:
Index: src/main/org/apache/tools/ant/types/resources/URLResource.java
===================================================================
--- src/main/org/apache/tools/ant/types/resources/URLResource.java
(revision 448066)
+++ src/main/org/apache/tools/ant/types/resources/URLResource.java
(working copy)
@@ -22,6 +22,7 @@
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
+import java.net.HttpURLConnection;
import java.net.URL;
import java.net.URLConnection;
import java.net.MalformedURLException;
@@ -145,6 +146,28 @@
* @return true if this resource exists.
*/
public synchronized boolean isExists() {
+ return isExists(true);
+ }
+
+ /**
+ * Find out whether the URL exists, and close the connection
+ * opened to the URL if closeConnection is true.
+ *
+ * Note that this method does ensure that if:
+ * - the resource exists (if it returns true)
+ * - and if the current object is not a reference
+ * (isReference() returns false)
+ * - and if it was called with closeConnection to false,
+ *
+ * then the connection to the URL (stored in the conn
+ * private field) will be opened, and require to be closed
+ * by the caller.
+ *
+ * @param closeConnection true if the connection should be closed
+ * after the call, false if it should stay open.
+ * @return true if this resource exists.
+ */
+ private synchronized boolean isExists(boolean closeConnection) {
if (isReference()) {
return ((Resource) getCheckedRef()).isExists();
}
@@ -156,9 +179,14 @@
return true;
} catch (IOException e) {
return false;
+ } finally {
+ if (closeConnection) {
+ close();
+ }
}
}
+
/**
* Tells the modification time in milliseconds since 01.01.1970 .
*
@@ -169,10 +197,12 @@
if (isReference()) {
return ((Resource) getCheckedRef()).getLastModified();
}
- if (!isExists()) {
+ if (!isExists(false)) {
return 0L;
}
try {
+ // note that this call to connect() is not necessary:
+ // isExists returned true and isReference returned false
connect();
return conn.getLastModified();
} catch (IOException e) {
@@ -177,6 +207,8 @@
return conn.getLastModified();
} catch (IOException e) {
return 0L;
+ } finally {
+ close();
}
}
@@ -199,16 +231,18 @@
if (isReference()) {
return ((Resource) getCheckedRef()).getSize();
}
- if (!isExists()) {
+ if (!isExists(false)) {
return 0L;
}
try {
+ // note that this call to connect() is not necessary:
+ // isExists returned true and isReference returned false
connect();
- long contentlength = conn.getContentLength();
- close();
- return contentlength;
+ return conn.getContentLength();
} catch (IOException e) {
return UNKNOWN_SIZE;
+ } finally {
+ close();
}
}
@@ -260,7 +294,7 @@
try {
return conn.getInputStream();
} finally {
- conn = null;
+ close();
}
}
@@ -280,7 +314,7 @@
try {
return conn.getOutputStream();
} finally {
- conn = null;
+ close();
}
}
@@ -304,7 +338,15 @@
}
}
- private void close() {
+ /**
+ * Closes the URL connection if:
+ * - it is opened (i.e. the field conn is not null)
+ * - this type of URLConnection supports some sort of close mechanism
+ *
+ * This method ensures the field conn will be null after the call.
+ *
+ */
+ private synchronized void close() {
if (conn != null) {
try {
if (conn instanceof JarURLConnection) {
@@ -312,6 +354,8 @@
JarFile jf = juc.getJarFile();
jf.close();
jf = null;
+ } else if (conn instanceof HttpURLConnection) {
+ ((HttpURLConnection) conn).disconnect();
}
} catch (IOException exc) {
@@ -327,7 +371,7 @@
*/
protected void finalize() throws Throwable {
close();
- conn = null;
+ conn = null; // Note: already done by close();
super.finalize();
}
Re: svn commit: r448049 - /ant/core/trunk/src/main/org/apache/tools/ant/types/resources/URLResource.java
Posted by Matt Benson <gu...@yahoo.com>.
--- Antoine Levy-Lambert <an...@gmx.de> wrote:
> Hello Xavier,
>
> the patch is welcome, but I do not see the
> attachment ? maybe it
> comes from my email reader.
Nor did I. :)
-Matt
>
> Regards
>
> Antoine
> On Sep 20, 2006, at 8:42 AM, Xavier Hanin wrote:
>
> > Hi Matt and Antoine,
> >
> > While looking at your patch Antoine I had a look
> at the URLResource
> > source code, and I think it would need some
> improvement about
> > connection closing:
> > * your patch only takes care of closing the
> connection when calling
> > getSize(), but I think it should be closed
> whenever it is opened.
> > * there is another type of URLConnection which
> needs to be closed:
> > HttpURLConnection. We had problems with non closed
>
> > HttpURLConnection in first versions of Ivy, and a
> user contributed
> > a simple patch very similar to the one you have
> for closing a
> > JarConnection.
> >
> > Please find attached a patch solving these issues.
> Feel free to
> > modify whatever you want or keep only a small
> part, I don't know
> > Antinternals very well, so maybe what I've done is
> not very well
> > designed. I've tried to compensate with some
> comments.
> >
>
>
---------------------------------------------------------------------
> To unsubscribe, e-mail:
> dev-unsubscribe@ant.apache.org
> For additional commands, e-mail:
> dev-help@ant.apache.org
>
>
__________________________________________________
Do You Yahoo!?
Tired of spam? Yahoo! Mail has the best spam protection around
http://mail.yahoo.com
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: svn commit: r448049 - /ant/core/trunk/src/main/org/apache/tools/ant/types/resources/URLResource.java
Posted by Antoine Levy-Lambert <an...@gmx.de>.
Hello Xavier,
the patch is welcome, but I do not see the attachment ? maybe it
comes from my email reader.
Regards
Antoine
On Sep 20, 2006, at 8:42 AM, Xavier Hanin wrote:
> Hi Matt and Antoine,
>
> While looking at your patch Antoine I had a look at the URLResource
> source code, and I think it would need some improvement about
> connection closing:
> * your patch only takes care of closing the connection when calling
> getSize(), but I think it should be closed whenever it is opened.
> * there is another type of URLConnection which needs to be closed:
> HttpURLConnection. We had problems with non closed
> HttpURLConnection in first versions of Ivy, and a user contributed
> a simple patch very similar to the one you have for closing a
> JarConnection.
>
> Please find attached a patch solving these issues. Feel free to
> modify whatever you want or keep only a small part, I don't know
> Antinternals very well, so maybe what I've done is not very well
> designed. I've tried to compensate with some comments.
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: svn commit: r448049 - /ant/core/trunk/src/main/org/apache/tools/ant/types/resources/URLResource.java
Posted by Xavier Hanin <xa...@gmail.com>.
Hi Matt and Antoine,
While looking at your patch Antoine I had a look at the URLResource source
code, and I think it would need some improvement about connection closing:
* your patch only takes care of closing the connection when calling
getSize(), but I think it should be closed whenever it is opened.
* there is another type of URLConnection which needs to be closed:
HttpURLConnection. We had problems with non closed HttpURLConnection in
first versions of Ivy, and a user contributed a simple patch very similar to
the one you have for closing a JarConnection.
Please find attached a patch solving these issues. Feel free to modify
whatever you want or keep only a small part, I don't know Antinternals very
well, so maybe what I've done is not very well designed. I've tried to
compensate with some comments.
BTW this is my first patch to ant, feel free to tell me if I should use
another channel to send it.
Regards,
Xavier
On 9/19/06, Antoine Levy-Lambert <an...@gmx.de> wrote:
>
> Hi Matt,
>
> I found this while trying to find out why some testcases leave stuff
> behind.
> Google + the work of our velocity colleagues brought me a solution.
> Maybe whe should close this close method also on isExists()
>
> Regards,
>
> Antoine
>
> antoine@apache.org wrote:
> > Author: antoine
> > Date: Tue Sep 19 20:51:10 2006
> > New Revision: 448049
> >
> > URL: http://svn.apache.org/viewvc?view=rev&rev=448049
> > Log:
> > add a close method for JarURLConnection, idea found in
> >
> http://svn.apache.org/repos/asf/jakarta/velocity/engine/trunk/src/java/org/apache/velocity/runtime/resource/loader/JarHolder.java
> >
> > Modified:
> >
> ant/core/trunk/src/main/org/apache/tools/ant/types/resources/URLResource.java
> >
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
> For additional commands, e-mail: dev-help@ant.apache.org
>
>
Re: svn commit: r448049 - /ant/core/trunk/src/main/org/apache/tools/ant/types/resources/URLResource.java
Posted by Antoine Levy-Lambert <an...@gmx.de>.
Hi Matt,
I found this while trying to find out why some testcases leave stuff behind.
Google + the work of our velocity colleagues brought me a solution.
Maybe whe should close this close method also on isExists()
Regards,
Antoine
antoine@apache.org wrote:
> Author: antoine
> Date: Tue Sep 19 20:51:10 2006
> New Revision: 448049
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=448049
> Log:
> add a close method for JarURLConnection, idea found in
> http://svn.apache.org/repos/asf/jakarta/velocity/engine/trunk/src/java/org/apache/velocity/runtime/resource/loader/JarHolder.java
>
> Modified:
> ant/core/trunk/src/main/org/apache/tools/ant/types/resources/URLResource.java
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org