You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by Sam Maloney <sa...@filogix.com> on 2003/02/19 23:23:34 UTC
[PATCH] Fix for bug 16458
Hi there,
As I am a new poster here, I will first describe myself and the situation. If
you wish to skip this, skip down to after the line '-----'.
In a very large project I am a senior on, I use to be using HTTPClient v0.3-3
(www.innovation.ch/java/HTTPClient/).
At the time I chose it, it was the superior client. However, because of the
facts:
a) It does not work properly with sending the request as a stream without
knowing the content length until stream.close(). (It claimed to work okay
with this).
b) After looking at the code to try to fix the problem, not only did I give up
trying to fix the problem, but I also gave up on the product :)
So anyways, hearing 2.0alpha of HttpClient was out, and supported both SSL
(needed) and Streams (very good), I decided to try it out.
I want to point out that I encountered bug 13463 early on, and after reading
the bugzilla db, I tried the patch attached to the end of it. I would just
like to give my vote to include it into CVS, as it fixes the problem (bug
13463) perfectly.
-----
As for bug 16458, I have fixed it.
It was a rather simple bug, and can be reproduced by closing the server side
socket while the client is still waiting for a response. This will cause the
client to take 100% cpu, and it will do so for ever and ever.
The fix is as follows (I have tested extensively any fixes I will post):
Index: HttpConnection.java
===================================================================
RCS file:
/home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/httpclient/HttpConnection.java,v
retrieving revision 1.44
diff -u -r1.44 HttpConnection.java
--- HttpConnection.java 13 Feb 2003 21:31:53 -0000 1.44
+++ HttpConnection.java 19 Feb 2003 21:27:26 -0000
@@ -128,6 +128,10 @@
StringBuffer buf = new StringBuffer();
int ch = inputStream.read();
+ if(ch == -1){
+ // End Of File!
+ return null; // Let caller know!
+ }
while (ch >= 0) {
if (ch == '\r') {
ch = inputStream.read();
I have another bugfix that I will post in my next message.
Thanks,
Sam Maloney <me...@sammaloney.com>
Re: [PATCH] Fix for bug 16458
Posted by Jeffrey Dever <js...@sympatico.ca>.
>
>
>In a very large project I am a senior on, I use to be using HTTPClient v0.3-3
>(www.innovation.ch/java/HTTPClient/).
>
>b) After looking at the code to try to fix the problem, not only did I give up
>trying to fix the problem, but I also gave up on the product :)
>
>
Yes, I went through exactly the same process. I needed preemptive
authorization which turned out to be a nightmare to add to the
innovation HTTPClient but oh so easy to add to Jakarta HttpClient.
>I want to point out that I encountered bug 13463 early on, and after reading
>the bugzilla db, I tried the patch attached to the end of it. I would just
>like to give my vote to include it into CVS, as it fixes the problem (bug
>13463) perfectly.
>
Its a pretty good sign when you start using a new product and they
already have fixes for some of the bugs you find! The development team
here working on HttpClient is better than any other team I have been
involved with in my corporate life.
>As for bug 16458, I have fixed it.
>
>It was a rather simple bug, and can be reproduced by closing the server side
>socket while the client is still waiting for a response. This will cause the
>client to take 100% cpu, and it will do so for ever and ever.
>
>
>
Its great you made that connection which would have gone unnoticed. I
would probablly have had to mark that bug as "works for me" otherwise as
it was apparently inadvertently fixed.
>I have another bugfix that I will post in my next message.
>
>
>
Looking forward to it.
Jandalf
HttpClient release prime.
Re: [PATCH] Fix for bug 16458
Posted by Jeffrey Dever <js...@sympatico.ca>.
>
>
>There is that one more bug I have fixed that is important to be included in
>final (perhaps beta even). I will post it soon.
>
>The next bug doesn't have a tracking number that I could find, but it results
>in the app using HttpClient to wait indefinatly for a connection that will
>never be released.
>
For clearly identified bugs, it is preferable create a bug report for it
an attach a patch to the bug itself for tracking purposes.
Also note the HttpClient powered page where you can have your
application listed if you wish. There are both closed source and open
source pages there.
http://jakarta.apache.org/commons/httpclient/applications.html
Jandalf.
Re: [PATCH] Fix for bug 16458
Posted by Sam Maloney <sa...@filogix.com>.
> Thank you so much for tracking it down. It's really appreciated.
No problem. When writing the adaptor for HttpClient to plug into ApacheSOAP
(we use both in our project), I instantly fell in love with the architecture
(as I said, I came from HTTPClient, which Jeffery Dever agrees with me is a
very scary and unloveable architecture :). My congrats go out to all involved
in this excellent creation.
With HttpClient I was able to stream the xml from the envelope while it is
being generated. Infact, the streaming works both ways from both sides
(tomcat on server side). This was not possible with HTTPClient, due to it not
handling not knowing the content length before it starts writing.
I doubt there will be ever be any HTTP Client for java that will compare to
this one.
>
> Would not it be a bit cleaner to fix this part, though? What's your
> opinion on that?
>
I thought the same thing at first, but there are two reasons I have found to
do the fix in ....<NEW EMAIL RECEIVED>
well, I just got your email about you finding those reasons yourself :)
Good to hear. I am looking forward to the first beta.
There is that one more bug I have fixed that is important to be included in
final (perhaps beta even). I will post it soon.
The next bug doesn't have a tracking number that I could find, but it results
in the app using HttpClient to wait indefinatly for a connection that will
never be released.
Sam
> while ((statusString != null) && !statusString.startsWith("HTTP/")) {
> statusString = conn.readLine();
> }
>
> Cheers
>
> Oleg
>
> On Thu, 2003-02-20 at 17:04, Sam Maloney wrote:
> > On Wednesday 19 February 2003 17:39, Oleg Kalnichevski wrote:
> > > Hi Sam
> > >
> > > I believe the bug has been fixed by now. I stumbled upon it a few days
> > > ago pretty much by chance
> > >
> > > http://www.mail-archive.com/commons-httpclient-dev%40jakarta.apache.org
> > >/msg 00536.html
> >
> > This fix does not fix the same problem. The fix linked to above will
> > prevent (byte)-1 from being appended to the buffer (a different bug), but
> > that is not the problem I/bug 16458 was having.
> >
> > I updated to CVS again, and there were some changes in this area (now
> > readLine calls HttpParser.readLine which calls HttpParser.readRawLine).
> > However, after testing again, I confirmed that the problem still exists
> > unaffected.
> >
> > Here is the problem:
> >
> > at
> > org.apache.commons.httpclient.HttpParser.readRawLine(HttpParser.java:46)
> > at
> > org.apache.commons.httpclient.HttpParser.readLine(HttpParser.java:81)
> > at
> > org.apache.commons.httpclient.HttpConnection.readLine(HttpConnection.java
> >:878) at
> > org.apache.commons.httpclient.HttpConnectionAdapter.readLine(MultiThreade
> >dHttpConnectionManager_fixed.java:730) at
> > org.apache.commons.httpclient.HttpMethodBase.readStatusLine(HttpMethodBas
> >e.java:1917)
> >
> > You can see that readStatusLine looks like this:
> >
> > <SNIP>
> > //read out the HTTP status string
> > String statusString = conn.readLine();
> > while ((statusString != null) && !statusString.startsWith("HTTP/")) {
> > statusString = conn.readLine();
> > }
> > if (statusString == null) {
> > // A null statusString means the connection was lost before we got a
> > // response. Try again.
> > throw new HttpRecoverableException("Error in parsing the status "
> > + " line from the response: unable to find line starting with"
> > + " \"HTTP/\"");
> > }
> > </SNIP>
> >
> > You can see from the above code, that until conn.readLine() returns null
> > or a string starting with "HTTP/", this piece of code will keep looping
> > indefinatly (what is taking 100% CPU). Right now, readLine never will
> > return null, only empty strings.
> >
> > I'm guessing readLine must have returned null at one point, as every
> > place that calls it expects null to be returned on closed connection.
> >
> > The way readLine must work for the calling code to work is:
> >
> > 1) Read data upto \r\n, return line. (currently happens)
> > 2) if EOF before finding \r\n, but we have data in our buffer, return the
> > buffer. (also happens)
> >
> > 3) if EOF before finding any data (buf.size()==0), then return null to
> > signal that no more data is possible, and caller should NOT call again.
> > (this is what patch adds)
> >
> > If we return empty string like is happening currently, then the caller
> > will not know NOT to call again. In this bugs case, the caller keeps
> > reading lines until it finds 'HTTP/'. Since the empty string doesn't
> > match that, the caller will keep trying to get the next line, and they
> > will just keep getting "".
> >
> > Here is my patch again, updated to apply to HttpParser (where the
> > readLine code has been moved to since my last patch). I have retested the
> > new patch and it is working in all my test cases, including the one to
> > reproduce the bug.
> >
> > Index: HttpParser.java
> > ===================================================================
> > RCS file:
> > /home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/ht
> >tpclient/HttpParser.java,v retrieving revision 1.1
> > diff -u -r1.1 HttpParser.java
> > --- HttpParser.java 16 Feb 2003 13:10:16 -0000 1.1
> > +++ HttpParser.java 20 Feb 2003 15:51:51 -0000
> > @@ -58,6 +58,10 @@
> > }
> > }
> > }
> > + if (buf.size() <= 0) {
> > + // Then we just started reading, but the stream is EOF
> > (closed). + return null; // Let caller know we got EOF BEFORE
> > any data. + }
> > if (WIRE_LOG.isDebugEnabled()) {
> > WIRE_LOG.debug("<< \"" + buf.toString() + (ch>0 ? "\"
> > [\\r\\n]" : ""));
> > }
> > @@ -79,6 +83,12 @@
> > public static String readLine(InputStream inputStream) throws
> > IOException {
> > LOG.trace("enter HttpConnection.readLine()");
> > byte[] rawdata = readRawLine(inputStream);
> > +
> > + if (rawdata == null) {
> > + // Then there was EOF before any data was read, we must let
> > caller know.
> > + return null;
> > + }
> > +
> > int len = rawdata.length;
> > if (( len >= 2) && (rawdata[len - 2] == '\r') && (rawdata[len -
> > 1] == '\n')) {
> > return HttpConstants.getString(rawdata, 0, rawdata.length -
> > 2);
> >
> > Cheers,
> > Sam Maloney
> >
> > > I was not aware that it might have fixed bug# 16458. Could you please
> > > take the newest CVS snapshout for a spin and let us know if the problem
> > > has indeed been eliminated?
> > >
> > > Other patches would be highly welcome
> > >
> > > Cheers
> > >
> > > Oleg
> > >
> > > On Wed, 2003-02-19 at 23:23, Sam Maloney wrote:
> > > > Hi there,
> > > >
> > > > As I am a new poster here, I will first describe myself and the
> > > > situation. If you wish to skip this, skip down to after the line
> > > > '-----'.
> > > >
> > > > In a very large project I am a senior on, I use to be using
> > > > HTTPClient v0.3-3 (www.innovation.ch/java/HTTPClient/).
> > > >
> > > > At the time I chose it, it was the superior client. However, because
> > > > of the facts:
> > > >
> > > > a) It does not work properly with sending the request as a stream
> > > > without knowing the content length until stream.close(). (It claimed
> > > > to work okay with this).
> > > >
> > > > b) After looking at the code to try to fix the problem, not only did
> > > > I give up trying to fix the problem, but I also gave up on the
> > > > product :)
> > > >
> > > > So anyways, hearing 2.0alpha of HttpClient was out, and supported
> > > > both SSL (needed) and Streams (very good), I decided to try it out.
> > > >
> > > > I want to point out that I encountered bug 13463 early on, and after
> > > > reading the bugzilla db, I tried the patch attached to the end of it.
> > > > I would just like to give my vote to include it into CVS, as it fixes
> > > > the problem (bug 13463) perfectly.
> > > >
> > > > -----
> > > >
> > > > As for bug 16458, I have fixed it.
> > > >
> > > > It was a rather simple bug, and can be reproduced by closing the
> > > > server side socket while the client is still waiting for a response.
> > > > This will cause the client to take 100% cpu, and it will do so for
> > > > ever and ever.
> > > >
> > > > The fix is as follows (I have tested extensively any fixes I will
> > > > post):
> > > >
> > > > Index: HttpConnection.java
> > > > ===================================================================
> > > > RCS file:
> > > > /home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/common
> > > >s/ht tpclient/HttpConnection.java,v retrieving revision 1.44
> > > > diff -u -r1.44 HttpConnection.java
> > > > --- HttpConnection.java 13 Feb 2003 21:31:53 -0000 1.44
> > > > +++ HttpConnection.java 19 Feb 2003 21:27:26 -0000
> > > > @@ -128,6 +128,10 @@
> > > >
> > > > StringBuffer buf = new StringBuffer();
> > > > int ch = inputStream.read();
> > > > + if(ch == -1){
> > > > + // End Of File!
> > > > + return null; // Let caller know!
> > > > + }
> > > > while (ch >= 0) {
> > > > if (ch == '\r') {
> > > > ch = inputStream.read();
> > > >
> > > > I have another bugfix that I will post in my next message.
> > > >
> > > > Thanks,
> > > > Sam Maloney <me...@sammaloney.com>
> > > >
> > > > ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail:
> > > > commons-httpclient-dev-unsubscribe@jakarta.apache.org For additional
> > > > commands, e-mail: commons-httpclient-dev-help@jakarta.apache.org
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail:
> > > commons-httpclient-dev-unsubscribe@jakarta.apache.org For additional
> > > commands, e-mail: commons-httpclient-dev-help@jakarta.apache.org
Re: [PATCH] Fix for bug 16458
Posted by Oleg Kalnichevski <o....@dplanet.ch>.
No, it would not. I was wrong in my assumption. I am going to apply
Sam's patch and commit it after having run all the test cases
Oleg
On Thu, 2003-02-20 at 17:18, Oleg Kalnichevski wrote:
> Sam
>
> Thank you so much for tracking it down. It's really appreciated.
>
> Would not it be a bit cleaner to fix this part, though? What's your
> opinion on that?
>
> while ((statusString != null) && !statusString.startsWith("HTTP/")) {
> statusString = conn.readLine();
> }
>
> Cheers
>
> Oleg
>
> On Thu, 2003-02-20 at 17:04, Sam Maloney wrote:
> > On Wednesday 19 February 2003 17:39, Oleg Kalnichevski wrote:
> > > Hi Sam
> > >
> > > I believe the bug has been fixed by now. I stumbled upon it a few days
> > > ago pretty much by chance
> > >
> > > http://www.mail-archive.com/commons-httpclient-dev%40jakarta.apache.org/msg
> > >00536.html
> > >
> > This fix does not fix the same problem. The fix linked to above will prevent
> > (byte)-1 from being appended to the buffer (a different bug), but that is not
> > the problem I/bug 16458 was having.
> >
> > I updated to CVS again, and there were some changes in this area (now readLine
> > calls HttpParser.readLine which calls HttpParser.readRawLine). However, after
> > testing again, I confirmed that the problem still exists unaffected.
> >
> > Here is the problem:
> >
> > at
> > org.apache.commons.httpclient.HttpParser.readRawLine(HttpParser.java:46)
> > at
> > org.apache.commons.httpclient.HttpParser.readLine(HttpParser.java:81)
> > at
> > org.apache.commons.httpclient.HttpConnection.readLine(HttpConnection.java:878)
> > at
> > org.apache.commons.httpclient.HttpConnectionAdapter.readLine(MultiThreadedHttpConnectionManager_fixed.java:730)
> > at
> > org.apache.commons.httpclient.HttpMethodBase.readStatusLine(HttpMethodBase.java:1917)
> >
> > You can see that readStatusLine looks like this:
> >
> > <SNIP>
> > //read out the HTTP status string
> > String statusString = conn.readLine();
> > while ((statusString != null) && !statusString.startsWith("HTTP/")) {
> > statusString = conn.readLine();
> > }
> > if (statusString == null) {
> > // A null statusString means the connection was lost before we got a
> > // response. Try again.
> > throw new HttpRecoverableException("Error in parsing the status "
> > + " line from the response: unable to find line starting with"
> > + " \"HTTP/\"");
> > }
> > </SNIP>
> >
> > You can see from the above code, that until conn.readLine() returns null or a
> > string starting with "HTTP/", this piece of code will keep looping
> > indefinatly (what is taking 100% CPU). Right now, readLine never will return
> > null, only empty strings.
> >
> > I'm guessing readLine must have returned null at one point, as every place
> > that calls it expects null to be returned on closed connection.
> >
> > The way readLine must work for the calling code to work is:
> >
> > 1) Read data upto \r\n, return line. (currently happens)
> > 2) if EOF before finding \r\n, but we have data in our buffer, return the
> > buffer. (also happens)
> >
> > 3) if EOF before finding any data (buf.size()==0), then return null to signal
> > that no more data is possible, and caller should NOT call again. (this is
> > what patch adds)
> >
> > If we return empty string like is happening currently, then the caller will
> > not know NOT to call again. In this bugs case, the caller keeps reading lines
> > until it finds 'HTTP/'. Since the empty string doesn't match that, the caller
> > will keep trying to get the next line, and they will just keep getting "".
> >
> > Here is my patch again, updated to apply to HttpParser (where the readLine
> > code has been moved to since my last patch). I have retested the new patch
> > and it is working in all my test cases, including the one to reproduce the
> > bug.
> >
> > Index: HttpParser.java
> > ===================================================================
> > RCS file:
> > /home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/httpclient/HttpParser.java,v
> > retrieving revision 1.1
> > diff -u -r1.1 HttpParser.java
> > --- HttpParser.java 16 Feb 2003 13:10:16 -0000 1.1
> > +++ HttpParser.java 20 Feb 2003 15:51:51 -0000
> > @@ -58,6 +58,10 @@
> > }
> > }
> > }
> > + if (buf.size() <= 0) {
> > + // Then we just started reading, but the stream is EOF (closed).
> > + return null; // Let caller know we got EOF BEFORE any data.
> > + }
> > if (WIRE_LOG.isDebugEnabled()) {
> > WIRE_LOG.debug("<< \"" + buf.toString() + (ch>0 ? "\" [\\r\\n]" :
> > ""));
> > }
> > @@ -79,6 +83,12 @@
> > public static String readLine(InputStream inputStream) throws IOException
> > {
> > LOG.trace("enter HttpConnection.readLine()");
> > byte[] rawdata = readRawLine(inputStream);
> > +
> > + if (rawdata == null) {
> > + // Then there was EOF before any data was read, we must let
> > caller know.
> > + return null;
> > + }
> > +
> > int len = rawdata.length;
> > if (( len >= 2) && (rawdata[len - 2] == '\r') && (rawdata[len - 1] ==
> > '\n')) {
> > return HttpConstants.getString(rawdata, 0, rawdata.length - 2);
> >
> > Cheers,
> > Sam Maloney
> >
> > > I was not aware that it might have fixed bug# 16458. Could you please
> > > take the newest CVS snapshout for a spin and let us know if the problem
> > > has indeed been eliminated?
> > >
> > > Other patches would be highly welcome
> > >
> > > Cheers
> > >
> > > Oleg
> > >
> > > On Wed, 2003-02-19 at 23:23, Sam Maloney wrote:
> > > > Hi there,
> > > >
> > > > As I am a new poster here, I will first describe myself and the
> > > > situation. If you wish to skip this, skip down to after the line '-----'.
> > > >
> > > > In a very large project I am a senior on, I use to be using HTTPClient
> > > > v0.3-3 (www.innovation.ch/java/HTTPClient/).
> > > >
> > > > At the time I chose it, it was the superior client. However, because of
> > > > the facts:
> > > >
> > > > a) It does not work properly with sending the request as a stream without
> > > > knowing the content length until stream.close(). (It claimed to work okay
> > > > with this).
> > > >
> > > > b) After looking at the code to try to fix the problem, not only did I
> > > > give up trying to fix the problem, but I also gave up on the product :)
> > > >
> > > > So anyways, hearing 2.0alpha of HttpClient was out, and supported both
> > > > SSL (needed) and Streams (very good), I decided to try it out.
> > > >
> > > > I want to point out that I encountered bug 13463 early on, and after
> > > > reading the bugzilla db, I tried the patch attached to the end of it. I
> > > > would just like to give my vote to include it into CVS, as it fixes the
> > > > problem (bug 13463) perfectly.
> > > >
> > > > -----
> > > >
> > > > As for bug 16458, I have fixed it.
> > > >
> > > > It was a rather simple bug, and can be reproduced by closing the server
> > > > side socket while the client is still waiting for a response. This will
> > > > cause the client to take 100% cpu, and it will do so for ever and ever.
> > > >
> > > > The fix is as follows (I have tested extensively any fixes I will post):
> > > >
> > > > Index: HttpConnection.java
> > > > ===================================================================
> > > > RCS file:
> > > > /home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/ht
> > > >tpclient/HttpConnection.java,v retrieving revision 1.44
> > > > diff -u -r1.44 HttpConnection.java
> > > > --- HttpConnection.java 13 Feb 2003 21:31:53 -0000 1.44
> > > > +++ HttpConnection.java 19 Feb 2003 21:27:26 -0000
> > > > @@ -128,6 +128,10 @@
> > > >
> > > > StringBuffer buf = new StringBuffer();
> > > > int ch = inputStream.read();
> > > > + if(ch == -1){
> > > > + // End Of File!
> > > > + return null; // Let caller know!
> > > > + }
> > > > while (ch >= 0) {
> > > > if (ch == '\r') {
> > > > ch = inputStream.read();
> > > >
> > > > I have another bugfix that I will post in my next message.
> > > >
> > > > Thanks,
> > > > Sam Maloney <me...@sammaloney.com>
> > > >
> > > > ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail:
> > > > commons-httpclient-dev-unsubscribe@jakarta.apache.org For additional
> > > > commands, e-mail: commons-httpclient-dev-help@jakarta.apache.org
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail:
> > > commons-httpclient-dev-unsubscribe@jakarta.apache.org For additional
> > > commands, e-mail: commons-httpclient-dev-help@jakarta.apache.org
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-httpclient-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-httpclient-dev-help@jakarta.apache.org
>
Re: [PATCH] Fix for bug 16458
Posted by Oleg Kalnichevski <o....@dplanet.ch>.
Sam
Thank you so much for tracking it down. It's really appreciated.
Would not it be a bit cleaner to fix this part, though? What's your
opinion on that?
while ((statusString != null) && !statusString.startsWith("HTTP/")) {
statusString = conn.readLine();
}
Cheers
Oleg
On Thu, 2003-02-20 at 17:04, Sam Maloney wrote:
> On Wednesday 19 February 2003 17:39, Oleg Kalnichevski wrote:
> > Hi Sam
> >
> > I believe the bug has been fixed by now. I stumbled upon it a few days
> > ago pretty much by chance
> >
> > http://www.mail-archive.com/commons-httpclient-dev%40jakarta.apache.org/msg
> >00536.html
> >
> This fix does not fix the same problem. The fix linked to above will prevent
> (byte)-1 from being appended to the buffer (a different bug), but that is not
> the problem I/bug 16458 was having.
>
> I updated to CVS again, and there were some changes in this area (now readLine
> calls HttpParser.readLine which calls HttpParser.readRawLine). However, after
> testing again, I confirmed that the problem still exists unaffected.
>
> Here is the problem:
>
> at
> org.apache.commons.httpclient.HttpParser.readRawLine(HttpParser.java:46)
> at
> org.apache.commons.httpclient.HttpParser.readLine(HttpParser.java:81)
> at
> org.apache.commons.httpclient.HttpConnection.readLine(HttpConnection.java:878)
> at
> org.apache.commons.httpclient.HttpConnectionAdapter.readLine(MultiThreadedHttpConnectionManager_fixed.java:730)
> at
> org.apache.commons.httpclient.HttpMethodBase.readStatusLine(HttpMethodBase.java:1917)
>
> You can see that readStatusLine looks like this:
>
> <SNIP>
> //read out the HTTP status string
> String statusString = conn.readLine();
> while ((statusString != null) && !statusString.startsWith("HTTP/")) {
> statusString = conn.readLine();
> }
> if (statusString == null) {
> // A null statusString means the connection was lost before we got a
> // response. Try again.
> throw new HttpRecoverableException("Error in parsing the status "
> + " line from the response: unable to find line starting with"
> + " \"HTTP/\"");
> }
> </SNIP>
>
> You can see from the above code, that until conn.readLine() returns null or a
> string starting with "HTTP/", this piece of code will keep looping
> indefinatly (what is taking 100% CPU). Right now, readLine never will return
> null, only empty strings.
>
> I'm guessing readLine must have returned null at one point, as every place
> that calls it expects null to be returned on closed connection.
>
> The way readLine must work for the calling code to work is:
>
> 1) Read data upto \r\n, return line. (currently happens)
> 2) if EOF before finding \r\n, but we have data in our buffer, return the
> buffer. (also happens)
>
> 3) if EOF before finding any data (buf.size()==0), then return null to signal
> that no more data is possible, and caller should NOT call again. (this is
> what patch adds)
>
> If we return empty string like is happening currently, then the caller will
> not know NOT to call again. In this bugs case, the caller keeps reading lines
> until it finds 'HTTP/'. Since the empty string doesn't match that, the caller
> will keep trying to get the next line, and they will just keep getting "".
>
> Here is my patch again, updated to apply to HttpParser (where the readLine
> code has been moved to since my last patch). I have retested the new patch
> and it is working in all my test cases, including the one to reproduce the
> bug.
>
> Index: HttpParser.java
> ===================================================================
> RCS file:
> /home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/httpclient/HttpParser.java,v
> retrieving revision 1.1
> diff -u -r1.1 HttpParser.java
> --- HttpParser.java 16 Feb 2003 13:10:16 -0000 1.1
> +++ HttpParser.java 20 Feb 2003 15:51:51 -0000
> @@ -58,6 +58,10 @@
> }
> }
> }
> + if (buf.size() <= 0) {
> + // Then we just started reading, but the stream is EOF (closed).
> + return null; // Let caller know we got EOF BEFORE any data.
> + }
> if (WIRE_LOG.isDebugEnabled()) {
> WIRE_LOG.debug("<< \"" + buf.toString() + (ch>0 ? "\" [\\r\\n]" :
> ""));
> }
> @@ -79,6 +83,12 @@
> public static String readLine(InputStream inputStream) throws IOException
> {
> LOG.trace("enter HttpConnection.readLine()");
> byte[] rawdata = readRawLine(inputStream);
> +
> + if (rawdata == null) {
> + // Then there was EOF before any data was read, we must let
> caller know.
> + return null;
> + }
> +
> int len = rawdata.length;
> if (( len >= 2) && (rawdata[len - 2] == '\r') && (rawdata[len - 1] ==
> '\n')) {
> return HttpConstants.getString(rawdata, 0, rawdata.length - 2);
>
> Cheers,
> Sam Maloney
>
> > I was not aware that it might have fixed bug# 16458. Could you please
> > take the newest CVS snapshout for a spin and let us know if the problem
> > has indeed been eliminated?
> >
> > Other patches would be highly welcome
> >
> > Cheers
> >
> > Oleg
> >
> > On Wed, 2003-02-19 at 23:23, Sam Maloney wrote:
> > > Hi there,
> > >
> > > As I am a new poster here, I will first describe myself and the
> > > situation. If you wish to skip this, skip down to after the line '-----'.
> > >
> > > In a very large project I am a senior on, I use to be using HTTPClient
> > > v0.3-3 (www.innovation.ch/java/HTTPClient/).
> > >
> > > At the time I chose it, it was the superior client. However, because of
> > > the facts:
> > >
> > > a) It does not work properly with sending the request as a stream without
> > > knowing the content length until stream.close(). (It claimed to work okay
> > > with this).
> > >
> > > b) After looking at the code to try to fix the problem, not only did I
> > > give up trying to fix the problem, but I also gave up on the product :)
> > >
> > > So anyways, hearing 2.0alpha of HttpClient was out, and supported both
> > > SSL (needed) and Streams (very good), I decided to try it out.
> > >
> > > I want to point out that I encountered bug 13463 early on, and after
> > > reading the bugzilla db, I tried the patch attached to the end of it. I
> > > would just like to give my vote to include it into CVS, as it fixes the
> > > problem (bug 13463) perfectly.
> > >
> > > -----
> > >
> > > As for bug 16458, I have fixed it.
> > >
> > > It was a rather simple bug, and can be reproduced by closing the server
> > > side socket while the client is still waiting for a response. This will
> > > cause the client to take 100% cpu, and it will do so for ever and ever.
> > >
> > > The fix is as follows (I have tested extensively any fixes I will post):
> > >
> > > Index: HttpConnection.java
> > > ===================================================================
> > > RCS file:
> > > /home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/ht
> > >tpclient/HttpConnection.java,v retrieving revision 1.44
> > > diff -u -r1.44 HttpConnection.java
> > > --- HttpConnection.java 13 Feb 2003 21:31:53 -0000 1.44
> > > +++ HttpConnection.java 19 Feb 2003 21:27:26 -0000
> > > @@ -128,6 +128,10 @@
> > >
> > > StringBuffer buf = new StringBuffer();
> > > int ch = inputStream.read();
> > > + if(ch == -1){
> > > + // End Of File!
> > > + return null; // Let caller know!
> > > + }
> > > while (ch >= 0) {
> > > if (ch == '\r') {
> > > ch = inputStream.read();
> > >
> > > I have another bugfix that I will post in my next message.
> > >
> > > Thanks,
> > > Sam Maloney <me...@sammaloney.com>
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail:
> > > commons-httpclient-dev-unsubscribe@jakarta.apache.org For additional
> > > commands, e-mail: commons-httpclient-dev-help@jakarta.apache.org
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail:
> > commons-httpclient-dev-unsubscribe@jakarta.apache.org For additional
> > commands, e-mail: commons-httpclient-dev-help@jakarta.apache.org
>
Re: [PATCH] Fix for bug 16458
Posted by Oleg Kalnichevski <o....@dplanet.ch>.
Committed
On Thu, 2003-02-20 at 17:32, Jeffrey Dever wrote:
> Oleg, this fix will need to go in before we drop alpha3.
>
> Sam Maloney wrote:
>
> >On Wednesday 19 February 2003 17:39, Oleg Kalnichevski wrote:
> >
> >
> >>Hi Sam
> >>
> >>I believe the bug has been fixed by now. I stumbled upon it a few days
> >>ago pretty much by chance
> >>
> >>http://www.mail-archive.com/commons-httpclient-dev%40jakarta.apache.org/msg
> >>00536.html
> >>
> >>
> >>
> >This fix does not fix the same problem. The fix linked to above will prevent
> >(byte)-1 from being appended to the buffer (a different bug), but that is not
> >the problem I/bug 16458 was having.
> >
> >I updated to CVS again, and there were some changes in this area (now readLine
> >calls HttpParser.readLine which calls HttpParser.readRawLine). However, after
> >testing again, I confirmed that the problem still exists unaffected.
> >
> >Here is the problem:
> >
> > at
> >org.apache.commons.httpclient.HttpParser.readRawLine(HttpParser.java:46)
> > at
> >org.apache.commons.httpclient.HttpParser.readLine(HttpParser.java:81)
> > at
> >org.apache.commons.httpclient.HttpConnection.readLine(HttpConnection.java:878)
> > at
> >org.apache.commons.httpclient.HttpConnectionAdapter.readLine(MultiThreadedHttpConnectionManager_fixed.java:730)
> > at
> >org.apache.commons.httpclient.HttpMethodBase.readStatusLine(HttpMethodBase.java:1917)
> >
> >You can see that readStatusLine looks like this:
> >
> ><SNIP>
> > //read out the HTTP status string
> > String statusString = conn.readLine();
> > while ((statusString != null) && !statusString.startsWith("HTTP/")) {
> > statusString = conn.readLine();
> > }
> > if (statusString == null) {
> > // A null statusString means the connection was lost before we got a
> > // response. Try again.
> > throw new HttpRecoverableException("Error in parsing the status "
> > + " line from the response: unable to find line starting with"
> > + " \"HTTP/\"");
> > }
> ></SNIP>
> >
> >You can see from the above code, that until conn.readLine() returns null or a
> >string starting with "HTTP/", this piece of code will keep looping
> >indefinatly (what is taking 100% CPU). Right now, readLine never will return
> >null, only empty strings.
> >
> >I'm guessing readLine must have returned null at one point, as every place
> >that calls it expects null to be returned on closed connection.
> >
> >The way readLine must work for the calling code to work is:
> >
> >1) Read data upto \r\n, return line. (currently happens)
> >2) if EOF before finding \r\n, but we have data in our buffer, return the
> >buffer. (also happens)
> >
> >3) if EOF before finding any data (buf.size()==0), then return null to signal
> >that no more data is possible, and caller should NOT call again. (this is
> >what patch adds)
> >
> >If we return empty string like is happening currently, then the caller will
> >not know NOT to call again. In this bugs case, the caller keeps reading lines
> >until it finds 'HTTP/'. Since the empty string doesn't match that, the caller
> >will keep trying to get the next line, and they will just keep getting "".
> >
> >Here is my patch again, updated to apply to HttpParser (where the readLine
> >code has been moved to since my last patch). I have retested the new patch
> >and it is working in all my test cases, including the one to reproduce the
> >bug.
> >
> >Index: HttpParser.java
> >===================================================================
> >RCS file:
> >/home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/httpclient/HttpParser.java,v
> >retrieving revision 1.1
> >diff -u -r1.1 HttpParser.java
> >--- HttpParser.java 16 Feb 2003 13:10:16 -0000 1.1
> >+++ HttpParser.java 20 Feb 2003 15:51:51 -0000
> >@@ -58,6 +58,10 @@
> > }
> > }
> > }
> >+ if (buf.size() <= 0) {
> >+ // Then we just started reading, but the stream is EOF (closed).
> >+ return null; // Let caller know we got EOF BEFORE any data.
> >+ }
> > if (WIRE_LOG.isDebugEnabled()) {
> > WIRE_LOG.debug("<< \"" + buf.toString() + (ch>0 ? "\" [\\r\\n]" :
> >""));
> > }
> >@@ -79,6 +83,12 @@
> > public static String readLine(InputStream inputStream) throws IOException
> >{
> > LOG.trace("enter HttpConnection.readLine()");
> > byte[] rawdata = readRawLine(inputStream);
> >+
> >+ if (rawdata == null) {
> >+ // Then there was EOF before any data was read, we must let
> >caller know.
> >+ return null;
> >+ }
> >+
> > int len = rawdata.length;
> > if (( len >= 2) && (rawdata[len - 2] == '\r') && (rawdata[len - 1] ==
> >'\n')) {
> > return HttpConstants.getString(rawdata, 0, rawdata.length - 2);
> >
> >Cheers,
> >Sam Maloney
> >
> >
> >
> >>I was not aware that it might have fixed bug# 16458. Could you please
> >>take the newest CVS snapshout for a spin and let us know if the problem
> >>has indeed been eliminated?
> >>
> >>Other patches would be highly welcome
> >>
> >>Cheers
> >>
> >>Oleg
> >>
> >>On Wed, 2003-02-19 at 23:23, Sam Maloney wrote:
> >>
> >>
> >>>Hi there,
> >>>
> >>>As I am a new poster here, I will first describe myself and the
> >>>situation. If you wish to skip this, skip down to after the line '-----'.
> >>>
> >>>In a very large project I am a senior on, I use to be using HTTPClient
> >>>v0.3-3 (www.innovation.ch/java/HTTPClient/).
> >>>
> >>>At the time I chose it, it was the superior client. However, because of
> >>>the facts:
> >>>
> >>>a) It does not work properly with sending the request as a stream without
> >>>knowing the content length until stream.close(). (It claimed to work okay
> >>>with this).
> >>>
> >>>b) After looking at the code to try to fix the problem, not only did I
> >>>give up trying to fix the problem, but I also gave up on the product :)
> >>>
> >>>So anyways, hearing 2.0alpha of HttpClient was out, and supported both
> >>>SSL (needed) and Streams (very good), I decided to try it out.
> >>>
> >>>I want to point out that I encountered bug 13463 early on, and after
> >>>reading the bugzilla db, I tried the patch attached to the end of it. I
> >>>would just like to give my vote to include it into CVS, as it fixes the
> >>>problem (bug 13463) perfectly.
> >>>
> >>>-----
> >>>
> >>>As for bug 16458, I have fixed it.
> >>>
> >>>It was a rather simple bug, and can be reproduced by closing the server
> >>>side socket while the client is still waiting for a response. This will
> >>>cause the client to take 100% cpu, and it will do so for ever and ever.
> >>>
> >>>The fix is as follows (I have tested extensively any fixes I will post):
> >>>
> >>>Index: HttpConnection.java
> >>>===================================================================
> >>>RCS file:
> >>>/home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/ht
> >>>tpclient/HttpConnection.java,v retrieving revision 1.44
> >>>diff -u -r1.44 HttpConnection.java
> >>>--- HttpConnection.java 13 Feb 2003 21:31:53 -0000 1.44
> >>>+++ HttpConnection.java 19 Feb 2003 21:27:26 -0000
> >>>@@ -128,6 +128,10 @@
> >>>
> >>> StringBuffer buf = new StringBuffer();
> >>> int ch = inputStream.read();
> >>>+ if(ch == -1){
> >>>+ // End Of File!
> >>>+ return null; // Let caller know!
> >>>+ }
> >>> while (ch >= 0) {
> >>> if (ch == '\r') {
> >>> ch = inputStream.read();
> >>>
> >>>I have another bugfix that I will post in my next message.
> >>>
> >>>Thanks,
> >>>Sam Maloney <me...@sammaloney.com>
> >>>
> >>>---------------------------------------------------------------------
> >>>To unsubscribe, e-mail:
> >>>commons-httpclient-dev-unsubscribe@jakarta.apache.org For additional
> >>>commands, e-mail: commons-httpclient-dev-help@jakarta.apache.org
> >>>
> >>>
> >>---------------------------------------------------------------------
> >>To unsubscribe, e-mail:
> >>commons-httpclient-dev-unsubscribe@jakarta.apache.org For additional
> >>commands, e-mail: commons-httpclient-dev-help@jakarta.apache.org
> >>
> >>
> >
> >
> >---------------------------------------------------------------------
> >To unsubscribe, e-mail: commons-httpclient-dev-unsubscribe@jakarta.apache.org
> >For additional commands, e-mail: commons-httpclient-dev-help@jakarta.apache.org
> >
> >
> >
> >
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-httpclient-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-httpclient-dev-help@jakarta.apache.org
>
Re: [PATCH] Fix for bug 16458
Posted by Jeffrey Dever <js...@sympatico.ca>.
Oleg, this fix will need to go in before we drop alpha3.
Sam Maloney wrote:
>On Wednesday 19 February 2003 17:39, Oleg Kalnichevski wrote:
>
>
>>Hi Sam
>>
>>I believe the bug has been fixed by now. I stumbled upon it a few days
>>ago pretty much by chance
>>
>>http://www.mail-archive.com/commons-httpclient-dev%40jakarta.apache.org/msg
>>00536.html
>>
>>
>>
>This fix does not fix the same problem. The fix linked to above will prevent
>(byte)-1 from being appended to the buffer (a different bug), but that is not
>the problem I/bug 16458 was having.
>
>I updated to CVS again, and there were some changes in this area (now readLine
>calls HttpParser.readLine which calls HttpParser.readRawLine). However, after
>testing again, I confirmed that the problem still exists unaffected.
>
>Here is the problem:
>
> at
>org.apache.commons.httpclient.HttpParser.readRawLine(HttpParser.java:46)
> at
>org.apache.commons.httpclient.HttpParser.readLine(HttpParser.java:81)
> at
>org.apache.commons.httpclient.HttpConnection.readLine(HttpConnection.java:878)
> at
>org.apache.commons.httpclient.HttpConnectionAdapter.readLine(MultiThreadedHttpConnectionManager_fixed.java:730)
> at
>org.apache.commons.httpclient.HttpMethodBase.readStatusLine(HttpMethodBase.java:1917)
>
>You can see that readStatusLine looks like this:
>
><SNIP>
> //read out the HTTP status string
> String statusString = conn.readLine();
> while ((statusString != null) && !statusString.startsWith("HTTP/")) {
> statusString = conn.readLine();
> }
> if (statusString == null) {
> // A null statusString means the connection was lost before we got a
> // response. Try again.
> throw new HttpRecoverableException("Error in parsing the status "
> + " line from the response: unable to find line starting with"
> + " \"HTTP/\"");
> }
></SNIP>
>
>You can see from the above code, that until conn.readLine() returns null or a
>string starting with "HTTP/", this piece of code will keep looping
>indefinatly (what is taking 100% CPU). Right now, readLine never will return
>null, only empty strings.
>
>I'm guessing readLine must have returned null at one point, as every place
>that calls it expects null to be returned on closed connection.
>
>The way readLine must work for the calling code to work is:
>
>1) Read data upto \r\n, return line. (currently happens)
>2) if EOF before finding \r\n, but we have data in our buffer, return the
>buffer. (also happens)
>
>3) if EOF before finding any data (buf.size()==0), then return null to signal
>that no more data is possible, and caller should NOT call again. (this is
>what patch adds)
>
>If we return empty string like is happening currently, then the caller will
>not know NOT to call again. In this bugs case, the caller keeps reading lines
>until it finds 'HTTP/'. Since the empty string doesn't match that, the caller
>will keep trying to get the next line, and they will just keep getting "".
>
>Here is my patch again, updated to apply to HttpParser (where the readLine
>code has been moved to since my last patch). I have retested the new patch
>and it is working in all my test cases, including the one to reproduce the
>bug.
>
>Index: HttpParser.java
>===================================================================
>RCS file:
>/home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/httpclient/HttpParser.java,v
>retrieving revision 1.1
>diff -u -r1.1 HttpParser.java
>--- HttpParser.java 16 Feb 2003 13:10:16 -0000 1.1
>+++ HttpParser.java 20 Feb 2003 15:51:51 -0000
>@@ -58,6 +58,10 @@
> }
> }
> }
>+ if (buf.size() <= 0) {
>+ // Then we just started reading, but the stream is EOF (closed).
>+ return null; // Let caller know we got EOF BEFORE any data.
>+ }
> if (WIRE_LOG.isDebugEnabled()) {
> WIRE_LOG.debug("<< \"" + buf.toString() + (ch>0 ? "\" [\\r\\n]" :
>""));
> }
>@@ -79,6 +83,12 @@
> public static String readLine(InputStream inputStream) throws IOException
>{
> LOG.trace("enter HttpConnection.readLine()");
> byte[] rawdata = readRawLine(inputStream);
>+
>+ if (rawdata == null) {
>+ // Then there was EOF before any data was read, we must let
>caller know.
>+ return null;
>+ }
>+
> int len = rawdata.length;
> if (( len >= 2) && (rawdata[len - 2] == '\r') && (rawdata[len - 1] ==
>'\n')) {
> return HttpConstants.getString(rawdata, 0, rawdata.length - 2);
>
>Cheers,
>Sam Maloney
>
>
>
>>I was not aware that it might have fixed bug# 16458. Could you please
>>take the newest CVS snapshout for a spin and let us know if the problem
>>has indeed been eliminated?
>>
>>Other patches would be highly welcome
>>
>>Cheers
>>
>>Oleg
>>
>>On Wed, 2003-02-19 at 23:23, Sam Maloney wrote:
>>
>>
>>>Hi there,
>>>
>>>As I am a new poster here, I will first describe myself and the
>>>situation. If you wish to skip this, skip down to after the line '-----'.
>>>
>>>In a very large project I am a senior on, I use to be using HTTPClient
>>>v0.3-3 (www.innovation.ch/java/HTTPClient/).
>>>
>>>At the time I chose it, it was the superior client. However, because of
>>>the facts:
>>>
>>>a) It does not work properly with sending the request as a stream without
>>>knowing the content length until stream.close(). (It claimed to work okay
>>>with this).
>>>
>>>b) After looking at the code to try to fix the problem, not only did I
>>>give up trying to fix the problem, but I also gave up on the product :)
>>>
>>>So anyways, hearing 2.0alpha of HttpClient was out, and supported both
>>>SSL (needed) and Streams (very good), I decided to try it out.
>>>
>>>I want to point out that I encountered bug 13463 early on, and after
>>>reading the bugzilla db, I tried the patch attached to the end of it. I
>>>would just like to give my vote to include it into CVS, as it fixes the
>>>problem (bug 13463) perfectly.
>>>
>>>-----
>>>
>>>As for bug 16458, I have fixed it.
>>>
>>>It was a rather simple bug, and can be reproduced by closing the server
>>>side socket while the client is still waiting for a response. This will
>>>cause the client to take 100% cpu, and it will do so for ever and ever.
>>>
>>>The fix is as follows (I have tested extensively any fixes I will post):
>>>
>>>Index: HttpConnection.java
>>>===================================================================
>>>RCS file:
>>>/home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/ht
>>>tpclient/HttpConnection.java,v retrieving revision 1.44
>>>diff -u -r1.44 HttpConnection.java
>>>--- HttpConnection.java 13 Feb 2003 21:31:53 -0000 1.44
>>>+++ HttpConnection.java 19 Feb 2003 21:27:26 -0000
>>>@@ -128,6 +128,10 @@
>>>
>>> StringBuffer buf = new StringBuffer();
>>> int ch = inputStream.read();
>>>+ if(ch == -1){
>>>+ // End Of File!
>>>+ return null; // Let caller know!
>>>+ }
>>> while (ch >= 0) {
>>> if (ch == '\r') {
>>> ch = inputStream.read();
>>>
>>>I have another bugfix that I will post in my next message.
>>>
>>>Thanks,
>>>Sam Maloney <me...@sammaloney.com>
>>>
>>>---------------------------------------------------------------------
>>>To unsubscribe, e-mail:
>>>commons-httpclient-dev-unsubscribe@jakarta.apache.org For additional
>>>commands, e-mail: commons-httpclient-dev-help@jakarta.apache.org
>>>
>>>
>>---------------------------------------------------------------------
>>To unsubscribe, e-mail:
>>commons-httpclient-dev-unsubscribe@jakarta.apache.org For additional
>>commands, e-mail: commons-httpclient-dev-help@jakarta.apache.org
>>
>>
>
>
>---------------------------------------------------------------------
>To unsubscribe, e-mail: commons-httpclient-dev-unsubscribe@jakarta.apache.org
>For additional commands, e-mail: commons-httpclient-dev-help@jakarta.apache.org
>
>
>
>
Re: [PATCH] Fix for bug 16458
Posted by Sam Maloney <sa...@filogix.com>.
On Wednesday 19 February 2003 17:39, Oleg Kalnichevski wrote:
> Hi Sam
>
> I believe the bug has been fixed by now. I stumbled upon it a few days
> ago pretty much by chance
>
> http://www.mail-archive.com/commons-httpclient-dev%40jakarta.apache.org/msg
>00536.html
>
This fix does not fix the same problem. The fix linked to above will prevent
(byte)-1 from being appended to the buffer (a different bug), but that is not
the problem I/bug 16458 was having.
I updated to CVS again, and there were some changes in this area (now readLine
calls HttpParser.readLine which calls HttpParser.readRawLine). However, after
testing again, I confirmed that the problem still exists unaffected.
Here is the problem:
at
org.apache.commons.httpclient.HttpParser.readRawLine(HttpParser.java:46)
at
org.apache.commons.httpclient.HttpParser.readLine(HttpParser.java:81)
at
org.apache.commons.httpclient.HttpConnection.readLine(HttpConnection.java:878)
at
org.apache.commons.httpclient.HttpConnectionAdapter.readLine(MultiThreadedHttpConnectionManager_fixed.java:730)
at
org.apache.commons.httpclient.HttpMethodBase.readStatusLine(HttpMethodBase.java:1917)
You can see that readStatusLine looks like this:
<SNIP>
//read out the HTTP status string
String statusString = conn.readLine();
while ((statusString != null) && !statusString.startsWith("HTTP/")) {
statusString = conn.readLine();
}
if (statusString == null) {
// A null statusString means the connection was lost before we got a
// response. Try again.
throw new HttpRecoverableException("Error in parsing the status "
+ " line from the response: unable to find line starting with"
+ " \"HTTP/\"");
}
</SNIP>
You can see from the above code, that until conn.readLine() returns null or a
string starting with "HTTP/", this piece of code will keep looping
indefinatly (what is taking 100% CPU). Right now, readLine never will return
null, only empty strings.
I'm guessing readLine must have returned null at one point, as every place
that calls it expects null to be returned on closed connection.
The way readLine must work for the calling code to work is:
1) Read data upto \r\n, return line. (currently happens)
2) if EOF before finding \r\n, but we have data in our buffer, return the
buffer. (also happens)
3) if EOF before finding any data (buf.size()==0), then return null to signal
that no more data is possible, and caller should NOT call again. (this is
what patch adds)
If we return empty string like is happening currently, then the caller will
not know NOT to call again. In this bugs case, the caller keeps reading lines
until it finds 'HTTP/'. Since the empty string doesn't match that, the caller
will keep trying to get the next line, and they will just keep getting "".
Here is my patch again, updated to apply to HttpParser (where the readLine
code has been moved to since my last patch). I have retested the new patch
and it is working in all my test cases, including the one to reproduce the
bug.
Index: HttpParser.java
===================================================================
RCS file:
/home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/httpclient/HttpParser.java,v
retrieving revision 1.1
diff -u -r1.1 HttpParser.java
--- HttpParser.java 16 Feb 2003 13:10:16 -0000 1.1
+++ HttpParser.java 20 Feb 2003 15:51:51 -0000
@@ -58,6 +58,10 @@
}
}
}
+ if (buf.size() <= 0) {
+ // Then we just started reading, but the stream is EOF (closed).
+ return null; // Let caller know we got EOF BEFORE any data.
+ }
if (WIRE_LOG.isDebugEnabled()) {
WIRE_LOG.debug("<< \"" + buf.toString() + (ch>0 ? "\" [\\r\\n]" :
""));
}
@@ -79,6 +83,12 @@
public static String readLine(InputStream inputStream) throws IOException
{
LOG.trace("enter HttpConnection.readLine()");
byte[] rawdata = readRawLine(inputStream);
+
+ if (rawdata == null) {
+ // Then there was EOF before any data was read, we must let
caller know.
+ return null;
+ }
+
int len = rawdata.length;
if (( len >= 2) && (rawdata[len - 2] == '\r') && (rawdata[len - 1] ==
'\n')) {
return HttpConstants.getString(rawdata, 0, rawdata.length - 2);
Cheers,
Sam Maloney
> I was not aware that it might have fixed bug# 16458. Could you please
> take the newest CVS snapshout for a spin and let us know if the problem
> has indeed been eliminated?
>
> Other patches would be highly welcome
>
> Cheers
>
> Oleg
>
> On Wed, 2003-02-19 at 23:23, Sam Maloney wrote:
> > Hi there,
> >
> > As I am a new poster here, I will first describe myself and the
> > situation. If you wish to skip this, skip down to after the line '-----'.
> >
> > In a very large project I am a senior on, I use to be using HTTPClient
> > v0.3-3 (www.innovation.ch/java/HTTPClient/).
> >
> > At the time I chose it, it was the superior client. However, because of
> > the facts:
> >
> > a) It does not work properly with sending the request as a stream without
> > knowing the content length until stream.close(). (It claimed to work okay
> > with this).
> >
> > b) After looking at the code to try to fix the problem, not only did I
> > give up trying to fix the problem, but I also gave up on the product :)
> >
> > So anyways, hearing 2.0alpha of HttpClient was out, and supported both
> > SSL (needed) and Streams (very good), I decided to try it out.
> >
> > I want to point out that I encountered bug 13463 early on, and after
> > reading the bugzilla db, I tried the patch attached to the end of it. I
> > would just like to give my vote to include it into CVS, as it fixes the
> > problem (bug 13463) perfectly.
> >
> > -----
> >
> > As for bug 16458, I have fixed it.
> >
> > It was a rather simple bug, and can be reproduced by closing the server
> > side socket while the client is still waiting for a response. This will
> > cause the client to take 100% cpu, and it will do so for ever and ever.
> >
> > The fix is as follows (I have tested extensively any fixes I will post):
> >
> > Index: HttpConnection.java
> > ===================================================================
> > RCS file:
> > /home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/ht
> >tpclient/HttpConnection.java,v retrieving revision 1.44
> > diff -u -r1.44 HttpConnection.java
> > --- HttpConnection.java 13 Feb 2003 21:31:53 -0000 1.44
> > +++ HttpConnection.java 19 Feb 2003 21:27:26 -0000
> > @@ -128,6 +128,10 @@
> >
> > StringBuffer buf = new StringBuffer();
> > int ch = inputStream.read();
> > + if(ch == -1){
> > + // End Of File!
> > + return null; // Let caller know!
> > + }
> > while (ch >= 0) {
> > if (ch == '\r') {
> > ch = inputStream.read();
> >
> > I have another bugfix that I will post in my next message.
> >
> > Thanks,
> > Sam Maloney <me...@sammaloney.com>
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail:
> > commons-httpclient-dev-unsubscribe@jakarta.apache.org For additional
> > commands, e-mail: commons-httpclient-dev-help@jakarta.apache.org
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail:
> commons-httpclient-dev-unsubscribe@jakarta.apache.org For additional
> commands, e-mail: commons-httpclient-dev-help@jakarta.apache.org
Re: [PATCH] Fix for bug 16458
Posted by Oleg Kalnichevski <o....@dplanet.ch>.
Hi Sam
I believe the bug has been fixed by now. I stumbled upon it a few days
ago pretty much by chance
http://www.mail-archive.com/commons-httpclient-dev%40jakarta.apache.org/msg00536.html
I was not aware that it might have fixed bug# 16458. Could you please
take the newest CVS snapshout for a spin and let us know if the problem
has indeed been eliminated?
Other patches would be highly welcome
Cheers
Oleg
On Wed, 2003-02-19 at 23:23, Sam Maloney wrote:
> Hi there,
>
> As I am a new poster here, I will first describe myself and the situation. If
> you wish to skip this, skip down to after the line '-----'.
>
> In a very large project I am a senior on, I use to be using HTTPClient v0.3-3
> (www.innovation.ch/java/HTTPClient/).
>
> At the time I chose it, it was the superior client. However, because of the
> facts:
>
> a) It does not work properly with sending the request as a stream without
> knowing the content length until stream.close(). (It claimed to work okay
> with this).
>
> b) After looking at the code to try to fix the problem, not only did I give up
> trying to fix the problem, but I also gave up on the product :)
>
> So anyways, hearing 2.0alpha of HttpClient was out, and supported both SSL
> (needed) and Streams (very good), I decided to try it out.
>
> I want to point out that I encountered bug 13463 early on, and after reading
> the bugzilla db, I tried the patch attached to the end of it. I would just
> like to give my vote to include it into CVS, as it fixes the problem (bug
> 13463) perfectly.
>
> -----
>
> As for bug 16458, I have fixed it.
>
> It was a rather simple bug, and can be reproduced by closing the server side
> socket while the client is still waiting for a response. This will cause the
> client to take 100% cpu, and it will do so for ever and ever.
>
> The fix is as follows (I have tested extensively any fixes I will post):
>
> Index: HttpConnection.java
> ===================================================================
> RCS file:
> /home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/httpclient/HttpConnection.java,v
> retrieving revision 1.44
> diff -u -r1.44 HttpConnection.java
> --- HttpConnection.java 13 Feb 2003 21:31:53 -0000 1.44
> +++ HttpConnection.java 19 Feb 2003 21:27:26 -0000
> @@ -128,6 +128,10 @@
>
> StringBuffer buf = new StringBuffer();
> int ch = inputStream.read();
> + if(ch == -1){
> + // End Of File!
> + return null; // Let caller know!
> + }
> while (ch >= 0) {
> if (ch == '\r') {
> ch = inputStream.read();
>
> I have another bugfix that I will post in my next message.
>
> Thanks,
> Sam Maloney <me...@sammaloney.com>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: commons-httpclient-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: commons-httpclient-dev-help@jakarta.apache.org
>