hc-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Eric Johnson <e...@tibco.com>
Subject Re: [PATCH] Closing HttpConnection before retrying
Date Fri, 24 Jan 2003 23:06:38 GMT
Rob,

Thanks for the further clarifications.  One critical point I missed with 
my previous response - do you have any relevant test cases to add?

Also, you say below "[t]he proxy server is not sending us any 
notification of closing the connection. The only way that we notice the 
closure is when we read the from the socket's input stream".  I see two 
possibilities here, then.  Either the proxy server being badly behaved, 
or HttpClient should be assuming that the connection is closed.  The 
second option doesn't seem to be the case, or your fix wouldn't have 
made it optional.  If the server is badly behaved, I don't see the 
problem with having the subsequent attempt to connect generating a 
"recoverable" exception.  What _could_ be a problem, however, is if 
HttpClient gives up too soon, and ends up throwing the exception back to 
the caller before trying one more time with a fresh connection.  Is this 
the problem you're seeing?  Can you more carefully describe your failure 
case?

How is it that you _know_ for a particular request, both that you will 
get a redirect, and that you will need to close the connection before 
retrying?  I don't understand how you could know this except to always 
assume it for your proxy server, in which case, it would seem to be a 
"connection" attribute, as before.  I asked the question before about 
whether the HttpMethod interface should have the functions partly 
rhetorically - any additional functions to this already too complicated 
interface need careful justification.  Serves me right for asking a 
rhetorical question :-) !

Based on the above, I bet you can tell that I'm still not sure that your 
solution is in the correct place.  On the other hand, I hope I'm not 
discouraging you.  I spent almost a month of back and forth (not full 
time, certainly - I'm not that slow!) trying to get a patch to handle 
the closing of streams and connections correctly.  I know it is hard to 
work on this particular area of httpclient, and I for one appreciate 
your persistence in your efforts to improve the code.

-Eric.

Rob Di Marco wrote:

>I'll address each point below.  Based on your feedback, I have prepared 
>another version of the patch.
>
>  
>
>>    * The new "get/setReinitializeConnectionOnRetry" pair seems
>>      suspiciously like a property of the 'connection', rather than the
>>      method.  Apparently, based on your patch, you think differently.
>>       Can you say why?
>>    
>>
>
>I viewed this property as an attribute of the overall request and not 
>necessarily of one particular connection of the request.  I see it as similar 
>to the followRedirects attribute that is defined for the HttpMethodBase.  It 
>is an attribute that tells the HttpMethodBase how to perform based on certain 
>data in the HTTP response. For the followRedirects attribute, it tells the 
>HttpMethodBase whether to follow the redirect, while in the 
>reinitializeConnection attributes case, it tells the HttpMethodBase that a 
>new connection is required before making retrying the request.
>
>  
>
>>    * If these functions are required as part of the functionality of
>>      HttpMethodBase, contrary to the previous point, do they also
>>      belong on HttpMethod?  If not, why not?
>>    
>>
>
>I am new to using the project and did not notice the HttpMethod interface.  I 
>would agree that the method definitions should be included in the interface 
>as well.
>
>  
>
>>    * Is the proxy server sending any indication that it is closing the
>>      connection?  If so, various parts of HttpMethodBase _should_
>>      already be detecting that the connection should be closed.  If
>>      HttpMethodBase is not detecting that the connection should be
>>      closed, why not?
>>    
>>
>
>The proxy server is not sending us any notification of closing the connection.  
>The only way that we notice the closure is when we read the from the socket's 
>input stream and immediately get back -1.
>
>  
>
>>    * Since the if condition you altered near the end of executeMethod()
>>      exists to consume the remainder of the response, or close the
>>      connection, based on the response stream, it would seem that your
>>      changes are needed when the responseStream is null (and perhaps
>>      shouldn't be?), or when the connection is being closed by the
>>      server without telling the client.  In the latter case, I would
>>      think it more appropriate to have a "recoverable" exception
>>      thrown, which exists for precisely those scenarios where the
>>      server "unexpectedly" closes the connection - which is to say that
>>      the connection dropped without any notification from the server
>>      that it was going to do so.
>>
>>    
>>
>
>The reason for changing the if condition is that the stream was already closed 
>when the connection was closed.  Perhaps the releaseConnection method should 
>be called here as a better alternative to the if statement entirely.  
>This method:
>1) Does the null check
>2) Handles the situation that the steam was already closed by catching the 
>exception.
>
>This is exactly the functionality we would want in either situation 
>(connection closed or not).
>
>Hope this answers your questions.  Let me know
>
>  
>
>>-Eric Johnson
>>
>>Rob Di Marco wrote:
>>    
>>
>>>We have recently experienced a problem when making an HTTP request through
>>>a proxy server when the response was a 302 - Redirect.  The
>>>HttpMethodBase object would attempt to use the same HttpConnection
>>>instance to make the redirect request.  However, the proxy server was
>>>closing the connection before the request could be made, so no data is
>>>read for the request, resulting in an HttpRecoverableException being
>>>thrown.
>>>
>>>The solution to this problem was to add a field to the HttpMethodBase
>>>object, reinitializeConnectionOnRetry and by forcing the connection to
>>>close anytime a retry is warranted.
>>>
>>>I have attached the patch to the message.
>>>
>>>Thanks.
>>>
>>>
>>>------------------------------------------------------------------------
>>>
>>>Index: src/java/org/apache/commons/httpclient/HttpMethodBase.java
>>>===================================================================
>>>RCS file:
>>>/home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/ht
>>>tpclient/HttpMethodBase.java,v retrieving revision 1.96
>>>diff -u -r1.96 HttpMethodBase.java
>>>--- src/java/org/apache/commons/httpclient/HttpMethodBase.java	23 Jan 2003
>>>22:47:47 -0000	1.96 +++
>>>src/java/org/apache/commons/httpclient/HttpMethodBase.java	24 Jan 2003
>>>18:55:44 -0000 @@ -252,6 +252,7 @@
>>>
>>>    private boolean doneWithConnection = false;
>>>
>>>+    private boolean reinitializeConnectionOnRetry = false;
>>>    //~ Constructors
>>>···························································
>>>
>>>    // -----------------------------------------------------------
>>>Constructors @@ -366,6 +367,26 @@
>>>    }
>>>
>>>    /**
>>>+     * Whether connections should be reinitialized across redirects.
>>>+     *
>>>+     * @param reinitializeConnectionOnRetry true to close and reopen a
>>>+     * connection, false otherwise.
>>>+     */
>>>+    public void setReinitializeConnectionOnRetry(boolean
>>>reinitializeConnectionOnRetry) { +       
>>>this.reinitializeConnectionOnRetry = reinitializeConnectionOnRetry; +   
>>>}
>>>+
>>>+    /**
>>>+     * Whether or not I should reconnect before following HTTP redirects
>>>+     * (status code 302, etc.)
>>>+     *
>>>+     * @return <tt>true</tt> if I will reconnect before follow HTTP
>>>redirects +     */
>>>+    public boolean getReinitializeConnectionOnRetry() {
>>>+        return this.reinitializeConnectionOnRetry;
>>>+    }
>>>+
>>>+    /**
>>>     * Set whether or not I should use the HTTP/1.1 protocol.
>>>     *
>>>     * @param http11 true to use HTTP/1.1, false to use 1.0
>>>@@ -929,7 +950,19 @@
>>>                // retry - close previous stream.  Caution - this causes
>>>                // responseBodyConsumed to be called, which may also
>>>close the // connection.
>>>-                if (responseStream != null) {
>>>+
>>>+                if (reinitializeConnectionOnRetry) {
>>>+
>>>+                    if (log.isDebugEnabled()) {
>>>+
>>>+                        log.debug("Closing the connection");
>>>+
>>>+                    }
>>>+
>>>+                    conn.close();
>>>+                }
>>>+
>>>+                if (conn.isOpen() && responseStream != null) {
>>>                    responseStream.close();
>>>                }
>>>
>>>
>>>
>>>
>>>------------------------------------------------------------------------
>>>
>>>--
>>>To unsubscribe, e-mail:  
>>><mailto:commons-httpclient-dev-unsubscribe@jakarta.apache.org> For
>>>additional commands, e-mail:
>>><mailto:commons-httpclient-dev-help@jakarta.apache.org>
>>>      
>>>
>
>  
>
>------------------------------------------------------------------------
>
>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.96
>diff -u -r1.96 HttpMethodBase.java
>--- src/java/org/apache/commons/httpclient/HttpMethodBase.java	23 Jan 2003 22:47:47 -0000
1.96
>+++ src/java/org/apache/commons/httpclient/HttpMethodBase.java	24 Jan 2003 20:25:11 -0000
>@@ -252,6 +252,7 @@
> 
>     private boolean doneWithConnection = false;
> 
>+    private boolean reinitializeConnectionOnRetry = false;
>     //~ Constructors ···························································
> 
>     // ----------------------------------------------------------- Constructors
>@@ -366,6 +367,26 @@
>     }
> 
>     /**
>+     * Whether connections should be reinitialized across redirects.
>+     *
>+     * @param reinitializeConnectionOnRetry true to close and reopen a 
>+     * connection, false otherwise.
>+     */
>+    public void setReinitializeConnectionOnRetry(boolean reinitializeConnectionOnRetry)
{
>+        this.reinitializeConnectionOnRetry = reinitializeConnectionOnRetry;
>+    }
>+
>+    /**
>+     * Whether or not I should reconnect before following HTTP redirects
>+     * (status code 302, etc.)
>+     *
>+     * @return <tt>true</tt> if I will reconnect before follow HTTP redirects
>+     */
>+    public boolean getReinitializeConnectionOnRetry() {
>+        return this.reinitializeConnectionOnRetry;
>+    }
>+
>+    /**
>      * Set whether or not I should use the HTTP/1.1 protocol.
>      *
>      * @param http11 true to use HTTP/1.1, false to use 1.0
>@@ -926,12 +947,21 @@
>                 visited.add(generateVisitedKey(conn));
>                 */
> 
>+                if (reinitializeConnectionOnRetry) {
>+
>+                    if (log.isDebugEnabled()) {
>+
>+                        log.debug("Closing the connection");
>+
>+                    }
>+
>+                    conn.close();
>+                }
>+
>                 // retry - close previous stream.  Caution - this causes
>                 // responseBodyConsumed to be called, which may also close the
>                 // connection.
>-                if (responseStream != null) {
>-                    responseStream.close();
>-                }
>+                releaseConnection();
> 
>             } //end of retry loop
> 
>Index: src/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.22
>diff -u -r1.22 HttpMethod.java
>--- src/java/org/apache/commons/httpclient/HttpMethod.java	23 Jan 2003 22:47:46 -0000
1.22
>+++ src/java/org/apache/commons/httpclient/HttpMethod.java	24 Jan 2003 20:25:11 -0000
>@@ -206,6 +206,22 @@
>     public void setFollowRedirects(boolean followRedirects);
> 
>     /**
>+     * Whether connections should be reinitialized across redirects.
>+     *
>+     * @param reinitializeConnectionOnRetry true to close and reopen a 
>+     * connection, false otherwise.
>+     */
>+    public void setReinitializeConnectionOnRetry(boolean reinitializeConnectionOnRetry);
>+
>+    /**
>+     * Whether or not I should reconnect before following HTTP redirects
>+     * (status code 302, etc.)
>+     *
>+     * @return <tt>true</tt> if I will reconnect before follow HTTP redirects
>+     */
>+    public boolean getReinitializeConnectionOnRetry();
>+
>+    /**
>      * Set my query string.
>      * @param queryString the query string
>      */
>
>  
>
>------------------------------------------------------------------------
>
>--
>To unsubscribe, e-mail:   <mailto:commons-httpclient-dev-unsubscribe@jakarta.apache.org>
>For additional commands, e-mail: <mailto:commons-httpclient-dev-help@jakarta.apache.org>
>


Mime
View raw message