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 22796 invoked by uid 98); 5 Dec 2002 21:21:45 -0000 X-Antivirus: nagoya (v4218 created Aug 14 2002) Received: (qmail 22758 invoked from network); 5 Dec 2002 21:21:43 -0000 Received: from daedalus.apache.org (HELO apache.org) (63.251.56.142) by nagoya.betaversion.org with SMTP; 5 Dec 2002 21:21:43 -0000 Received: (qmail 69376 invoked by uid 500); 5 Dec 2002 21:20:32 -0000 Received: (qmail 69368 invoked from network); 5 Dec 2002 21:20:31 -0000 Received: from unknown (HELO extmail.extensibility.com) (63.86.210.252) by daedalus.apache.org with SMTP; 5 Dec 2002 21:20:31 -0000 Received: from tibco.com (remote-10.98.40.49.tibco.com [10.98.40.49]) by extmail.extensibility.com with SMTP (Microsoft Exchange Internet Mail Service Version 5.5.2655.55) id SGSJA9W9; Thu, 5 Dec 2002 16:17:41 -0500 Message-ID: <3DEFC343.2000205@tibco.com> Date: Thu, 05 Dec 2002 16:21:07 -0500 From: Eric Johnson Organization: TIBCO Extensibility User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2.1) Gecko/20021130 X-Accept-Language: en-us, en MIME-Version: 1.0 To: HttpClient Project Subject: [PATCH] handling close properly on response input streams Content-Type: multipart/mixed; boundary="------------020200080500070203010001" X-Spam-Rating: daedalus.apache.org 1.6.2 0/1000/N --------------020200080500070203010001 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit OK, as a hopeful sign, I addressed one more round at some issues pointed out by Michael, Ortwin, and Ryan, and my patch shrank a little bit. Key changes over previous submission: * spelling insure --> ensure * AutoCloseInputStream is still called that, but now actually calls close on the underlying stream (this is essential for correctly handling partial reads of previous responses on persistent connections). ContentLengthInputStream and ChunkedInputStream no longer take a "watcher" as a parameter, do not close the underlying stream (as before), and do not attempt to notify any watchers. Instead _readResponseBody() _always_ wraps a non-null response in an AutoCloseInputStream. * ResponseConsumedWatcher interface is no longer public, instead it is a package interface, as it is now only used by HttpMethodBase and AutoCloseInputStream (which is package level access itself). * Above changes should accommodate oversight pointed out by Michael - the previously unwrapped stream now gets wrapped. Keep in mind that the wrapping has two effects, one of which is to close the underlying stream once the data is consumed, and another of which is to alert the "watcher" that the response has been consumed. The watcher (in this case, the instance of HttpMethodBase) figures out whether the connection should be closed, and whether it should be released to the connection manager. All of this logic can be found in one place - HttpMethodBase.responseBodyConsumed. * I had missed, in my previous submission, the fact that ConnectMethod overrides execute(). Unfortunately, HttpMethodBase.execute() will now "release" the connection if the response has been consumed prior to the function exiting. In the case of ConnectMethod, by virtual of the a null input stream on the response, my alterations assumed that this meant the connection had been fully consumed. I made the "responseBodyConsumed()" function protected, and now overridden by ConnectMethod to do nothing. I can only guess that my oversight would have caused problems with proxied connections, as I am not currently able to test them. As to this last point, I was hoping that there would be some test case that I could try, or write, but as near as I can tell, this would be very difficult to set up a test case without an actual proxy server, which I don't have hanging around. Thus, I ask the question - could someone try this patch using the test cases configured to talk to a proxy server? And then let the group know, so that I can stop worrying about getting this patch accepted! Thanks. -Eric. --------------020200080500070203010001 Content-Type: text/plain; name="close3.patch" Content-Transfer-Encoding: 8bit Content-Disposition: inline; filename="close3.patch" ? close.patch ? close2.patch ? close3.patch ? lib/junit.jar ? lib/servlet.jar ? src/java/org/apache/commons/httpclient/ResponseConsumedWatcher.java Index: src/java/org/apache/commons/httpclient/AutoCloseInputStream.java =================================================================== RCS file: /home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/httpclient/AutoCloseInputStream.java,v retrieving revision 1.2 diff -u -r1.2 AutoCloseInputStream.java --- src/java/org/apache/commons/httpclient/AutoCloseInputStream.java 9 Sep 2002 05:43:39 -0000 1.2 +++ src/java/org/apache/commons/httpclient/AutoCloseInputStream.java 5 Dec 2002 21:07:26 -0000 @@ -67,44 +67,57 @@ import java.io.InputStream; /** - * Closes a HttpConnection as soon as the end of the stream is reached. + * Closes an underlying stream as soon as the end of the stream is reached, and + * notifies a client when it has done so. + * * @author Ortwin Gl�ck * * @since 2.0 */ class AutoCloseInputStream extends FilterInputStream { - /** the connection the input stream comes from */ - private HttpConnection conn; + + // assume that the underlying stream is open until we get an EOF indication. + private boolean streamOpen = true; + + private boolean selfClosed = false; + + /** The watcher is notified when the contents of the stream have been exhausted */ + private ResponseConsumedWatcher watcher = null; /** * Create a new auto closing stream for the provided connection - * + * * @param in the input stream to read from - * @param conn the connection to close when done reading + * @param watcher To be notified when the contents of the stream have been + * consumed. */ - public AutoCloseInputStream(InputStream in, HttpConnection conn) { + public AutoCloseInputStream(InputStream in, ResponseConsumedWatcher watcher) { super(in); - this.conn = conn; + this.watcher = watcher; } /** * Reads the next byte of data from the input stream. - * + * * @throws IOException when there is an error reading * @return the character read, or -1 for EOF */ public int read() throws IOException { - int l = super.read(); - if (l == -1) { - conn.close(); + int l = -1; + + if (isReadAllowed()) { + // underlying stream not closed, go ahead and read. + l = super.read(); + checkClose(l); } + return l; } /** * Reads up to len bytes of data from the stream. - * + * * @param b a byte array to read data into * @param off an offset within the array to store data * @param len the maximum number of bytes to read @@ -112,26 +125,80 @@ * @throws IOException if there are errors reading */ public int read(byte[] b, int off, int len) throws IOException { - int l = super.read(b, off, len); - if (l == -1) { - conn.close(); + int l = -1; + + if ( isReadAllowed() ) { + l = super.read(b, off, len); + checkClose(l); } + return l; } /** * Reads some number of bytes from the input stream and stores them into the * buffer array b. - * + * * @param b a byte array to read data into * @return the number of bytes read or -1 for EOF * @throws IOException if there are errors reading */ public int read(byte[] b) throws IOException { - int l = super.read(b); - if (l == -1) { - conn.close(); + int l = -1; + + if ( isReadAllowed() ) { + l = super.read(b); + checkClose(l); } return l; } -} \ No newline at end of file + + /** + * Close the stream, and also close the underlying stream if it is not + * already closed. + */ + public void close() throws IOException { + if (!selfClosed) { + selfClosed = true; + notifyWatcher(); + } + } + + /** + * Close the underlying stream should the end of the stream arrive. + * + * @param readResult The result of the read operation to check. + */ + private void checkClose(int readResult) throws IOException { + if (readResult == -1) { + notifyWatcher(); + } + } + + /** + * See whether a read of the underlying stream should be allowed, and if + * not, check to see whether our stream has already been closed! + * + * @return true if it is still OK to read from the stream. + */ + private boolean isReadAllowed() throws IOException { + if (!streamOpen && selfClosed) { + throw new IOException("Attempted read on closed stream."); + } + return streamOpen; + } + + /** + * Notify the watcher that the contents have been consumed. + */ + private void notifyWatcher() throws IOException { + if (streamOpen) { + super.close(); + streamOpen = false; + + if (watcher != null) + watcher.responseConsumed(); + } + } +} + Index: src/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.6 diff -u -r1.6 ChunkedInputStream.java --- src/java/org/apache/commons/httpclient/ChunkedInputStream.java 16 Oct 2002 13:14:11 -0000 1.6 +++ src/java/org/apache/commons/httpclient/ChunkedInputStream.java 5 Dec 2002 21:07:26 -0000 @@ -63,12 +63,16 @@ package org.apache.commons.httpclient; import java.io.*; -import java.util.*; /** *

Transparently coalesces chunks of a HTTP stream that uses Transfer-Encoding * chunked.

* + *

Note that this class NEVER closes the underlying stream, even when close gets + * called. Instead, it will read until the "end" of its chunking on close, which + * allows for the seamless invocation of subsequent HTTP 1.1 calls, while not + * requiring the client to remember to read the entire contents of the response.

+ * * @see ResponseInputStream * * @author Ortwin Gl�ck @@ -83,6 +87,7 @@ private InputStream in; private int chunkSize, pos; private boolean eof = false; + private boolean closed = false; private static final String HTTP_ENC = "US-ASCII"; private HttpMethod method; @@ -96,7 +101,7 @@ * @throws java.lang.NullPointerException * */ - public ChunkedInputStream(final InputStream in, final HttpMethod method) throws IOException { + public ChunkedInputStream(InputStream in, HttpMethod method) throws IOException { if (null == in) { throw new NullPointerException("InputStream parameter"); } @@ -124,6 +129,9 @@ * @throws IOException */ public int read() throws IOException { + + if (closed) + throw new IOException("Attempted read from closed stream."); if (eof) return -1; if (pos >= chunkSize) { nextChunk(); @@ -134,6 +142,10 @@ } public int read(byte[] b, int off, int len) throws java.io.IOException { + + if (closed) + throw new IOException("Attempted read from closed stream."); + if (eof) return -1; if (pos >= chunkSize) { nextChunk(); @@ -274,7 +286,40 @@ return (buf.toString()); } + /** + * Upon close, this reads the remainder of the chunked message, + * leaving the underlying socket at a position to start reading the + * next response without scanning. + */ public void close() throws IOException { - in.close(); + if (!closed) { + try { + if (!eof) { + exhaustInputStream(this); + } + } + finally { + eof = true; + closed = true; + } + } + } + + /** + * Exhaust an input stream, reading until EOF has been encountered. + * + *

Note that this function is intended as a non-public utility. + * This is a little weird, but it seemed silly to make a utility + * class for this one function, so instead it is just static and + * shared that way.

+ * + * @param inStream The {@link InputStream} to exhaust. + */ + static void exhaustInputStream(InputStream inStream) throws IOException { + // read and discard the remainder of the message + byte buffer[] = new byte[1024]; + while ( inStream.read(buffer) >= 0) { + ; + } } } Index: src/java/org/apache/commons/httpclient/ConnectMethod.java =================================================================== RCS file: /home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/httpclient/ConnectMethod.java,v retrieving revision 1.4 diff -u -r1.4 ConnectMethod.java --- src/java/org/apache/commons/httpclient/ConnectMethod.java 5 Aug 2002 12:26:24 -0000 1.4 +++ src/java/org/apache/commons/httpclient/ConnectMethod.java 5 Dec 2002 21:07:26 -0000 @@ -166,6 +166,17 @@ conn.printLine(line); } + /** + * A response has been consumed. + * + * This method simply ignores this notification - once the response + * body has been consumed, this method is ready to turn around and + * execute its "wrapped" method. It does not want to close the connection + * or surrender it unto the connection manager. + */ + protected void responseBodyConsumed() { + + } /** Log object for this class. */ private static final Log log = LogFactory.getLog(ConnectMethod.class); Index: src/java/org/apache/commons/httpclient/ContentLengthInputStream.java =================================================================== RCS file: /home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/httpclient/ContentLengthInputStream.java,v retrieving revision 1.2 diff -u -r1.2 ContentLengthInputStream.java --- src/java/org/apache/commons/httpclient/ContentLengthInputStream.java 19 Sep 2002 10:15:08 -0000 1.2 +++ src/java/org/apache/commons/httpclient/ContentLengthInputStream.java 5 Dec 2002 21:07:26 -0000 @@ -75,6 +75,8 @@ private int contentLength; private int pos = 0; + private boolean closed = false; + /** * Creates a new length limited stream * @@ -88,14 +90,34 @@ } public int read() throws java.io.IOException { - if (pos >= contentLength) return -1; + if (closed) + throw new IOException("Attempted read from closed stream."); + + if (pos >= contentLength) + return -1; pos++; return super.read(); } - + /** + * Does standard {@link InputStream#read(byte[], int, int)} behavior, but + * also notifies the watcher when the contents have been consumed. + * + * @param b The byte array to fill. + * @param off Start filling at this position. + * @param len The number of bytes to attempt to read. + * @return The number of bytes read, or -1 if the end of content has been + * reached. + * + * @throws java.io.IOException Should an error occur on the wrapped stream. + */ public int read(byte[] b, int off, int len) throws java.io.IOException { - if (pos >= contentLength) return -1; + if (closed) + throw new IOException("Attempted read from closed stream."); + + if (pos >= contentLength) + return -1; + if (pos + len > contentLength) { len = contentLength - pos; } @@ -104,9 +126,25 @@ return count; } - public int read(byte[] b) throws java.io.IOException { return read(b, 0, b.length); } + /** + * Reads until the end of the known length of content. + * + *

Does not close the underlying socket input, but instead leaves it + * primed to parse the next response.

+ */ + public void close() throws IOException { + if (!closed) { + try { + ChunkedInputStream.exhaustInputStream(this); + } finally { + // close after above so that we don't throw an exception trying + // to read after closed! + closed = true; + } + } + } } Index: src/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.26 diff -u -r1.26 HttpConnection.java --- src/java/org/apache/commons/httpclient/HttpConnection.java 3 Dec 2002 05:46:15 -0000 1.26 +++ src/java/org/apache/commons/httpclient/HttpConnection.java 5 Dec 2002 21:07:27 -0000 @@ -306,6 +306,37 @@ return (!(null == _proxyHost || 0 >= _proxyPort)); } + /** + * Set the state to keep track of the last response for the last request. + * + *

The connection managers use this to ensure that previous requests are + * properly closed before a new request is attempted. That way, a GET + * request need not be read in its entirety before a new request is issued. + * Instead, this stream can be closed as appropriate.

+ * + * @param inStream The stream associated with an HttpMethod. + */ + public void setLastResponseInputStream(InputStream inStream) { + _lastResponseInput = inStream; + } + + /** + * Returns the stream used to read the last response's body. + * + *

Clients will generally not need to call this function unless + * using HttpConnection directly, instead of calling {@link HttpClient#executeMethod}. + * For those clients, call this function, and if it returns a non-null stream, + * close the stream before attempting to execute a method. Note that + * calling "close" on the stream returned by this function may close + * the connection if the previous response contained a "Connection: close" header.

+ * + * @return An {@link InputStream} corresponding to the body of the last + * response. + */ + public InputStream getLastResponseInputStream() { + return _lastResponseInput; + } + // --------------------------------------------------- Other Public Methods /** @@ -776,6 +807,9 @@ protected void closeSocketAndStreams() { log.trace("enter HttpConnection.closeSockedAndStreams()"); + // no longer care about previous responses... + _lastResponseInput = null; + if (null != _input) { try { _input.close(); @@ -891,6 +925,8 @@ private InputStream _input = null; /** My OutputStream. */ private OutputStream _output = null; + /** An {@link InputStream} for the response to an individual request. */ + private InputStream _lastResponseInput = null; /** Whether or not I am connected. */ private boolean _open = false; /** Whether or not I am/should connect via SSL. */ Index: src/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.83 diff -u -r1.83 HttpMethodBase.java --- src/java/org/apache/commons/httpclient/HttpMethodBase.java 3 Dec 2002 15:28:18 -0000 1.83 +++ src/java/org/apache/commons/httpclient/HttpMethodBase.java 5 Dec 2002 21:07:29 -0000 @@ -233,12 +233,19 @@ /** Whether or not I have been executed. */ private boolean used = false; + /** How many times did this transparently handle a recoverable exception? */ + private int recoverableExceptionCount = 0; + /** * The maximum number of attempts to attempt recovery from an * HttpRecoverableException. */ private int maxRetries = 3; + private boolean inExecute = false; + + private boolean doneWithConnection = false; + /** Default content encoding chatset */ protected static final String DEFAULT_CHARSET = "ISO-8859-1"; @@ -566,6 +573,8 @@ while ((len = is.read(buffer)) > 0) { os.write(buffer, 0, len); } + is.close(); + os.close(); responseBody = os.toByteArray(); setResponseStream(null); log.debug("buffering response body"); @@ -590,9 +599,9 @@ return responseStream; } if (responseBody != null) { - responseStream = new ByteArrayInputStream(responseBody); + InputStream byteResponseStream = new ByteArrayInputStream(responseBody); log.debug("re-creating response stream from byte array"); - return responseStream; + return byteResponseStream; } return null; } @@ -717,25 +726,7 @@ setRequestHeader(header); } - - /** - * Close the provided HTTP connection, if: - * http 1.0 and not using the 'connect' method, or - * http 1.1 and the Connection: close header is sent - * - * @param connection the HTTP connection to process - * Add the specified request header. If a header of the same name already - * exists, the new value will be appended onto the the existing value - * list. A header value of null will be ignored. Note - * that header-name matching is case insensitive. - */ - private void closeConnection(HttpConnection connection) { - if (shouldCloseConnection()) { - connection.close(); - } - } - - private boolean shouldCloseConnection() { + protected boolean shouldCloseConnection() { if (!http11) { if (getName().equals(ConnectMethod.NAME) && (statusLine.getStatusCode() == HttpStatus.SC_OK)) { @@ -757,13 +748,59 @@ return false; } - private void wrapResponseStream( HttpConnection connection ) { + private boolean isRetryNeeded(int statusCode, HttpState state, HttpConnection conn) { + switch (statusCode) { + case HttpStatus.SC_UNAUTHORIZED: + case HttpStatus.SC_PROXY_AUTHENTICATION_REQUIRED: + log.debug("Authorization required"); + if (doAuthentication) { //process authentication response + //if the authentication is successful, return the statusCode + //otherwise, drop through the switch and try again. + if (processAuthenticationResponse(state)) { + return false; + } + } else { //let the client handle the authenticaiton + return false; + } + break; + + case HttpStatus.SC_MOVED_TEMPORARILY: + case HttpStatus.SC_MOVED_PERMANENTLY: + case HttpStatus.SC_TEMPORARY_REDIRECT: + log.debug("Redirect required"); + + if (! processRedirectResponse(conn)) { + return false; + } + break; + + default: + // neither an unauthorized nor a redirect response + return false; + } //end of switch + + return true; + } + + private void checkExecuteConditions(HttpState state, HttpConnection conn) + throws HttpException { - if ( responseStream != null ) { - this.responseConnection = connection; - this.responseStream = new ResponseAutoReleaseInputStream(responseStream); + if (null == state) { + throw new NullPointerException("HttpState parameter"); + } + if (null == conn) { + throw new NullPointerException("HttpConnection parameter"); + } + if (hasBeenUsed()) { + throw new HttpException("Already used, but not recycled."); + } + if (!validate()) { + throw new HttpException("Not valid"); } + if (inExecute) { + throw new IllegalStateException("Execute invoked recursively, or exited abnormally."); + } } /** @@ -793,104 +830,83 @@ throws HttpException, IOException, NullPointerException { log.trace("enter HttpMethodBase.execute(HttpState, HttpConnection)"); - //TODO: This method is too large - //check some error conditions - if (null == state) { - throw new NullPointerException("HttpState parameter"); - } - if (null == conn) { - throw new NullPointerException("HttpConnection parameter"); - } - if (hasBeenUsed()) { - throw new HttpException("Already used, but not recycled."); - } - if (!validate()) { - throw new HttpException("Not valid"); - } - - //pre-emptively add the authorization header, if required. - Authenticator.authenticate(this, state); - if (conn.isProxied()) { - Authenticator.authenticateProxy(this, state); - } + checkExecuteConditions(state, conn); + inExecute = true; - //Set visited = new HashSet(); - realms = new HashSet(); - proxyRealms = new HashSet(); - int forwardCount = 0; //protect from an infinite loop + try { + //TODO: This method is too large - while (forwardCount++ < maxForwards) { - if (log.isDebugEnabled()) { - log.debug("Execute loop try " + forwardCount); - } + //pre-emptively add the authorization header, if required. + Authenticator.authenticate(this, state); + if (conn.isProxied()) { + Authenticator.authenticateProxy(this, state); + } + + //Set visited = new HashSet(); + realms = new HashSet(); + proxyRealms = new HashSet(); + int forwardCount = 0; //protect from an infinite loop + + while (forwardCount++ < maxForwards) { + // on every retry, reset this state information. + responseConnection = conn; + conn.setLastResponseInputStream(null); - //write the request and read the response, will retry - processRequest(state, conn); + if (log.isDebugEnabled()) { + log.debug("Execute loop try " + forwardCount); + } - //if SC_CONTINUE write the request body - writeRemainingRequestBody(state, conn); + //write the request and read the response, will retry + processRequest(state, conn); - int statusCode = statusLine.getStatusCode(); - switch (statusCode) { - case HttpStatus.SC_UNAUTHORIZED: - case HttpStatus.SC_PROXY_AUTHENTICATION_REQUIRED: - log.debug("Authorization required"); - if (doAuthentication) { //process authentication response - //if the authentication is successful, return the statusCode - //otherwise, drop through the switch and try again. - if (processAuthenticationResponse(state, conn)) { - wrapResponseStream(conn); - return statusCode; - } - } else { //let the client handle the authenticaiton - wrapResponseStream(conn); - return statusCode; - } - break; + //if SC_CONTINUE write the request body + writeRemainingRequestBody(state, conn); - case HttpStatus.SC_MOVED_TEMPORARILY: - case HttpStatus.SC_MOVED_PERMANENTLY: - case HttpStatus.SC_TEMPORARY_REDIRECT: - log.debug("Redirect required"); - - if (! processRedirectResponse(state, conn)) { - wrapResponseStream(conn); - return statusCode; - } + if (!isRetryNeeded(statusLine.getStatusCode(), state, conn)) { + // nope, no retry needed, exit loop. break; + } + /* + Revisiting may be desired. We do not know about the server's internal state. - default: - // neither an unauthorized nor a redirect response - wrapResponseStream(conn); + //check to see if we have visited this url before + if (visited.contains(generateVisitedKey(conn))) { + log.error("Link " + generateVisitedKey(conn) + "' revisited"); return statusCode; - } //end of switch + } + visited.add(generateVisitedKey(conn)); + */ -/* - Revisiting may be desired. We do not know about the server's internal state. + // retry - close previous stream. Caution - this causes + // responseBodyConsumed to be called, which may also close the + // connection. + if (responseStream != null) { + responseStream.close(); + } - //check to see if we have visited this url before - if (visited.contains(generateVisitedKey(conn))) { - log.error("Link " + generateVisitedKey(conn) + "' revisited"); - return statusCode; - } - visited.add(generateVisitedKey(conn)); -*/ + } //end of retry loop - //close connection if required - closeConnection(conn); - if (conn.isOpen()) { - //throw away body to position the stream after the response - getResponseBodyAsString(); + if (forwardCount >= maxForwards) { + log.error("Narrowly avoided an infinite loop in execute"); + throw new HttpRecoverableException("Maximum redirects ("+ maxForwards +") exceeded"); + } + } + finally { + inExecute = false; + // If the response has been fully processed, return the connection + // to the pool. Use this flag, rather than other tests (like + // responseStream == null), as subclasses, might reset the stream, + // for example, reading the entire response into a file and then + // setting the file as the stream. + if (doneWithConnection) { + ensureConnectionRelease(); } - } //end of loop - - wrapResponseStream(conn); + } - log.error("Narrowly avoided an infinite loop in execute"); - throw new HttpRecoverableException("Maximum redirects ("+ maxForwards +") exceeded"); + return statusLine.getStatusCode(); } - private boolean processRedirectResponse(HttpState state, HttpConnection conn) { + private boolean processRedirectResponse(HttpConnection conn) { if (!getFollowRedirects()) { log.info("Redirect requested but followRedirects is " @@ -965,8 +981,8 @@ * Check for a valid redirect given the current conn and new url. * Redirect to a different protocol, host or port are checked for validity. * - * @param conn The existing HttpConnection - * @param url The new URL to redirect to + * @param currentUrl The current URL (redirecting from) + * @param redirectUrl The new URL to redirect to * @throws HttpException if the redirect is invalid * @since 2.0 */ @@ -1054,6 +1070,9 @@ http11 = true; bodySent = false; responseBody = null; + recoverableExceptionCount = 0; + inExecute = false; + doneWithConnection = false; } /** @@ -1063,10 +1082,13 @@ */ public void releaseConnection() { - if ( responseConnection != null ) { - responseConnection.releaseConnection(); - this.responseConnection = null; - this.responseStream = null; + if (responseStream != null) { + try { + // FYI - this may indirectly invoke responseBodyConsumed. + responseStream.close(); + } catch (IOException e) { + // attempting cleanup, don't care about exception. + } } } @@ -1568,9 +1590,10 @@ * Read the response body from the given {@link HttpConnection}. * *

- * The current implementation simply consumes the expected response body - * (according to the values of the Content-Length and - * Transfer-Encoding headers, if any). + * The current implementation wraps the socket level stream with + * an appropriate stream for the type of response (chunked, content-length, + * or auto-close). If there is no response body, the connection associated + * with the request will be returned to the connection manager. *

* *

@@ -1591,7 +1614,17 @@ log.trace( "enter HttpMethodBase.readResponseBody(HttpState, HttpConnection)"); - setResponseStream(_readResponseBody(state, conn)); + // assume we are not done with the connection if we get a stream + doneWithConnection = false; + InputStream stream = _readResponseBody(conn); + if (stream == null) { + // done using the connection! + responseBodyConsumed(); + } + else { + conn.setLastResponseInputStream(stream); + setResponseStream(stream); + } } /** @@ -1606,11 +1639,10 @@ * @see #readResponse * @see #processResponseBody * - * @param state the client state * @param conn the {@link HttpConnection} to read the response from * @return InputStream to read the response body from */ - private InputStream _readResponseBody(HttpState state, HttpConnection conn) + private InputStream _readResponseBody(HttpConnection conn) throws IOException { log.trace("enter HttpMethodBase.readResponseBody(HttpState, HttpConnection)"); @@ -1662,12 +1694,12 @@ && !getName().equals(ConnectMethod.NAME)){ result = is; } - if (result == null) { - return null; - } - if (shouldCloseConnection()) { - result = new AutoCloseInputStream(result, conn); + // if there is a result - ALWAYS wrap it in an observer which will + // close the underlying stream as soon as it is consumed, and notify + // the watcher that the stream has been consumed. + if (result != null) { + result = new AutoCloseInputStream(result, m_responseWatcher); } return result; @@ -2065,29 +2097,14 @@ } /** - * Generates a key used for idenifying visited URLs. - * - * @param conn DOCUMENT ME! - * - * @return DOCUMENT ME! - */ - private String generateVisitedKey(HttpConnection conn) { - return conn.getHost() + ":" + conn.getPort() + "|" - + generateRequestLine(conn, getName(), getPath(), - getQueryString(), getHttpVersion()); - } - - /** * process a response that requires authentication * * @param state the current state - * @param connection the connection for communication * * @return true if the request has completed process, false if more * attempts are needed */ - private boolean processAuthenticationResponse(HttpState state, - HttpConnection connection) { + private boolean processAuthenticationResponse(HttpState state) { log.trace("enter HttpMethodBase.processAuthenticationResponse(" + "HttpState, HttpConnection)"); @@ -2175,8 +2192,8 @@ * @throws IOException when an I/O error occurs communicating with the * server * - * @see writeRequest(HttpState,HttpConnection) - * @see readResponse(HttpState,HttpConnection) + * @see #writeRequest(HttpState,HttpConnection) + * @see #readResponse(HttpState,HttpConnection) */ private void processRequest(HttpState state, HttpConnection connection) throws HttpException, IOException { @@ -2203,6 +2220,9 @@ log.debug("Closing the connection."); } + // update the recoverable exception count. + recoverableExceptionCount++; + connection.close(); log.info("Recoverable exception caught when writing request"); if (retryCount == maxRetries) { @@ -2300,64 +2320,59 @@ } /** - * Releases this connection from its connectionManager when the response has - * been read. + * Returns the number of "recoverable" exceptions thrown and handled, to + * allow for monitoring the quality of the connection. + * + * @return The number of recoverable exceptions handled by the method. */ - private class ResponseAutoReleaseInputStream extends InputStream { + public int getRecoverableExceptionCount() { + return recoverableExceptionCount; + } - private InputStream is; + /** + * A response has been consumed. + * + *

The default behavior for this class is to check to see if the connection + * should be closed, and close if need be, and to ensure that the connection + * is returned to the connection manager - if and only if we are not still + * inside the execute call.

+ * + */ + protected void responseBodyConsumed() { - public ResponseAutoReleaseInputStream(InputStream is) { - this.is = is; - } + // make sure this is the initial invocation of the notification, + // ignore subsequent ones. + responseStream = null; + responseConnection.setLastResponseInputStream(null); - /** - * @see java.io.InputStream#close() - */ - public void close() throws IOException { - is.close(); - releaseConnection(); + if (shouldCloseConnection()) { + responseConnection.close(); } - /** - * @see java.io.InputStream#read() - */ - public int read() throws IOException { - int b = is.read(); - - if ( b == -1 ) { - releaseConnection(); - } - - return b; + doneWithConnection = true; + if (!inExecute) { + ensureConnectionRelease(); } + } - /** - * @see java.io.InputStream#read(byte, int, int) - */ - public int read(byte[] array, int off, int len) throws IOException { - int b = is.read(array, off, len); - - if ( b == -1 ) { - releaseConnection(); - } - - return b; + /** + * Insure that the connection is released back to the pool. + */ + private void ensureConnectionRelease() { + if ( responseConnection != null ) { + responseConnection.releaseConnection(); + responseConnection = null; } + } - /** - * @see java.io.InputStream#read(byte) - */ - public int read(byte[] array) throws IOException { - int b = is.read(array); - - if ( b == -1 ) { - releaseConnection(); - } - - return b; + /** + * This exists so that the public interface to this class need not include + * either the responseConsumed or the responseBodyConsumed methods. + */ + private ResponseConsumedWatcher m_responseWatcher = new ResponseConsumedWatcher() { + public void responseConsumed() { + responseBodyConsumed(); } - - } + }; } Index: src/java/org/apache/commons/httpclient/MultiThreadedHttpConnectionManager.java =================================================================== RCS file: /home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/httpclient/MultiThreadedHttpConnectionManager.java,v retrieving revision 1.1 diff -u -r1.1 MultiThreadedHttpConnectionManager.java --- src/java/org/apache/commons/httpclient/MultiThreadedHttpConnectionManager.java 3 Dec 2002 05:46:15 -0000 1.1 +++ src/java/org/apache/commons/httpclient/MultiThreadedHttpConnectionManager.java 5 Dec 2002 21:07:29 -0000 @@ -349,6 +349,8 @@ public void releaseConnection(HttpConnection conn) { log.trace("enter HttpConnectionManager.releaseConnection(HttpConnection)"); + // make sure that the response has been read. + SimpleHttpConnectionManager.finishLastResponse(conn); String host = conn.getHost(); int port = conn.getPort(); String key = host + ":" + port; Index: src/java/org/apache/commons/httpclient/SimpleHttpConnectionManager.java =================================================================== RCS file: /home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/httpclient/SimpleHttpConnectionManager.java,v retrieving revision 1.1 diff -u -r1.1 SimpleHttpConnectionManager.java --- src/java/org/apache/commons/httpclient/SimpleHttpConnectionManager.java 3 Dec 2002 05:46:15 -0000 1.1 +++ src/java/org/apache/commons/httpclient/SimpleHttpConnectionManager.java 5 Dec 2002 21:07:29 -0000 @@ -62,6 +62,8 @@ package org.apache.commons.httpclient; import java.net.MalformedURLException; +import java.io.InputStream; +import java.io.IOException; /** @@ -137,8 +139,10 @@ httpConnection.setProxyHost(hostConfiguration.getProxyHost()); httpConnection.setProxyPort(hostConfiguration.getProxyPort()); - } - + } + else { + finishLastResponse(httpConnection); + } } return httpConnection; @@ -149,6 +153,28 @@ * @see org.apache.commons.httpclient.HttpConnectionManager#releaseConnection(org.apache.commons.httpclient.HttpConnection) */ public void releaseConnection(HttpConnection conn) { + if (conn != httpConnection) + throw new IllegalStateException("Unexpected close on a different connection."); + + finishLastResponse(httpConnection); } + /** + * Since the same connection is about to be reused, make sure the + * previous request was completely processed, and if not + * consume it now. + */ + static void finishLastResponse(HttpConnection conn) { + InputStream lastResponse = conn.getLastResponseInputStream(); + if ( lastResponse != null) { + conn.setLastResponseInputStream(null); + try { + lastResponse.close(); + } + catch (IOException ioe) { + // badness - close to force reconnect. + conn.close(); + } + } + } } Index: src/java/org/apache/commons/httpclient/methods/GetMethod.java =================================================================== RCS file: /home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/httpclient/methods/GetMethod.java,v retrieving revision 1.18 diff -u -r1.18 GetMethod.java --- src/java/org/apache/commons/httpclient/methods/GetMethod.java 3 Sep 2002 01:36:26 -0000 1.18 +++ src/java/org/apache/commons/httpclient/methods/GetMethod.java 5 Dec 2002 21:07:30 -0000 @@ -372,6 +372,7 @@ while ((len = in.read(buffer)) > 0) { out.write(buffer, 0, len); } + in.close(); out.close(); setResponseStream(new FileInputStream(createTempFile())); } Index: src/java/org/apache/commons/httpclient/methods/HeadMethod.java =================================================================== RCS file: /home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/httpclient/methods/HeadMethod.java,v retrieving revision 1.11 diff -u -r1.11 HeadMethod.java --- src/java/org/apache/commons/httpclient/methods/HeadMethod.java 1 Sep 2002 01:27:37 -0000 1.11 +++ src/java/org/apache/commons/httpclient/methods/HeadMethod.java 5 Dec 2002 21:07:30 -0000 @@ -146,8 +146,9 @@ log.trace( "enter HeadMethod.readResponseBody(HttpState, HttpConnection)"); - // despite the possible presence of a content-length header, + // despite the possible presence of a content-length header, // HEAD returns no response body + responseBodyConsumed(); return; } -} \ No newline at end of file +} Index: src/java/org/apache/commons/httpclient/methods/PostMethod.java =================================================================== RCS file: /home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/httpclient/methods/PostMethod.java,v retrieving revision 1.27 diff -u -r1.27 PostMethod.java --- src/java/org/apache/commons/httpclient/methods/PostMethod.java 12 Nov 2002 09:58:23 -0000 1.27 +++ src/java/org/apache/commons/httpclient/methods/PostMethod.java 5 Dec 2002 21:07:30 -0000 @@ -736,6 +736,8 @@ outstream = new ChunkedOutputStream(outstream); } if (this.requestContentLength >= 0) { + // don't need a watcher here - we're reading from something local, + // not server-side. instream = new ContentLengthInputStream(instream, this.requestContentLength); } Index: src/test/org/apache/commons/httpclient/TestGetMethodLocal.java =================================================================== RCS file: /home/cvspublic/jakarta-commons/httpclient/src/test/org/apache/commons/httpclient/TestGetMethodLocal.java,v retrieving revision 1.3 diff -u -r1.3 TestGetMethodLocal.java --- src/test/org/apache/commons/httpclient/TestGetMethodLocal.java 4 Feb 2002 15:26:43 -0000 1.3 +++ src/test/org/apache/commons/httpclient/TestGetMethodLocal.java 5 Dec 2002 21:07:31 -0000 @@ -89,6 +89,7 @@ // -------------------------------------------------------------- Constants private static final String host = System.getProperty("httpclient.test.localHost","127.0.0.1"); + private static final String webAppContext = System.getProperty("httpclient.test.webappContext"); private static final int port; static { String portString = System.getProperty("httpclient.test.localPort","8080"); @@ -223,6 +224,40 @@ } assertEquals(404,method.getStatusCode()); + } + + /** + * The intent of this test is to allow for the incomplete parsing of a GET + * response, and to make it particularly tricky, the GET response issues + * a Connection: close". + * + *

This wants to insure that a recoverable exception is not unexpectedly + * triggered.

+ */ + public void testGetResponseNotReadAutoRecover() { + + HttpClient client = new HttpClient(); + client.startSession(host, port); + + try { + // issue a GET with a connection: close, and don't parse the body. + String path = "/" + webAppContext + "/body"; + GetMethod method1 = new GetMethod(path); + method1.addRequestHeader("Connection", "close"); + client.executeMethod(method1); + assertEquals(0, method1.getRecoverableExceptionCount() ); + + // issue another GET. + GetMethod method2 = new GetMethod(path); + client.executeMethod(method2); + assertEquals(0, method2.getRecoverableExceptionCount() ); + + client.endSession(); + } + catch (IOException ioe) { + + fail("Problem executing method : " + ioe.toString() ); + } } } Index: src/test/org/apache/commons/httpclient/TestHttpClientLocalHost.java =================================================================== RCS file: /home/cvspublic/jakarta-commons/httpclient/src/test/org/apache/commons/httpclient/TestHttpClientLocalHost.java,v retrieving revision 1.4 diff -u -r1.4 TestHttpClientLocalHost.java --- src/test/org/apache/commons/httpclient/TestHttpClientLocalHost.java 3 Dec 2002 05:46:16 -0000 1.4 +++ src/test/org/apache/commons/httpclient/TestHttpClientLocalHost.java 5 Dec 2002 21:07:31 -0000 @@ -84,6 +84,10 @@ // -------------------------------------------------------------- Constants + private static final String host = "127.0.0.1"; + private static final int port = 8080; + private static final String webAppContext = System.getProperty("httpclient.test.webappContext"); + // ------------------------------------------------------------ Constructor @@ -100,12 +104,12 @@ } private HttpClient client = null; + private String getPath = null; private GetMethod getSlash = null; - private GetMethod getSlash2 = null; public void setUp() { + getPath = "/" + webAppContext + "/body"; client = new HttpClient(); - getSlash = new GetMethod("/"); } public void tearDown() { @@ -115,6 +119,7 @@ public void testExecuteMethod() throws Exception { client.startSession(host, port); + GetMethod getSlash = new GetMethod(getPath); assertEquals(200, client.executeMethod(getSlash)); String data = getSlash.getResponseBodyAsString(); assertTrue(null != data); @@ -125,13 +130,14 @@ public void testExecuteMultipleMethods() throws Exception { client.startSession(host, port); + getSlash = new GetMethod(getPath); for(int i=0;i<10;i++) { assertEquals(200, client.executeMethod(getSlash)); String data = getSlash.getResponseBodyAsString(); assertTrue(null != data); assertTrue(data.length() > 0); getSlash.recycle(); - getSlash.setPath("/"); + getSlash.setPath(getPath); } client.endSession(); } Index: src/test/org/apache/commons/httpclient/TestMethodsLocalHost.java =================================================================== RCS file: /home/cvspublic/jakarta-commons/httpclient/src/test/org/apache/commons/httpclient/TestMethodsLocalHost.java,v retrieving revision 1.4 diff -u -r1.4 TestMethodsLocalHost.java --- src/test/org/apache/commons/httpclient/TestMethodsLocalHost.java 1 Nov 2002 09:51:05 -0000 1.4 +++ src/test/org/apache/commons/httpclient/TestMethodsLocalHost.java 5 Dec 2002 21:07:31 -0000 @@ -88,6 +88,7 @@ // -------------------------------------------------------------- Constants + private static final String webAppContext = System.getProperty("httpclient.test.webappContext"); private static final String host = "127.0.0.1"; private static final int port = 8080; @@ -208,7 +209,8 @@ fail("Unable to execute method : " + t.toString()); } - HeadMethod method = new HeadMethod("/"); + String path = "/" + webAppContext + "/body"; + HeadMethod method = new HeadMethod(path); try { client.executeMethod(method); @@ -220,7 +222,7 @@ assertEquals(200, method.getStatusCode()); method.recycle(); - method.setPath("/index.html"); + method.setPath(path); try { client.executeMethod(method); Index: src/test/org/apache/commons/httpclient/TestMethodsNoHost.java =================================================================== RCS file: /home/cvspublic/jakarta-commons/httpclient/src/test/org/apache/commons/httpclient/TestMethodsNoHost.java,v retrieving revision 1.11 diff -u -r1.11 TestMethodsNoHost.java --- src/test/org/apache/commons/httpclient/TestMethodsNoHost.java 31 Oct 2002 07:45:35 -0000 1.11 +++ src/test/org/apache/commons/httpclient/TestMethodsNoHost.java 5 Dec 2002 21:07:31 -0000 @@ -71,6 +71,7 @@ import junit.framework.TestSuite; import org.apache.commons.httpclient.methods.GetMethod; import org.apache.commons.httpclient.methods.PostMethod; +import org.apache.commons.httpclient.methods.HeadMethod; /** * @author Rodney Waldhoff @@ -276,7 +277,8 @@ HttpMethodBase method = new GetMethod("/"); method.execute(new HttpState(), conn); String responseBody = method.getResponseBodyAsString(); - conn.close(); + // verify that the connection was closed. + conn.assertNotOpen(); assertEquals("1234567890123", responseBody); } @@ -300,7 +302,22 @@ while ((c = response.read()) != -1) { assertEquals((int) 'A', c); } - assertTrue(!conn.isOpen()); + conn.assertNotOpen(); + + // note - this test is here because the HEAD method handler overrides the + // standard behavior for reading a response body. + HeadMethod headMethod = new HeadMethod("/"); + + conn.addResponse(headers, ""); + + try { + headMethod.execute(new HttpState(), conn); + conn.assertNotOpen(); + + } catch (Throwable t) { + t.printStackTrace(); + fail("Unable to execute method : " + t.toString()); + } } public void testSetGetQueryString1() { Index: src/test/org/apache/commons/httpclient/TestStreams.java =================================================================== RCS file: /home/cvspublic/jakarta-commons/httpclient/src/test/org/apache/commons/httpclient/TestStreams.java,v retrieving revision 1.5 diff -u -r1.5 TestStreams.java --- src/test/org/apache/commons/httpclient/TestStreams.java 25 Oct 2002 10:15:52 -0000 1.5 +++ src/test/org/apache/commons/httpclient/TestStreams.java 5 Dec 2002 21:07:31 -0000 @@ -2,7 +2,6 @@ package org.apache.commons.httpclient; import java.io.*; -import java.util.Map; import junit.framework.*; import org.apache.commons.httpclient.methods.GetMethod; --------------020200080500070203010001--