The situation causing the problem is the following:
1) The HttpClient makes a connection to the proxy server for a site.
2) The proxy server connects to the remote web server and gets a 302
redirect response
3) The proxy server returns the 302 response back to the HttpClient
4) The proxy server closes the socket between it and the HttpClient.
5) The HttpClient assumes that the connection is still open and tries to
make the redirect down the same connection
6) Once the connection to the proxy is closed no data can be written to the
proxy or read from the next request. This results in the exception being
thrown.
I would not think that the client should assume that the connection
automatically closes as this is not the case for the vast majority of
servers that it would connect to. One of the problems with the recoverable
exception is that is telling me that I am failing, but I am failing on the
redirect rather than on the initial request. So if I try the exact same
request again, the exception will still occur.
So for a particular request, I do not explicity know that I will need a
redirect until I get the initial response from the external server. But I
do know that if I do get such a response, I want to make a new connection to
the server. You are correct in saying the reason that I know this is
because I am setting up the connection to the proxy server, I see your point
there. But it seems odd to me to have that attribute in the connection. I
guess I look at the attributes of HttpConnection as those pieces of data
involved in making the connection rather than being involved with decisions
about whether the connection should be open or closed. That is why I still
think that the value belongs in the HttpMethodBase. If the methods are
defined in this class, I think HttpMethod should be modified as well for the
sake of consistency as the other attributes of the class are defined in the
interface.
As of yet, I haven't written any test cases for this problem.
----- Original Message -----
From: "Eric Johnson" <[EMAIL PROTECTED]>
To: "Commons HttpClient Project" <[EMAIL PROTECTED]>
Sent: Friday, January 24, 2003 6:06 PM
Subject: Re: [PATCH] Closing HttpConnection before retrying
> 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:[EMAIL PROTECTED]> For
> >>>additional commands, e-mail:
> >>><mailto:[EMAIL PROTECTED]>
> >>>
> >>>
> >
> >
> >
> >------------------------------------------------------------------------
> >
> >Index: src/java/org/apache/commons/httpclient/HttpMethodBase.java
> >===================================================================
> >RCS file:
/home/cvspublic/jakarta-commons/httpclient/src/java/org/apache/commons/httpc
lient/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/httpc
lient/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:[EMAIL PROTECTED]>
> >For additional commands, e-mail:
<mailto:[EMAIL PROTECTED]>
> >
>
>
> --
> To unsubscribe, e-mail:
<mailto:[EMAIL PROTECTED]>
> For additional commands, e-mail:
<mailto:[EMAIL PROTECTED]>
>
--
To unsubscribe, e-mail:
<mailto:[EMAIL PROTECTED]>
For additional commands, e-mail:
<mailto:[EMAIL PROTECTED]>
