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]
