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 <[EMAIL PROTECTED]>
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail:
> > > [EMAIL PROTECTED] For additional
> > > commands, e-mail: [EMAIL PROTECTED]
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail:
> > [EMAIL PROTECTED] For additional
> > commands, e-mail: [EMAIL PROTECTED]
>
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
