Thanks for the heads up. Done. https://issues.apache.org/jira/browse/HTTPCLIENT-1134
-- David Hosier On Thursday, October 6, 2011 at 3:42 PM, sebb wrote: > On 6 October 2011 23:29, David Hosier <[email protected] > (mailto:[email protected])> wrote: > > How does this look? Diff is attached. > > No attachment seen here; mailing list tends to drop them. > > Patches are best provided as attachments to a JIRA issue please. > > > > > On Thursday, October 6, 2011 at 3:13 PM, David Hosier wrote: > > > > > Ok, thanks. I'm taking a stab at a patch right now. > > > > > > -- David Hosier > > > > > > On Thursday, October 6, 2011 at 2:52 PM, Oleg Kalnichevski wrote: > > > > > > > On Thu, 2011-10-06 at 14:23 -0700, David Hosier wrote: > > > > > On Thursday, October 6, 2011 at 1:40 PM, Oleg Kalnichevski wrote: > > > > > > On Thu, 2011-10-06 at 13:21 -0700, David Hosier wrote: > > > > > > > Understood. The library does not support the use case of > > > > > > > obtaining the entity of a response via the recommended usage for > > > > > > > any response other than a 2xx. > > > > > > > > > > > > BasicResponseHandler does not, hence, unsurprisingly, the name > > > > > > Basic. > > > > > > However, there is _nothing_ that prevents you from writing a custom, > > > > > > better ResponseHandler which handles response entities differently. > > > > > > While the ResponseHandler interface is indeed the recommended way of > > > > > > handling responses, the BasicResponseHandler is just its very basic > > > > > > implementation which was never meant to be used for anything else > > > > > > but > > > > > > the most simplistic use cases. No one in their sane mind should > > > > > > _ever_ > > > > > > convert an HTTP response to a string in a productive application. > > > > > > > > > > > > > Additionally, the javadoc for BasicResponseHandler is incorrect. > > > > > > > > > > > > What exactly is incorrect? If you think javadocs are not clear > > > > > > enough or > > > > > > specific enough I'll happily apply a patch if you submit one. > > > > > I went and looked more closely, and the issue is that I was looking > > > > > at the class-level javadoc for BasicResponseHandler. The class-level > > > > > javadoc indicates that the response body is read before throwing the > > > > > exception on status codes >=300. However, the javadoc for the > > > > > handleResponse() method does not indicate that the response body is > > > > > read. The statement about reading the response body on >=300 really > > > > > only occurs when used with HttpClient, and it's that class that > > > > > actually does the reading. That's how I read the code at least. > > > > > > > > Fair enough. This may sound misleading if taken out of the usual context > > > > of always using ResponseHandler together with HttpClient. I'll tweak the > > > > javadocs tomorrow (unless you would like to submit a patch yourself) > > > > > > > > Oleg > > > > > > > > > > Oleg > > > > > > > > > > > > > So now that I understand better how things work, I can take > > > > > > > action accordingly. Thanks for the responses. > > > > > > > > > > > > > > -- David Hosier > > > > > > > > > > > > > > On Thursday, October 6, 2011 at 11:40 AM, Oleg Kalnichevski wrote: > > > > > > > > > > > > > > > On Thu, 2011-10-06 at 11:31 -0700, David Hosier wrote: > > > > > > > > > I'm using the DefaultHttpClient to make the call, yes. I want > > > > > > > > > to use DefaultHttpClient with the ResponseHandler the way I > > > > > > > > > am supposed to. However, the API does not give me the ability > > > > > > > > > to get a hold of the Entity if the status code is 404, > > > > > > > > > because it throws an Exception which does not contain the > > > > > > > > > entity value. I need the Entity value, even if the call > > > > > > > > > returns 404. As far as I can tell, I cannot get the > > > > > > > > > information I need from the API the way it is designed to be > > > > > > > > > used. Is that clearer? Is my assessment correct? > > > > > > > > > > > > > > > > Yes, it is intentional that the exception thrown does not > > > > > > > > contain a > > > > > > > > response body, because it would involve reading the entire body > > > > > > > > content > > > > > > > > into a memory buffer. > > > > > > > > > > > > > > > > Oleg > > > > > > > > > > > > > > > > > > > > > > > > > -- David Hosier > > > > > > > > > > > > > > > > > > On Thursday, October 6, 2011 at 11:29 AM, Oleg Kalnichevski > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > On Thu, 2011-10-06 at 10:47 -0700, David Hosier wrote: > > > > > > > > > > > I am using this to interface with some REST services. One > > > > > > > > > > > key to a good REST service is to never let something like > > > > > > > > > > > a 404 spit out the server's generic 404 HTML page in > > > > > > > > > > > response to a REST request. So my service instead returns > > > > > > > > > > > an entity with the 404 that says something like "Could > > > > > > > > > > > not find alert 12334". I should be able to show this > > > > > > > > > > > response entity. However, given the way the > > > > > > > > > > > ResponseHandler works with HttpClient, this is not > > > > > > > > > > > possible, because the entity is not part of the Exception > > > > > > > > > > > that is thrown when the ResponseHandler encounters a 404. > > > > > > > > > > > Without manually reading the entity after ResponseHandler > > > > > > > > > > > throws an Exception, I would only be able to show the > > > > > > > > > > > fields that are contained in the Exception. That means I > > > > > > > > > > > could only show the text 'Not Found', which is hardly > > > > > > > > > > > meaningful since the status code of 404 already tells me > > > > > > > > > > > that. > > > > > > > > > > > > > > > > > > > > You are using ResponseHandler to interface with some REST > > > > > > > > > > services > > > > > > > > > > without using DefaultHttpClient? I am sorry but it still > > > > > > > > > > makes no sense > > > > > > > > > > to me. You might as well handle responses from that service > > > > > > > > > > _any_ way > > > > > > > > > > you like without using a ResponseHandler. > > > > > > > > > > > > > > > > > > > > Oleg > > > > > > > > > > > > > > > > > > > > > -- David Hosier > > > > > > > > > > > > > > > > > > > > > > On Thursday, October 6, 2011 at 5:39 AM, Oleg > > > > > > > > > > > Kalnichevski wrote: > > > > > > > > > > > > > > > > > > > > > > > On Thu, 2011-10-06 at 02:59 -0700, David Hosier wrote: > > > > > > > > > > > > > Ok, I see what the difference is in this situation. I > > > > > > > > > > > > > am not passing the ResponseHandler to the execute() > > > > > > > > > > > > > method. I am actually calling handleResponse() on the > > > > > > > > > > > > > ResponseHandler manually. > > > > > > > > > > > > > > > > > > > > > > > > I honestly see no sense in doing so. ResponseHandler is > > > > > > > > > > > > pretty much > > > > > > > > > > > > useless without the resource management code in > > > > > > > > > > > > AbstractHttpClient. > > > > > > > > > > > > > > > > > > > > > > > > What is the reason you want to invoke #handleResponse > > > > > > > > > > > > manually? > > > > > > > > > > > > > > > > > > > > > > > > Oleg > > > > > > > > > > > > > > > > > > > > > > > > > The problem I have with the implementation is that I > > > > > > > > > > > > > return error messages on error conditions. With the > > > > > > > > > > > > > way this works, you can only get very basic > > > > > > > > > > > > > information from the HttpResponseException. For > > > > > > > > > > > > > example, on a 404, it looks like the Exception only > > > > > > > > > > > > > contains 404 and 'Not Found'. I am able to pluck out > > > > > > > > > > > > > the entity when invoking handleResponse() manually by > > > > > > > > > > > > > simply consuming the entity myself, but it's not > > > > > > > > > > > > > possible to get the entity if the ResponseHandler is > > > > > > > > > > > > > passed to execute() and the status is not 2xx. Am I > > > > > > > > > > > > > off base here or is my analysis correct? Would you > > > > > > > > > > > > > recommend that if I really need the entity on a > > > > > > > > > > > > > non-2xx response that I just keep manually consuming > > > > > > > > > > > > > the entity? I'm not sure it would make sense for your > > > > > > > > > > > > > library to attempt to consume the entity in > > > > > > > > > > > > > BasicResponseHandler and try to add it as an > > > > > > > > > > > > > other fi > > > > > > > > > > > > > eld to the HttpResponseException. The > > > > > > > > > > > > > AbstractHttpClient code you linked me to would have > > > > > > > > > > > > > to change if you did that. > > > > > > > > > > > > > > > > > > > > > > > > > > -- David Hosier > > > > > > > > > > > > > > > > > > > > > > > > > > On Thursday, October 6, 2011 at 2:30 AM, David Hosier > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thursday, October 6, 2011 at 2:22 AM, Oleg > > > > > > > > > > > > > > Kalnichevski wrote: > > > > > > > > > > > > > > > On Wed, 2011-10-05 at 13:44 -0700, David Hosier > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > Perhaps I'm wrong, but the code for > > > > > > > > > > > > > > > > BasicResponseHandler in httpclient 4.1.2 does > > > > > > > > > > > > > > > > not satisfy the javadocs as written. The > > > > > > > > > > > > > > > > javadoc states the following: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > "If the response code was >= 300, the response > > > > > > > > > > > > > > > > body is consumed and an HttpResponseException > > > > > > > > > > > > > > > > (http://hc.apache.org/httpcomponents-client-ga/httpclient/apidocs/org/apache/http/client/HttpResponseException.html) > > > > > > > > > > > > > > > > is thrown." > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > However, the code does not do that: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > StatusLine statusLine = > > > > > > > > > > > > > > > > response.getStatusLine(); > > > > > > > > > > > > > > > > if (statusLine.getStatusCode() >= 300) { > > > > > > > > > > > > > > > > throw new > > > > > > > > > > > > > > > > HttpResponseException(statusLine.getStatusCode(), > > > > > > > > > > > > > > > > statusLine.getReasonPhrase()); > > > > > > > > > > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > HttpEntity entity = response.getEntity(); > > > > > > > > > > > > > > > > return entity == null ? null : > > > > > > > > > > > > > > > > EntityUtils.toString(entity); > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The code clearly throws the Exception without > > > > > > > > > > > > > > > > reading the entity. So what happens is that if > > > > > > > > > > > > > > > > you get a non-2xx response, connections are > > > > > > > > > > > > > > > > never released as can be seen by enabling DEBUG > > > > > > > > > > > > > > > > logging for the library. Am I misreading the > > > > > > > > > > > > > > > > code or javadocs, or is this really broken? If > > > > > > > > > > > > > > > > I catch the Exception and then read the entity > > > > > > > > > > > > > > > > manually like shown above, I can see the > > > > > > > > > > > > > > > > connections being closed. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -David > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi David > > > > > > > > > > > > > > > The resource management is taken care of by > > > > > > > > > > > > > > > HttpClient [1]. I do not > > > > > > > > > > > > > > > think BasicResponseHandler is broken. The whole > > > > > > > > > > > > > > > point of ResponseHandler > > > > > > > > > > > > > > > is to free the user from having to worry about > > > > > > > > > > > > > > > resource management and > > > > > > > > > > > > > > > response entities. > > > > > > > > > > > > > > Interesting. Thanks for the link to the code. I can > > > > > > > > > > > > > > assure you that in my situation however, that the > > > > > > > > > > > > > > connections are not getting closed. I'll take a > > > > > > > > > > > > > > closer look at the code and compare it to this > > > > > > > > > > > > > > linked code to see if I'm using the right stuff. My > > > > > > > > > > > > > > assumption at this point then is that I'm just > > > > > > > > > > > > > > doing something wrong. Thanks. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Oleg > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > [1] > > > > > > > > > > > > > > > http://hc.apache.org/httpcomponents-client-ga/httpclient/xref/org/apache/http/impl/client/AbstractHttpClient.html#930 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --------------------------------------------------------------------- > > > > > > > > > > > > > > > > To unsubscribe, e-mail: > > > > > > > > > > > > > > > > [email protected] > > > > > > > > > > > > > > > > (mailto:[email protected]) > > > > > > > > > > > > > > > > For additional commands, e-mail: > > > > > > > > > > > > > > > > [email protected] > > > > > > > > > > > > > > > > (mailto:[email protected]) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --------------------------------------------------------------------- > > > > > > > > > > > > > > > To unsubscribe, e-mail: > > > > > > > > > > > > > > > [email protected] > > > > > > > > > > > > > > > (mailto:[email protected]) > > > > > > > > > > > > > > > For additional commands, e-mail: > > > > > > > > > > > > > > > [email protected] > > > > > > > > > > > > > > > (mailto:[email protected]) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --------------------------------------------------------------------- > > > > > > > > > > > > > > To unsubscribe, e-mail: > > > > > > > > > > > > > > [email protected] > > > > > > > > > > > > > > (mailto:[email protected]) > > > > > > > > > > > > > > For additional commands, e-mail: > > > > > > > > > > > > > > [email protected] > > > > > > > > > > > > > > (mailto:[email protected]) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --------------------------------------------------------------------- > > > > > > > > > > > > > To unsubscribe, e-mail: > > > > > > > > > > > > > [email protected] > > > > > > > > > > > > > (mailto:[email protected]) > > > > > > > > > > > > > For additional commands, e-mail: > > > > > > > > > > > > > [email protected] > > > > > > > > > > > > > (mailto:[email protected]) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --------------------------------------------------------------------- > > > > > > > > > > > > To unsubscribe, e-mail: > > > > > > > > > > > > [email protected] > > > > > > > > > > > > (mailto:[email protected]) > > > > > > > > > > > > For additional commands, e-mail: > > > > > > > > > > > > [email protected] > > > > > > > > > > > > (mailto:[email protected]) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --------------------------------------------------------------------- > > > > > > > > > > > To unsubscribe, e-mail: > > > > > > > > > > > [email protected] > > > > > > > > > > > (mailto:[email protected]) > > > > > > > > > > > For additional commands, e-mail: > > > > > > > > > > > [email protected] > > > > > > > > > > > (mailto:[email protected]) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --------------------------------------------------------------------- > > > > > > > > > > To unsubscribe, e-mail: > > > > > > > > > > [email protected] > > > > > > > > > > (mailto:[email protected]) > > > > > > > > > > For additional commands, e-mail: > > > > > > > > > > [email protected] > > > > > > > > > > (mailto:[email protected]) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --------------------------------------------------------------------- > > > > > > > > > To unsubscribe, e-mail: > > > > > > > > > [email protected] > > > > > > > > > (mailto:[email protected]) > > > > > > > > > For additional commands, e-mail: > > > > > > > > > [email protected] > > > > > > > > > (mailto:[email protected]) > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --------------------------------------------------------------------- > > > > > > > > To unsubscribe, e-mail: > > > > > > > > [email protected] > > > > > > > > (mailto:[email protected]) > > > > > > > > For additional commands, e-mail: > > > > > > > > [email protected] > > > > > > > > (mailto:[email protected]) > > > > > > > > > > > > > > > > > > > > > > > > > > > > --------------------------------------------------------------------- > > > > > > > To unsubscribe, e-mail: > > > > > > > [email protected] > > > > > > > (mailto:[email protected]) > > > > > > > For additional commands, e-mail: > > > > > > > [email protected] > > > > > > > (mailto:[email protected]) > > > > > > > > > > > > > > > > > > > > > > > > --------------------------------------------------------------------- > > > > > > To unsubscribe, e-mail: [email protected] > > > > > > (mailto:[email protected]) > > > > > > For additional commands, e-mail: > > > > > > [email protected] > > > > > > (mailto:[email protected]) > > > > > > > > > > > > > > > > > > > > --------------------------------------------------------------------- > > > > > To unsubscribe, e-mail: [email protected] > > > > > (mailto:[email protected]) > > > > > For additional commands, e-mail: [email protected] > > > > > (mailto:[email protected]) > > > > > > > > > > > > > > > > --------------------------------------------------------------------- > > > > To unsubscribe, e-mail: [email protected] > > > > (mailto:[email protected]) > > > > For additional commands, e-mail: [email protected] > > > > (mailto:[email protected]) > > > > > > > > > > > > --------------------------------------------------------------------- > > > To unsubscribe, e-mail: [email protected] > > > (mailto:[email protected]) > > > For additional commands, e-mail: [email protected] > > > (mailto:[email protected]) > > > > > > > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: [email protected] > > (mailto:[email protected]) > > For additional commands, e-mail: [email protected] > > (mailto:[email protected]) > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > (mailto:[email protected]) > For additional commands, e-mail: [email protected] > (mailto:[email protected]) --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
