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 <[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] > > > > > > > > > > > > --------------------------------------------------------------------- > 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]
