On 6 October 2011 23:29, David Hosier <[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] > For additional commands, e-mail: [email protected] > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
