Index: 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.21 diff -u -r1.21 ConnectMethod.java --- java/org/apache/commons/httpclient/ConnectMethod.java 12 Aug 2003 02:35:17 -0000 1.21 +++ java/org/apache/commons/httpclient/ConnectMethod.java 8 Nov 2003 21:31:26 -0000 @@ -196,10 +196,6 @@ if (LOG.isDebugEnabled()) { LOG.debug("CONNECT status code " + code); } - if ((code >= 200) && (code < 300)) { - conn.tunnelCreated(); - } - return code; } Index: java/org/apache/commons/httpclient/HttpClient.java =================================================================== RCS file: /home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/httpclient/HttpClient.java,v retrieving revision 1.88 diff -u -r1.88 HttpClient.java --- java/org/apache/commons/httpclient/HttpClient.java 26 Oct 2003 09:49:16 -0000 1.88 +++ java/org/apache/commons/httpclient/HttpClient.java 8 Nov 2003 21:31:29 -0000 @@ -423,21 +423,10 @@ } HostConfiguration defaultHostConfiguration = null; - HttpMethodDirector methodDirector = new HttpMethodDirector(); - - /* access all synchronized data in a single block, this will keeps us - * from accessing data asynchronously as well having to regain the lock - * for each item. - */ synchronized (this) { - methodDirector.setParams(this.params); - methodDirector.setState(state == null ? getState() : state); - methodDirector.setConnectionManager(this.httpConnectionManager); defaultHostConfiguration = getHostConfiguration(); } - HostConfiguration methodConfiguration = new HostConfiguration(hostConfiguration); - if (hostConfiguration != defaultHostConfiguration) { // we may need to apply some defaults if (!methodConfiguration.isHostSet()) { @@ -463,12 +452,21 @@ } } - methodDirector.setHostConfiguration(methodConfiguration); - methodDirector.setMethod(method); - - methodDirector.executeMethod(); - - return methodDirector.getMethod().getStatusCode(); + /* access all synchronized data in a single block, this will keeps us + * from accessing data asynchronously as well having to regain the lock + * for each item. + */ + HttpMethodDirector methodDirector = null; + synchronized (this) { + methodDirector = new HttpMethodDirector( + this.httpConnectionManager, + methodConfiguration, + this.params, + (state == null ? getState() : state)); + defaultHostConfiguration = getHostConfiguration(); + } + methodDirector.executeMethod(method); + return method.getStatusCode(); } /** Index: java/org/apache/commons/httpclient/HttpMethod.java =================================================================== RCS file: /home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/httpclient/HttpMethod.java,v retrieving revision 1.30 diff -u -r1.30 HttpMethod.java --- java/org/apache/commons/httpclient/HttpMethod.java 3 Oct 2003 20:57:35 -0000 1.30 +++ java/org/apache/commons/httpclient/HttpMethod.java 8 Nov 2003 21:31:32 -0000 @@ -540,4 +540,18 @@ */ public void setParams(final HttpMethodParams params); + /** + * Returns the {@link MethodRetryHandler retry handler} for this HTTP method + * + * @return the methodRetryHandler + */ + public MethodRetryHandler getMethodRetryHandler(); + + /** + * Sets the {@link MethodRetryHandler retry handler} for this HTTP method + * + * @param handler the methodRetryHandler to use when this method executed + */ + public void setMethodRetryHandler(MethodRetryHandler handler); + } Index: java/org/apache/commons/httpclient/HttpMethodDirector.java =================================================================== RCS file: /home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/httpclient/HttpMethodDirector.java,v retrieving revision 1.7 diff -u -r1.7 HttpMethodDirector.java --- java/org/apache/commons/httpclient/HttpMethodDirector.java 22 Oct 2003 19:31:00 -0000 1.7 +++ java/org/apache/commons/httpclient/HttpMethodDirector.java 8 Nov 2003 21:31:36 -0000 @@ -77,7 +77,7 @@ import org.apache.commons.logging.LogFactory; /** - * Handles the process of executing a method including authentication, redirection and retries. + * Handles the process of executing a this.method including authentication, redirection and retries. */ class HttpMethodDirector { @@ -86,24 +86,21 @@ private static final Log LOG = LogFactory.getLog(HttpMethodDirector.class); - private HttpMethod method; - + private HttpMethod method; + private HttpState state; private HostConfiguration hostConfiguration; private HttpConnectionManager connectionManager; - private HttpConnection connection; - private HttpClientParams params; - /** A flag to indicate if the connection should be released after the method is executed. */ + private HttpConnection conn; + + /** A flag to indicate if the this.conn should be released after the this.method is executed. */ private boolean releaseConnection = false; - /** How many times did this transparently handle a recoverable exception? */ - private int recoverableExceptionCount = 0; - /** Realms that we tried to authenticate to */ private Set realms = null; @@ -115,17 +112,35 @@ /** Actual proxy authentication realm */ private String proxyRealm = null; + + public HttpMethodDirector( + final HttpConnectionManager connectionManager, + final HostConfiguration hostConfiguration, + final HttpClientParams params, + final HttpState state + ) { + super(); + this.connectionManager = connectionManager; + this.hostConfiguration = hostConfiguration; + this.params = params; + this.state = state; + } + /** - * Executes the method associated with this method director. + * Executes the this.method associated with this this.method director. * * @throws IOException * @throws HttpException */ - public void executeMethod() throws IOException, HttpException { - - method.getParams().setDefaults(this.params); + public void executeMethod(final HttpMethod method) throws IOException, HttpException { + if (method == null) + { + throw new IllegalArgumentException("Method may not be null"); + } + this.method = method; + this.method.getParams().setDefaults(this.params); try { int forwardCount = 0; //protect from an infinite loop @@ -135,8 +150,27 @@ LOG.debug("Execute loop try " + forwardCount); } - executeMethodForHost(); + // make sure the this.conn we have is appropriate + if (this.conn != null && !hostConfiguration.hostEquals(this.conn)) { + this.conn.setLocked(false); + this.conn.releaseConnection(); + this.conn = null; + } + + // get a this.conn, if we need one + if (this.conn == null) { + this.conn = connectionManager.getConnectionWithTimeout( + hostConfiguration, + this.params.getConnectionManagerTimeout() + ); + this.conn.setLocked(true); + realms = new HashSet(); + proxyRealms = new HashSet(); + + addPreemtiveAuthenticationHeaders(); + } + executeWithRetry(this.method); if (!isRetryNeeded()) { // nope, no retry needed, exit loop. break; @@ -144,9 +178,9 @@ // retry - close previous stream. Caution - this causes // responseBodyConsumed to be called, which may also close the - // connection. - if (method.getResponseBodyAsStream() != null) { - method.getResponseBodyAsStream().close(); + // this.conn. + if (this.method.getResponseBodyAsStream() != null) { + this.method.getResponseBodyAsStream().close(); } } //end of retry loop @@ -158,19 +192,19 @@ } } finally { - if (connection != null) { - connection.setLocked(false); + if (this.conn != null) { + this.conn.setLocked(false); } - // If the response has been fully processed, return the connection + // If the response has been fully processed, return the this.conn // 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 ( - (releaseConnection || method.getResponseBodyAsStream() == null) - && connection != null + (releaseConnection || this.method.getResponseBodyAsStream() == null) + && this.conn != null ) { - connection.releaseConnection(); + this.conn.releaseConnection(); } } @@ -189,11 +223,11 @@ LOG.debug("Preemptively sending default basic credentials"); try { - if (HttpAuthenticator.authenticateDefault(method, connection, state)) { + if (HttpAuthenticator.authenticateDefault(this.method, this.conn, state)) { LOG.debug("Default basic credentials applied"); } - if (connection.isProxied()) { - if (HttpAuthenticator.authenticateProxyDefault(method, connection, state)) { + if (this.conn.isProxied()) { + if (HttpAuthenticator.authenticateProxyDefault(this.method, this.conn, state)) { LOG.debug("Default basic proxy credentials applied"); } } @@ -205,121 +239,72 @@ } /** - * Makes sure there is a connection allocated and that it is valid and open. - * - * @return true if a valid connection was established, - * false otherwise - * - * @throws IOException - * @throws HttpException - */ - private boolean establishValidOpenConnection() throws IOException, HttpException { - - // make sure the connection we have is appropriate - if (connection != null && !hostConfiguration.hostEquals(connection)) { - connection.setLocked(false); - connection.releaseConnection(); - connection = null; - } - - // get a connection, if we need one - if (connection == null) { - connection = connectionManager.getConnectionWithTimeout( - hostConfiguration, - this.params.getConnectionManagerTimeout() - ); - connection.setLocked(true); - - realms = new HashSet(); - proxyRealms = new HashSet(); - - addPreemtiveAuthenticationHeaders(); - } - - try { - // Catch all possible exceptions to make sure to release the - // connection, as although the user may call - // Method->releaseConnection(), the method doesn't know about the - // connection until HttpMethod.execute() is called. - - if (!connection.isOpen()) { - // this connection must be opened before it can be used - connection.open(); - if (connection.isProxied() && connection.isSecure()) { - // we need to create a secure tunnel before we can execute the real method - if (!executeConnect()) { - // abort, the connect method failed - return false; - } - } - } else if ( - !(method instanceof ConnectMethod) - && connection.isProxied() - && connection.isSecure() - && !connection.isTransparent() - ) { - // this connection is open but the secure tunnel has not be created yet, - // execute the connect again - if (!executeConnect()) { - // abort, the connect method failed - return false; - } - } - - } catch (IOException e) { - releaseConnection = true; - throw e; - } catch (RuntimeException e) { - releaseConnection = true; - throw e; - } - - return true; - } - - /** - * Executes a method with the current hostConfiguration. + * Executes a this.method with the current hostConfiguration. * * @throws IOException if an I/O (transport) error occurs. Some transport exceptions * can be recovered from. * @throws HttpException if a protocol exception occurs. Usually protocol exceptions * cannot be recovered from. */ - private void executeMethodForHost() throws IOException, HttpException { + private void executeWithRetry(final HttpMethod curmethod) + throws IOException, HttpException { + /** How many times did this transparently handle a recoverable exception? */ + int recoverableExceptionCount = 0; int execCount = 0; // TODO: how do we get requestSent? boolean requestSent = false; - // loop until the method is successfully processed, the retryHandler + // loop until the this.method is successfully processed, the retryHandler // returns false or a non-recoverable exception is thrown while (true) { execCount++; requestSent = false; - if (!establishValidOpenConnection()) { - return; - } - if (LOG.isTraceEnabled()) { LOG.trace("Attempt number " + execCount + " to process request"); } + if (!this.conn.isOpen()) { + // this this.conn must be opened before it can be used + // This has nothing to do with opening a secure tunnel + try { + this.conn.open(); + if (this.conn.isProxied() && this.conn.isSecure() + && !(curmethod instanceof ConnectMethod)) { + // we need to create a secure tunnel before we can execute the real this.method + if (!executeConnect()) { + // abort, the connect this.method failed + break; + } + } + } catch (IOException e) { + releaseConnection = true; + throw e; + } catch (RuntimeException e) { + releaseConnection = true; + throw e; + } + } try { - method.execute(state, connection); + curmethod.execute(state, this.conn); break; } catch (HttpRecoverableException httpre) { if (LOG.isDebugEnabled()) { - LOG.debug("Closing the connection."); + LOG.debug("Closing the this.conn."); } - connection.close(); + this.conn.close(); LOG.info("Recoverable exception caught when processing request"); // update the recoverable exception count. recoverableExceptionCount++; - // test if this method should be retried - if (!getMethodRetryHandler().retryMethod( - method, - connection, + // test if this this.method should be retried + MethodRetryHandler handler = curmethod.getMethodRetryHandler(); + if (handler == null) { + handler = new DefaultMethodRetryHandler(); + } + if (!handler.retryMethod( + curmethod, + this.conn, httpre, execCount, requestSent) @@ -328,7 +313,7 @@ "Recoverable exception caught but MethodRetryHandler.retryMethod() " + "returned false, rethrowing exception" ); - // this connection can no longer be used, it has been closed + // this this.conn can no longer be used, it has been closed releaseConnection = true; throw httpre; } @@ -337,68 +322,47 @@ } - private MethodRetryHandler getMethodRetryHandler() { - - if (method instanceof HttpMethodBase) { - return ((HttpMethodBase) method).getMethodRetryHandler(); - } else { - return new DefaultMethodRetryHandler(); - } - } - /** - * Executes a ConnectMethod to establish a tunneled connection. + * Executes a ConnectMethod to establish a tunneled this.conn. * * @return true if the connect was successful * * @throws IOException * @throws HttpException */ - private boolean executeConnect() throws IOException, HttpException { + private boolean executeConnect() + throws IOException, HttpException { ConnectMethod connectMethod = new ConnectMethod(); - - HttpMethod tempMethod = this.method; - this.method = connectMethod; - - try { - executeMethod(); - } catch (HttpException e) { - this.method = tempMethod; - throw e; - } catch (IOException e) { - this.method = tempMethod; - throw e; - } - - int code = method.getStatusCode(); + executeWithRetry(connectMethod); + int code = connectMethod.getStatusCode(); if ((code >= 200) && (code < 300)) { - this.method = tempMethod; + this.conn.tunnelCreated(); return true; } else { // What is to follow is an ugly hack. // I REALLY hate having to resort to such // an appalling trick - // TODO: Connect method must be redesigned. + // TODO: Connect this.method must be redesigned. // The only feasible solution is to split monolithic // HttpMethod into HttpRequest/HttpResponse pair. - // That would allow to execute CONNECT method + // That would allow to execute CONNECT this.method // behind the scene and return CONNECT HttpResponse // object in response to the original request that // contains the correct status line, headers & // response body. - LOG.debug("CONNECT failed, fake the response for the original method"); + LOG.debug("CONNECT failed, fake the response for the original this.method"); // Pass the status, headers and response stream to the wrapped - // method. - // To ensure that the connection is not released more than once - // this method is still responsible for releasing the connection. + // this.method. + // To ensure that the this.conn is not released more than once + // this this.method is still responsible for releasing the this.conn. // This will happen when the response body is consumed, or when - // the wrapped method closes the response connection in + // the wrapped this.method closes the response this.conn in // releaseConnection(). - if (tempMethod instanceof HttpMethodBase) { - ((HttpMethodBase) tempMethod).fakeResponse( + if (this.method instanceof HttpMethodBase) { + ((HttpMethodBase) this.method).fakeResponse( connectMethod.getStatusLine(), connectMethod.getResponseHeaderGroup(), connectMethod.getResponseBodyAsStream() @@ -406,9 +370,8 @@ } else { releaseConnection = true; LOG.warn( - "Unable to fake response on method as it is not derived from HttpMethodBase."); + "Unable to fake response on this.method as it is not derived from HttpMethodBase."); } - this.method = tempMethod; return false; } } @@ -420,17 +383,17 @@ */ private boolean processRedirectResponse() { - if (!method.getFollowRedirects()) { + if (!this.method.getFollowRedirects()) { LOG.info("Redirect requested but followRedirects is " + "disabled"); return false; } //get the location header to find out where to redirect to - Header locationHeader = method.getResponseHeader("location"); + Header locationHeader = this.method.getResponseHeader("location"); if (locationHeader == null) { // got a redirect response, but no location header - LOG.error("Received redirect response " + method.getStatusCode() + LOG.error("Received redirect response " + this.method.getStatusCode() + " but no location header"); return false; } @@ -447,15 +410,15 @@ try { currentUri = new URI( - connection.getProtocol().getScheme(), + this.conn.getProtocol().getScheme(), null, - connection.getHost(), - connection.getPort(), - method.getPath() + this.conn.getHost(), + this.conn.getPort(), + this.method.getPath() ); redirectUri = new URI(location, true); if (redirectUri.isRelativeURI()) { - if (method.isStrictMode()) { + if (this.method.isStrictMode()) { LOG.warn("Redirected location '" + location + "' is not acceptable in strict mode"); return false; @@ -473,12 +436,12 @@ //invalidate the list of authentication attempts this.realms.clear(); //remove exisitng authentication headers - method.removeRequestHeader(HttpAuthenticator.WWW_AUTH_RESP); + this.method.removeRequestHeader(HttpAuthenticator.WWW_AUTH_RESP); //update the current location with the redirect location. //avoiding use of URL.getPath() and URL.getQuery() to keep //jdk1.2 comliance. - method.setPath(redirectUri.getEscapedPath()); - method.setQueryString(redirectUri.getEscapedQuery()); + this.method.setPath(redirectUri.getEscapedPath()); + this.method.setQueryString(redirectUri.getEscapedQuery()); hostConfiguration.setHost(redirectUri); if (LOG.isDebugEnabled()) { @@ -493,33 +456,33 @@ * Processes a response that requires authentication * * @param state the current state - * @param conn The connection + * @param conn The this.conn * * @return true if the request has completed processing, false * if more attempts are needed */ - private boolean processAuthenticationResponse(HttpState state, HttpConnection conn) { + private boolean processAuthenticationResponse() { LOG.trace("enter HttpMethodBase.processAuthenticationResponse(" + "HttpState, HttpConnection)"); - int statusCode = method.getStatusCode(); + int statusCode = this.method.getStatusCode(); // handle authentication required Header[] challenges = null; Set realmsUsed = null; String host = null; switch (statusCode) { case HttpStatus.SC_UNAUTHORIZED: - challenges = method.getResponseHeaders(HttpAuthenticator.WWW_AUTH); + challenges = this.method.getResponseHeaders(HttpAuthenticator.WWW_AUTH); realmsUsed = realms; - host = conn.getVirtualHost(); + host = this.conn.getVirtualHost(); if (host == null) { - host = conn.getHost(); + host = this.conn.getHost(); } break; case HttpStatus.SC_PROXY_AUTHENTICATION_REQUIRED: - challenges = method.getResponseHeaders(HttpAuthenticator.PROXY_AUTH); + challenges = this.method.getResponseHeaders(HttpAuthenticator.PROXY_AUTH); realmsUsed = proxyRealms; - host = conn.getProxyHost(); + host = this.conn.getProxyHost(); break; } boolean authenticated = false; @@ -554,7 +517,7 @@ buffer.append("' authentication realm at "); buffer.append(host); buffer.append(", but still receiving: "); - buffer.append(method.getStatusLine().toString()); + buffer.append(this.method.getStatusLine().toString()); LOG.info(buffer.toString()); } return true; @@ -562,19 +525,19 @@ realmsUsed.add(realm); } + this.method.removeRequestHeader(HttpAuthenticator.WWW_AUTH_RESP); + this.method.removeRequestHeader(HttpAuthenticator.PROXY_AUTH_RESP); try { //remove preemptive header and reauthenticate switch (statusCode) { case HttpStatus.SC_UNAUTHORIZED: - method.removeRequestHeader(HttpAuthenticator.WWW_AUTH_RESP); authenticated = HttpAuthenticator.authenticate( - authscheme, method, conn, state); + authscheme, this.method, this.conn, this.state); this.realm = authscheme.getRealm(); break; case HttpStatus.SC_PROXY_AUTHENTICATION_REQUIRED: - method.removeRequestHeader(HttpAuthenticator.PROXY_AUTH_RESP); authenticated = HttpAuthenticator.authenticateProxy( - authscheme, method, conn, state); + authscheme, this.method, this.conn, this.state); this.proxyRealm = authscheme.getRealm(); break; } @@ -611,14 +574,14 @@ * @return boolean true if a retry is needed. */ private boolean isRetryNeeded() { - switch (method.getStatusCode()) { + switch (this.method.getStatusCode()) { case HttpStatus.SC_UNAUTHORIZED: case HttpStatus.SC_PROXY_AUTHENTICATION_REQUIRED: LOG.debug("Authorization required"); - if (method.getDoAuthentication()) { //process authentication response + if (this.method.getDoAuthentication()) { //process authentication response //if the authentication is successful, return the statusCode //otherwise, drop through the switch and try again. - if (processAuthenticationResponse(state, connection)) { + if (processAuthenticationResponse()) { return false; } } else { //let the client handle the authenticaiton @@ -653,27 +616,6 @@ } /** - * @param hostConfiguration - */ - public void setHostConfiguration(HostConfiguration hostConfiguration) { - this.hostConfiguration = hostConfiguration; - } - - /** - * @return - */ - public HttpMethod getMethod() { - return method; - } - - /** - * @param method - */ - public void setMethod(HttpMethod method) { - this.method = method; - } - - /** * @return */ public HttpState getState() { @@ -681,13 +623,6 @@ } /** - * @param state - */ - public void setState(HttpState state) { - this.state = state; - } - - /** * @return */ public HttpConnectionManager getConnectionManager() { @@ -695,24 +630,10 @@ } /** - * @param connectionManager - */ - public void setConnectionManager(HttpConnectionManager connectionManager) { - this.connectionManager = connectionManager; - } - - /** * @return */ public HttpParams getParams() { return this.params; - } - - /** - * @param params - */ - public void setParams(final HttpClientParams params) { - this.params = params; } }