Return-Path: Mailing-List: contact commons-httpclient-dev-help@jakarta.apache.org; run by ezmlm Delivered-To: mailing list commons-httpclient-dev@jakarta.apache.org Received: (qmail 9986 invoked from network); 13 Feb 2003 22:45:25 -0000 Received: from mail2.bluewin.ch (195.186.4.73) by daedalus.apache.org with SMTP; 13 Feb 2003 22:45:25 -0000 Received: from [192.168.1.48] (213.3.41.216) by mail2.bluewin.ch (Bluewin AG 6.7.015) id 3E1029A200429E78 for commons-httpclient-dev@jakarta.apache.org; Thu, 13 Feb 2003 22:45:31 +0000 Subject: [PATCH]: Minor bugs in the HttpConnection From: Oleg Kalnichevski To: Commons HttpClient Project In-Reply-To: <3E4BF8F1.5000802@sympatico.ca> References: <4916B7B6-3F0C-11D7-BD2B-00306557E112@u.washington.edu> <3E4B3BA1.6030007@sympatico.ca> <1045163441.2220.42.camel@kczrh-okt22.corp.bearingpoint.com> <3E4BF8F1.5000802@sympatico.ca> Content-Type: multipart/mixed; boundary="=-PteB5iKd/51avjtScQJz" Organization: Message-Id: <1045176297.2704.124.camel@kczrh-okt22.corp.bearingpoint.com> Mime-Version: 1.0 X-Mailer: Ximian Evolution 1.2.2 Date: 13 Feb 2003 23:44:57 +0100 X-Spam-Rating: daedalus.apache.org 1.6.2 0/1000/N --=-PteB5iKd/51avjtScQJz Content-Type: text/plain Content-Transfer-Encoding: 7bit Jeff, Mike, Odi How about this for a starter? Cheers Oleg PS: I'll be off-line for a few days (gonna be boarding in Davos) On Thu, 2003-02-13 at 20:58, Jeffrey Dever wrote: > > > > > > > >Firstly, there's end of stream check missing, but it's just a minor bug > > > > ch = inputStream.read(); > > if (ch == -1) { > > break; > > } > > if (ch == '\n') { > > break; > > } else { > > buf.append('\r'); > > } > > > > > Deja vu. Didn't I already fix that in HttpConnection, which now reads: > int ch = inputStream.read(); > while (ch >= 0) { > if (ch == '\r') { > ch = inputStream.read(); > if (ch == '\n') { > break; > } else { > buf.append('\r'); > } > } > buf.append((char) ch); > ch = inputStream.read(); > } > > >Secondly, I am a bit unhappy with this line > > > > buf.append((char) ch); > > > You are right. We should be using a byte[] here as the buffer instead > of a StringBuffer. Then call the correct string conversion routine as > you stated. This is only buffering one line of headers here, so its > good from a performance perspective. > > >Bottom line. Let's tackle problems step by step through a series of > >small patches and if we see that wire logging cannot be made > >comprehensive, so be it. > > > >Personally I am a strong believer in making things work 100% or not > >making them at all. I'd rather remove wire logging all together, or > >restrict it to exclusively dealing with request/response headers, rather > >than having it occasionally spit out some bits and pieces. > > > > > Like I said, if we split the wire logging responsibilities into headers > and bodies, I think that we could be a lot more flexible. Lets face it, > the information in a header is completely different than the data in a > body, including the content encoding. HttpClient already handles these > sperately by reading the headers completely, and then passing the body > to the client as an output stream. > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: commons-httpclient-dev-unsubscribe@jakarta.apache.org > For additional commands, e-mail: commons-httpclient-dev-help@jakarta.apache.org > --=-PteB5iKd/51avjtScQJz Content-Disposition: attachment; filename=httpparser.patch Content-Type: text/x-patch; name=httpparser.patch; charset=KOI8-R Content-Transfer-Encoding: 7bit Index: java/org/apache/commons/httpclient/ChunkedInputStream.java =================================================================== RCS file: /home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/httpclient/ChunkedInputStream.java,v retrieving revision 1.14 diff -u -r1.14 ChunkedInputStream.java --- java/org/apache/commons/httpclient/ChunkedInputStream.java 13 Feb 2003 21:31:53 -0000 1.14 +++ java/org/apache/commons/httpclient/ChunkedInputStream.java 13 Feb 2003 22:40:17 -0000 @@ -67,7 +67,6 @@ import java.io.IOException; import java.io.InputStream; -import org.apache.commons.httpclient.util.HeaderParser; /** *

Transparently coalesces chunks of a HTTP stream that uses @@ -340,7 +339,7 @@ * @throws IOException If an IO problem occurs */ private void parseTrailerHeaders() throws IOException { - Header[] footers = HeaderParser.parseHeaders(in); + Header[] footers = HttpParser.parseHeaders(in); for (int i = 0; i < footers.length; i++) { method.addResponseFooter(footers[i]); Index: java/org/apache/commons/httpclient/HttpConnection.java =================================================================== RCS file: /home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/httpclient/HttpConnection.java,v retrieving revision 1.44 diff -u -r1.44 HttpConnection.java --- java/org/apache/commons/httpclient/HttpConnection.java 13 Feb 2003 21:31:53 -0000 1.44 +++ java/org/apache/commons/httpclient/HttpConnection.java 13 Feb 2003 22:40:14 -0000 @@ -110,42 +110,6 @@ */ public class HttpConnection { - /** - * Read up to "\r\n" from an (unchunked) input stream. - * If the stream ends before the line terminator is found, - * the last part of the string will still be returned. - * '\r' and '\n' are allowed to appear individually in the stream. - * - * @param inputStream the stream to read from - * - * @throws IOException if an I/O problem occurs - * @return a line from the stream - * - * @since 2.0beta1 - */ - public static String readLine(InputStream inputStream) throws IOException { - LOG.trace("enter HttpConnection.readLine()"); - - StringBuffer buf = new StringBuffer(); - int ch = inputStream.read(); - while (ch >= 0) { - if (ch == '\r') { - ch = inputStream.read(); - if (ch == '\n') { - break; - } else { - buf.append('\r'); - } - } - buf.append((char) ch); - ch = inputStream.read(); - } - if (WIRE_LOG.isDebugEnabled()) { - WIRE_LOG.debug("<< \"" + buf.toString() + (ch>0 ? "\" [\\r\\n]" : "")); - } - return (buf.toString()); - } - // ----------------------------------------------------------- Constructors /** @@ -911,7 +875,7 @@ LOG.trace("enter HttpConnection.readLine()"); assertOpen(); - return HttpConnection.readLine(inputStream); + return HttpParser.readLine(inputStream); } /** Index: java/org/apache/commons/httpclient/HttpMethodBase.java =================================================================== RCS file: /home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/httpclient/HttpMethodBase.java,v retrieving revision 1.111 diff -u -r1.111 HttpMethodBase.java --- java/org/apache/commons/httpclient/HttpMethodBase.java 11 Feb 2003 03:23:05 -0000 1.111 +++ java/org/apache/commons/httpclient/HttpMethodBase.java 13 Feb 2003 22:40:30 -0000 @@ -75,7 +75,6 @@ import org.apache.commons.httpclient.cookie.CookiePolicy; import org.apache.commons.httpclient.cookie.CookieSpec; -import org.apache.commons.httpclient.util.HeaderParser; import org.apache.commons.httpclient.util.URIUtil; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -1862,7 +1861,7 @@ + "HttpConnection)"); getResponseHeaderGroup().clear(); - Header[] headers = HeaderParser.parseHeaders(conn.getResponseInputStream()); + Header[] headers = HttpParser.parseHeaders(conn.getResponseInputStream()); getResponseHeaderGroup().setHeaders(headers); } Index: java/org/apache/commons/httpclient/HttpParser.java =================================================================== RCS file: java/org/apache/commons/httpclient/HttpParser.java diff -N java/org/apache/commons/httpclient/HttpParser.java --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ java/org/apache/commons/httpclient/HttpParser.java 13 Feb 2003 22:40:15 -0000 @@ -0,0 +1,147 @@ +package org.apache.commons.httpclient; + +import java.io.IOException; +import java.io.InputStream; +import java.io.ByteArrayOutputStream; +import java.util.ArrayList; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; + +/** + * A utility class for parsing http header values. + * + * @author Michael Becke + * @author Oleg Kalnichevski + * + * @since 2.0beta1 + */ +public class HttpParser { + + /** Log object for this class. */ + private static final Log LOG = LogFactory.getLog(HttpParser.class); + + /** Log for any wire messages. */ + private static final Log WIRE_LOG = LogFactory.getLog("httpclient.wire"); + /** + * Constructor for HttpParser. + */ + private HttpParser() {} + + /** + * Return byte array from an (unchunked) input stream. + * Stop reading when "\r\n" terminator encountered + * If the stream ends before the line terminator is found, + * the last part of the string will still be returned. + * '\r' and '\n' are allowed to appear individually in the stream. + * + * @param inputStream the stream to read from + * + * @throws IOException if an I/O problem occurs + * @return a byte array from the stream + */ + public static byte[] readRawLine(InputStream inputStream) throws IOException { + LOG.trace("enter HttpConnection.readRawLine()"); + + ByteArrayOutputStream buf = new ByteArrayOutputStream(); + int ch; + while ((ch = inputStream.read()) >= 0) { + buf.write(ch); + if (ch == '\r') { + ch = inputStream.read(); + if (ch < 0) { + break; + } + buf.write(ch); + if (ch == '\n') { + break; + } + } + } + if (WIRE_LOG.isDebugEnabled()) { + WIRE_LOG.debug("<< \"" + buf.toString() + (ch>0 ? "\" [\\r\\n]" : "")); + } + return buf.toByteArray(); + } + + /** + * Read up to "\r\n" from an (unchunked) input stream. + * If the stream ends before the line terminator is found, + * the last part of the string will still be returned. + * '\r' and '\n' are allowed to appear individually in the stream. + * + * @param inputStream the stream to read from + * + * @throws IOException if an I/O problem occurs + * @return a line from the stream + */ + + public static String readLine(InputStream inputStream) throws IOException { + LOG.trace("enter HttpConnection.readLine()"); + byte[] rawdata = readRawLine(inputStream); + if ((rawdata.length >= 2) && (rawdata[0] == '\r') && (rawdata[1] == '\n')) { + return HttpConstants.getString(rawdata, 0, rawdata.length - 2); + } else { + return HttpConstants.getString(rawdata); + } + } + + /** + * Parses headers from the given stream. Headers with the same name are not + * combined. + * + * @param is the stream to read headers from + * + * @return an array of headers in the order in which they were parsed + * + * @throws IOException if an IO error occurs while reading from the stream + * @throws HttpException if there is an error parsing a header value + */ + public static Header[] parseHeaders(InputStream is) throws IOException, HttpException { + LOG.trace("enter HeaderParser.parseHeaders(HttpConnection, HeaderGroup)"); + + ArrayList headers = new ArrayList(); + String name = null; + StringBuffer value = null; + for (; ;) { + String line = HttpParser.readLine(is); + if ((line == null) || (line.length() < 1)) { + break; + } + + // Parse the header name and value + // Check for folded headers first + // Detect LWS-char see HTTP/1.0 or HTTP/1.1 Section 2.2 + // discussion on folded headers + if ((line.charAt(0) == ' ') || (line.charAt(0) == '\t')) { + // we have continuation folded header + // so append value + value.append(' '); + value.append(line.trim()); + } else { + // make sure we save the previous name,value pair if present + if (name != null) { + headers.add(new Header(name, value.toString())); + } + + // Otherwise we should have normal HTTP header line + // Parse the header name and value + int colon = line.indexOf(":"); + if (colon < 0) { + throw new HttpException("Unable to parse header: " + line); + } + name = line.substring(0, colon).trim(); + value = new StringBuffer(line.substring(colon + 1).trim()); + } + + } + + // make sure we save the last name,value pair if present + if (name != null) { + headers.add(new Header(name, value.toString())); + } + + return (Header[]) headers.toArray(new Header[headers.size()]); + } + +} Index: java/org/apache/commons/httpclient/util/HeaderParser.java =================================================================== RCS file: java/org/apache/commons/httpclient/util/HeaderParser.java diff -N java/org/apache/commons/httpclient/util/HeaderParser.java --- java/org/apache/commons/httpclient/util/HeaderParser.java 11 Feb 2003 05:04:10 -0000 1.1 +++ /dev/null 1 Jan 1970 00:00:00 -0000 @@ -1,88 +0,0 @@ -package org.apache.commons.httpclient.util; - -import java.io.IOException; -import java.io.InputStream; -import java.util.ArrayList; - -import org.apache.commons.httpclient.Header; -import org.apache.commons.httpclient.HttpConnection; -import org.apache.commons.httpclient.HttpException; -import org.apache.commons.logging.Log; -import org.apache.commons.logging.LogFactory; - -/** - * A utility class for parsing http header values. - * - * @author Michael Becke - * - * @since 2.0beta1 - */ -public class HeaderParser { - - /** Log object for this class. */ - private static final Log LOG = LogFactory.getLog(HeaderParser.class); - - /** - * Constructor for HeaderParser. - */ - private HeaderParser() {} - - /** - * Parses headers from the given stream. Headers with the same name are not - * combined. - * - * @param is the stream to read headers from - * - * @return an array of headers in the order in which they were parsed - * - * @throws IOException if an IO error occurs while reading from the stream - * @throws HttpException if there is an error parsing a header value - */ - public static Header[] parseHeaders(InputStream is) throws IOException, HttpException { - LOG.trace("enter HeaderParser.parseHeaders(HttpConnection, HeaderGroup)"); - - ArrayList headers = new ArrayList(); - String name = null; - StringBuffer value = null; - for (; ;) { - String line = HttpConnection.readLine(is); - if ((line == null) || (line.length() < 1)) { - break; - } - - // Parse the header name and value - // Check for folded headers first - // Detect LWS-char see HTTP/1.0 or HTTP/1.1 Section 2.2 - // discussion on folded headers - if ((line.charAt(0) == ' ') || (line.charAt(0) == '\t')) { - // we have continuation folded header - // so append value - value.append(' '); - value.append(line.trim()); - } else { - // make sure we save the previous name,value pair if present - if (name != null) { - headers.add(new Header(name, value.toString())); - } - - // Otherwise we should have normal HTTP header line - // Parse the header name and value - int colon = line.indexOf(":"); - if (colon < 0) { - throw new HttpException("Unable to parse header: " + line); - } - name = line.substring(0, colon).trim(); - value = new StringBuffer(line.substring(colon + 1).trim()); - } - - } - - // make sure we save the last name,value pair if present - if (name != null) { - headers.add(new Header(name, value.toString())); - } - - return (Header[]) headers.toArray(new Header[headers.size()]); - } - -} Index: test/org/apache/commons/httpclient/SimpleHttpConnection.java =================================================================== RCS file: /home/cvspublic/jakarta-commons/httpclient/src/test/org/apache/commons/httpclient/SimpleHttpConnection.java,v retrieving revision 1.9 diff -u -r1.9 SimpleHttpConnection.java --- test/org/apache/commons/httpclient/SimpleHttpConnection.java 11 Feb 2003 03:23:05 -0000 1.9 +++ test/org/apache/commons/httpclient/SimpleHttpConnection.java 13 Feb 2003 22:40:08 -0000 @@ -186,7 +186,7 @@ public String readLine() throws IOException, IllegalStateException { - String str = HttpConnection.readLine(inputStream); + String str = HttpParser.readLine(inputStream); log.debug("read: " + str); return str; } --=-PteB5iKd/51avjtScQJz--