Re: [PATCH] Fix for bug 16458

2003-02-21 Thread Sam Maloney 
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

2003-02-20 Thread Oleg Kalnichevski 
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

2003-02-20 Thread Oleg Kalnichevski 
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

2003-02-20 Thread Aurelien Pernoud 
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

2003-02-19 Thread Jeffrey Dever 


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]