Re: [PATCH] Fix for bug 16458
Hi, This is what I have found, as I have worked in that area now: The exception you speak is caused by fix for 16458. Before, instead of said exception, you would have got 100% CPU * forever. Here is the source of the exception (HttpMethodBase.java): SNIP protected void readStatusLine(HttpState state, HttpConnection conn) throws IOException, HttpRecoverableException, HttpException { LOG.trace(enter HttpMethodBase.readStatusLine(HttpState, HttpConnection)); //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 Before fix for 16458, is was impossible to have a return value of null from readLine(). Only an empty string would be returned. The comment says the correct thing here, that the author expeced null return to signify connection reset by peer/incorrect response. The comment implies that somewhere up the call stack, the HttpRecoverableException will be caught and the request automatically retried, but I noticed it doesn't. Instead the exception gets to the main thread. I'm not sure if the code just hasn't been written to auto retry the request when an HttpRecoverableException, or if it is broken, or if this is the way intended, for the onus to be on the user of HttpClient to catch the exception and try the request again. I'm sure someone more experienced with HttpClient can answer that. I think what makes sense as to what should happen is that any auto-retry code (and remember, the HTTP RFC says only to retry once!) should be in HttpClient.java. If the user uses the HttpConnection and HttpMethod classes directly, then it would be up to the user to retry if they wanted to. As for you Aurelien, you can simpy wrap your call to HttpClient-executeMethod() like this: for(int tries = 1; tries = 2; tries++){ try{ client.executeMethod(...); break; // If success (no exception), then break. }catch(HttpRecoverableException re){ if(tries == 2){ throw re; // If this was try 2, then give up. } }catch(IOException ioe){ throw new RuntimeException(., ioe); } } To HttpClient developers: That above part of code should probably be put into HttpClient, as if the user is using HttpClient, I think a recoverable error like the socket closing or bad response should be auto retried, perhaps have a setting on HttpClient (enableAutoRetry(bool) or something.) Later, Sam Maloney On Friday 21 February 2003 03:28, Aurelien Pernoud wrote: Weel, this one seems to be over (for me at least). But I'm sorry Mike, I found out my webapp was not logging httpexception correctly (I catched it and logged it in debug mode and i was only logging errors so...), I still have HttpRecoverable exception about can't find line starting with HTTP/ with multiple users. I'm gonna check my code again, try with the old httpclient, to see the difference, and be sure of what it can come from. Aurelien Pernoud a écrit : Made a test for 45 min, seems to be going fine now. I'll test furthermore tomorrow. Aurelien Pernoud a écrit : Ok, I'm gonna test it right now and tell you tomorrow morning if everything went ok with my app. Oleg Kalnichevski a écrit : Committed - 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]
Re: [PATCH] Fix for bug 16458
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 - 1.1 +++ HttpParser.java 20 Feb 2003 15:51:51 - @@ -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() + (ch0 ? \ [\\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
Re: [PATCH] Fix for bug 16458
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 - 1.1 +++ HttpParser.java 20 Feb 2003 15:51:51 - @@ -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() + (ch0 ? \ [\\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
RE: [PATCH] Fix for bug 16458
Ok, I'm gonna test it right now and tell you tomorrow morning if everything went ok with my app. Oleg Kalnichevski a écrit : Committed - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
Re: [PATCH] Fix for bug 16458
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. - To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]