On Thu, Sep 03, 2009 at 03:04:33PM -0700, Gerald Turner wrote:
> Oleg Kalnichevski <[email protected]> writes:
>
> > Gerald Turner wrote:
> >> ??? Releasing connections has increased the amount of code (finally
> >> block logic) that client applications need:
> >>
> >> HttpClient 3.x:
> >>
> >> HttpMethod method = null;
> >> try {
> >> ???
> >> }
> >> finally {
> >> if (method != null) {
> >> try {
> >> method.releaseConnection()
> >> }
> >> catch (Exception e) {
> >> log.warn("Failed to release connection: "
> >> + e.getMessage(), e);
> >> }
> >> }
> >> }
> >>
> >> HttpClient 4.0:
> >>
> >> HttpUriRequest request = null;
> >> HttpResponse response = null;
> >> try {
> >> ???
> >> }
> >> finally {
> >> boolean released = false;
> >>
> >> // Try consumeContent first, so that connection may be
> >> // kept-alive and reused. Note there is no response entity
> >> // for HTTP codes like 304
> >> if (response != null) {
> >> try {
> >> HttpEntity entity = response.getEntity();
> >> if (entity != null) {
> >> entity.consumeContent();
> >> released = true;
> >> }
> >> }
> >> catch (Exception e) {
> >> log.warn("Failed to consume HttpEntity: "
> >> + e.getMessage(), e);
> >> }
> >> }
> >>
> >> // Otherwise abort the connection, seems allow the
> >> // connection to be kept-alive and reused in some cases
> >> // (like when there was no response entity), this is a good
> >> // thing even if ???abort??? sounds like it's going to close.
> >> if (!released && request != null) {
> >> try {
> >> request.abort();
> >> }
> >> catch (Exception e) {
> >> log.warn("Failed to abort HttpUriRequest: "
> >> + e.getMessage(), e);
> >> }
> >> }
> >> }
> >> }
> >>
> >
> > Pretty much all this code is not necessary. (1) HttpClient will
> > automatically release the underlying connection as long as the entity
> > content is consumed to the end of stream; (2) HttpClient will
> > automatically release the underlying connection on any I/O exception
> > thrown while reading the response content. No special processing is
> > required in such as case.
> >
>
> Ah!, I should've mentioned that much of the code I've been working on
> fires off GETs/POSTs (even a Range-conditioned GET in one place) and
> doesn't bother reading the response content, it's happy with a 200 (or
> 206).
>
I do not think this makes any difference. If there is no response entity there
is no connection associated with that response that has to be released back to
the pool.
> Some of the code I've been working on expects 304's (If-Modified-Since)
> and 404's, there is response content (like a user-friendly File Not
> Found HTML page), but again it only cares about the status code.
>
> Anyway the heavy finally block is safe correct? For instance a 304 has
> no content, but I fall thru can call request.abort() even though it's
> already been recycled.
>
The finally block is okay but it is just plain ugly and the request.abort() bit
in it is completely useless, because there will never a connection to abort in
the first place.
> > In fact, this is perfectly sufficient to ensure proper release of
> > resources:
> >
> > HttpResponse rsp = httpclient.execute(target, req);
> > HttpEntity entity = rsp.getEntity();
> > if (entity != null) {
> > InputStream instream = entity.getContent();
> > try {
> > // process content
> > } finally {
> > instream.close();
> > // entity.consumeContent() would also do
> > }
> > }
> >
> > That is it.
> >
> >>
> >> ??? In addition to this complex finally-block, I have created a class
> >> which extends HttpEntity (CompressedEntity, does gzip
> >> Transfer-Encoding of requests), it's assigned to requests, it needs
> >> to release resources, HttpService.handleRequest does call
> >> consumeContent, however I am concerned with HTTP request executions
> >> that don't make it that far (connection manager timeout for
> >> instance), in that case my finally block has to call
> >> request.getEntity().consumeContent().
> >>
> >
> > See above. In case of an I/O error the connection will get
> > automatically closed and released back to the manager.
> >
>
> I'm not so sure about this - it's a request entity that begins life with
> some resource that needs to be cleaned. If HttpClient doesn't get as
> far as obtaining a connection from it's pool, wouldn't the request *not*
> have it's consumeContent called?
>
I see now. If a request entity allocates some resources at the _construction_
time, HttpClient will make no attempt to release them in case of an abnormal
termination of the request. The caller will have to ensure resources get
cleaned up.
> >> ??? Multi-part requests in HttpClient 3.x were a little easier to work
> >> with in a few special cases, the Part interface included the ???name???
> >> field, whereas the ???name??? field is now a parameter in the
> >> MultipartEntity.addPart(String,BodyContent) call.
> >>
> >> Before:
> >>
> >> Part[] parts = {
> >> new StringPart("Some-Field", "Some-Value"),
> >> new FilePart("Some-File", f)
> >> };
> >>
> >> MultipartRequestEntity entity =
> >> new MultipartRequestEntity(parts, method.getParams());
> >>
> >> After:
> >>
> >> LinkedHashMap<String,ContentBody> parts =
> >> new LinkedHashMap<String,ContentBody>();
> >> parts.put("Some-Field", new StringBody("Some-Value"));
> >> parts.put("Some-File", new FileBody(f));
> >>
> >> MultipartEntity entity = new MultipartEntity();
> >> for (Entry<String,ContentBody> part : parts)
> >> entity.addPart(part.getKey(), part.getValue());
> >>
> >> In some respects I like the API better, but I do have some code which
> >> needs to assemble the parts (including names) independent of the HTTP
> >> request execution, so in one application I wrote a Part wrapper:
> >>
> >> for (Part part : parts)
> >> entity.addPart(part.getName(), part.getContent());
> >>
> >
> > If you can think of a fix for the issue, raise a JIRA.
> >
>
> FormBodyPart does the trick (couples name and content).
>
> MultipartEntity.addPart looks like this:
>
> public void addPart(final String name, final ContentBody contentBody) {
> this.multipart.addBodyPart(new FormBodyPart(name, contentBody));
> this.dirty = true;
> }
>
> How about an addPart override that looks like:
>
> public void addPart(final BodyPart bodyPart) {
> this.multipart.addBodyPart(bodyPart);
> this.dirty = true;
> }
>
Raise a JIRA.
> > ConnPerRouteBean was a bug mistake on my part. I am thinking about
> > deprecating the damn thing in 4.1.
> >
>
> That's too bad, although it does seem useful to configure varying pool
> sizes by hostname.
>
> What do you think about the nested introspection thing?
>
> It could be done by having the bean classes reference each other, like
> ClientParamBean.getConnManagerParam (returns ConnManagerParamBean), but
> there is also the problem of complex types ??? maybe make a policy for the
> bean classes to deal with int/long/boolean/String types only (and
> getters for nested beans).
>
I have been thinking about using good ol' setters and getters on
the ThreadSafeClientConnManager class
> >> ??? The method DateUtils#formatDate(Date) is very handy and I use it in
> >> a lot of applications, nothing wrong here except that the package
> >> ??????.impl.cookie??? throws me off ??? having me rely on any external API
> >> implementation (???impl???) doesn't feel like a smart thing for me to do,
> >> also I don't use cookies at all, I use this method for handling
> >> Last-Modified and If-Modified-Since kinds of headers.
> >>
> >
> > We could deprecate DateUtils class in the impl package and move it to
> > another package within the 'public' API space.
> >
>
> I vote yes ???
>
If you feel very strongly about it, raise a JIRA.
Cheers
Oleg
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
