No, it would not. I was wrong in my assumption. I am going to apply
Sam's patch and commit it after having run all the test cases
Oleg
On Thu, 2003-02-20 at 17:18, Oleg Kalnichevski wrote:
> 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]
>
---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]
