You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by Rob Di Marco <rd...@hmsonline.com> on 2003/01/24 20:18:28 UTC
[PATCH] Closing HttpConnection before retrying
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.
--
Rob Di Marco
Re: [PATCH] Closing HttpConnection before retrying
Posted by Jeffrey Dever <js...@sympatico.ca>.
A JUnit test case that we could patch into the HttpClient tests would be
ideal ...
>
>
>> As of yet, I haven't written any test cases for this problem.
>>
> Perhaps you can provide the stack trace of the exception failure, or a
> more specific "trace" of the scenario, mentioning routine names (and
> line numbers?) as I have attempted to do based on your description?
> You might also consider including a "wire" level debug trace, and/or
> snooping on the connection (see the mailing list archive for tips on
> how you might do this). Maybe, you could augmenting
> SimpleHttpConnection so that it can mimic the unexpected close for
> you, and make your case 100% reproducible for others.
>
> -Eric.
>
>
> --
> To unsubscribe, e-mail:
> <ma...@jakarta.apache.org>
> For additional commands, e-mail:
> <ma...@jakarta.apache.org>
>
>
Re: [PATCH] Closing HttpConnection before retrying
Posted by Eric Johnson <er...@tibco.com>.
Rob,
OK, your description helps. I'm trying to step through the code in my
head, though, trying to figure out where things are going wrong, as you
didn't describe the HttpClient routines that fail. I augmented your
steps through the failure case to see if I can understand why it doesn't
work.
Rob Di Marco wrote:
>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
>
>
- aka. HttpMethodBase.execute() calls writeRequest(), followed by
readResponse(). Of course, you didn't mention whether this was a "GET",
"HEAD", "POST", or some other request, which could matter.
>3) The proxy server returns the 302 response back to the HttpClient
>4) The proxy server closes the socket between it and the HttpClient.
>
- at this point HttpClient is oblivious, the execute() method continues,
invoking isRetryNeeded() - which returns "true".
>5) The HttpClient assumes that the connection is still open and tries to
>make the redirect down the same connection
>
execute() proceeds apace, and invokes responseStream.close() - still no
exceptions thrown in any of this behavior so far. (Or is this throwing
an exception in your scenario?!)
>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.
>
Since execute() still has a "forwardCount" less than "maxForwards", it
loops around, invoking processRequest again. As you note,
processRequest() calls writeRequest(), which (correctly!) throws an
HttpRecoverableException (It is unclear to me whether you are
complaining that an exception is thrown, which I think is correct
behavior in your described scenario, or rather that an exception is
thrown, but not caught?).
At this point, I become confused. I expect, at this point, that
processRequest() would catch the exception, close the connection, and
loop again, reopening the connection and retrying the request, and this
time it would succeed. Based on your writeups, however, I'm wondering
whether things are out of whack at this point, and the sequence of
events is different than what I expect would occur.
>
>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.
>
>
Understood. I agree there is a bug here based on what you've described,
I'm just concerned that the failure is more subtle than you've uncovered
so far.
>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.
>
>
That HttpMethod is already too large an interface cuts strongly against
your proposed solution, for the very reason that the HttpMethodBase
class and HttpMethod interface should be in sync.
>As of yet, I haven't written any test cases for this problem.
>
Perhaps you can provide the stack trace of the exception failure, or a
more specific "trace" of the scenario, mentioning routine names (and
line numbers?) as I have attempted to do based on your description? You
might also consider including a "wire" level debug trace, and/or
snooping on the connection (see the mailing list archive for tips on how
you might do this). Maybe, you could augmenting SimpleHttpConnection so
that it can mimic the unexpected close for you, and make your case 100%
reproducible for others.
-Eric.
Re: [PATCH] Closing HttpConnection before retrying
Posted by Rob Di Marco <rd...@hmsonline.com>.
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" <er...@tibco.com>
To: "Commons HttpClient Project" <co...@jakarta.apache.org>
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:
> >>><ma...@jakarta.apache.org> For
> >>>additional commands, e-mail:
> >>><ma...@jakarta.apache.org>
> >>>
> >>>
> >
> >
> >
> >------------------------------------------------------------------------
> >
> >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:
<ma...@jakarta.apache.org>
> >For additional commands, e-mail:
<ma...@jakarta.apache.org>
> >
>
>
> --
> To unsubscribe, e-mail:
<ma...@jakarta.apache.org>
> For additional commands, e-mail:
<ma...@jakarta.apache.org>
>
Re: [PATCH] Closing HttpConnection before retrying
Posted by Eric Johnson <er...@tibco.com>.
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:
>>><ma...@jakarta.apache.org> For
>>>additional commands, e-mail:
>>><ma...@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: <ma...@jakarta.apache.org>
>For additional commands, e-mail: <ma...@jakarta.apache.org>
>
Re: [PATCH] Closing HttpConnection before retrying
Posted by Rob Di Marco <rd...@hmsonline.com>.
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:
> > <ma...@jakarta.apache.org> For
> > additional commands, e-mail:
> > <ma...@jakarta.apache.org>
--
Rob Di Marco
Re: [PATCH] Closing HttpConnection before retrying
Posted by Eric Johnson <er...@tibco.com>.
Rob,
Your patch makes me slightly nervous. Let me see if I can explain my
reasons cogently:
* 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?
* 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?
* 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?
* 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.
-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/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 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: <ma...@jakarta.apache.org>
>For additional commands, e-mail: <ma...@jakarta.apache.org>
>