You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by Oleg Kalnichevski <ol...@apache.org> on 2010/04/09 21:12:41 UTC
HttpEntity#consumeContent deprecated; was Re: HttpEntity#writeTo
and #consumeContent
...
> >
> > > I still don't understand what should be done if an IOException occurs
> > > (or RuntimeException) while in the read() method. Must consumeContent be
> > > called then? Or, in an exception enough to consider the stream released?
> > >
> >
> > When reading from an input stream, #close method should be called from a
> > finally clause to ensure proper resource release.
> >
>
> Sounds like you think/agree that writeTo implementations should close
> the inputstream in a finally block (since they read from it). I'll file
> a bug report and include a patch next week.
>
> James
>
James et al
I deprecated HttpEntity#consumeContent in favor of a more standard
InputStream#close contract. Javadocs / tutorial have been updated to
describe the recommended way to ensure resource deallocation:
http://svn.apache.org/viewvc?rev=932551&view=rev
Please review / comment / critique
Oleg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: HttpEntity#consumeContent deprecated; was Re:
HttpEntity#writeTo and #consumeContent
Posted by Oleg Kalnichevski <ol...@apache.org>.
On Wed, 2010-04-21 at 10:01 -0400, James Leigh wrote:
> On Sat, 2010-04-10 at 17:43 +0200, Oleg Kalnichevski wrote:
> > On Sat, 2010-04-10 at 11:12 -0400, James Leigh wrote:
> > > On Fri, 2010-04-09 at 21:12 +0200, Oleg Kalnichevski wrote:
> > > >
> > > > James et al
> > > >
> > > > I deprecated HttpEntity#consumeContent in favor of a more standard
> > > > InputStream#close contract. Javadocs / tutorial have been updated to
> > > > describe the recommended way to ensure resource deallocation:
> > > >
> > > > http://svn.apache.org/viewvc?rev=932551&view=rev
> > > >
> > > > Please review / comment / critique
> > > >
> > > > Oleg
> > > >
> > >
> > > Thanks for doing this Oleg. However, it is not clear from the revised
> > > javadocs if getContent().close() must be called if the content is not
> > > read.
> >
> > The trouble is this very much depends on the connection management
> > logic, which is out of scope for HttpCore. If one does not intent to
> > re-use the underlying connection is going to close it in any way, there
> > is no point reading the content.
> >
> > I intent to address this issue in the documentation for HttpClient once
> > HttpClient is upgraded to the next version of HttpCore.
>
> > >
> > > While getContent().close() works there are now many ways to clean-up the
> > > same resources and the javadocs need to be very clear. Perhaps more
> > > information in needed in HttpMessage and its sub-interfaces?
> > >
> >
> > I am not much of a writer. I always tend to be too terse with my
> > writing. Please do feel free to improve the javadocs and submit the
> > changes as a patch.
> >
> > Cheers
> >
> > Oleg
> >
>
> How about the following javadoc additions?
>
> For HttpMessage we could say:
>
> Some instances of this require resources to be manually closed. If this
> instance is also an instance of HttpEntityEnclosingRequest or
> HttpResponse and the result of getEntity() is non-null, it must be
> manually closed. See HttpEntity for how to close the resource.
>
> For HttpRequest we could say:
>
> Some instances of this require resources to be manually closed. If this
> instance is also an instance of HttpEntityEnclosingRequest and the
> result of getEntity() is non-null, it must be manually closed. See
> HttpEntity for how to close the resource.
>
> For HttpEntityEnclosingRequest we could say:
>
> Instances of this interface may require resources to be manually closed.
> If the result of getEntity() is non-null, it must be manually closed.
> See HttpEntity for how to close the resource.
>
> For HttpResponse we could say:
>
> Instances of this interface may require resources to be manually closed.
> If the result of getEntity() is non-null, it must be manually closed.
> See HttpEntity for how to close the resource.
>
> For HttpEntity we could say:
>
> Instances of this interface require resources to be manually closed. To
> free up any used resources every instance of HttpEntity must either call
> #writeTo or InputStream#close() on the resust of #getContent() or if
> this is also an instance of ProducingNHttpEntity or ConsumingNHttpEntity
> #finish()
>
> For ProducingNHttpEntity we could say:
>
> See HttpEntity on how to close this resource.
>
> For ConsumingNHttpEntity we could say:
>
> See HttpEntity on how to close this resource.
>
> James
>
James,
It would be much easier for me if could submit these changes as a patch
in udiff format against SVN trunk.
Evil Extortionist Oleg
PS:
I have also been thinking about using generics in HttpEntity and related
interfaces to make it more apparent that some entities are backed by a
dynamic stream of data that needs to be properly closed out. Something
along these lines:
interface HttpEntity<T> {
T getContent();
}
interface HttpResponse<T> {
HttpEntity<T> getContent();
}
HttpResponse<InputStream> response = httpclient.execute(...);
HttpEntity<InputStream> entity = response.getEntity();
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: HttpEntity#consumeContent deprecated; was Re:
HttpEntity#writeTo and #consumeContent
Posted by James Leigh <ja...@leighnet.ca>.
On Sat, 2010-04-10 at 17:43 +0200, Oleg Kalnichevski wrote:
> On Sat, 2010-04-10 at 11:12 -0400, James Leigh wrote:
> > On Fri, 2010-04-09 at 21:12 +0200, Oleg Kalnichevski wrote:
> > >
> > > James et al
> > >
> > > I deprecated HttpEntity#consumeContent in favor of a more standard
> > > InputStream#close contract. Javadocs / tutorial have been updated to
> > > describe the recommended way to ensure resource deallocation:
> > >
> > > http://svn.apache.org/viewvc?rev=932551&view=rev
> > >
> > > Please review / comment / critique
> > >
> > > Oleg
> > >
> >
> > Thanks for doing this Oleg. However, it is not clear from the revised
> > javadocs if getContent().close() must be called if the content is not
> > read.
>
> The trouble is this very much depends on the connection management
> logic, which is out of scope for HttpCore. If one does not intent to
> re-use the underlying connection is going to close it in any way, there
> is no point reading the content.
>
> I intent to address this issue in the documentation for HttpClient once
> HttpClient is upgraded to the next version of HttpCore.
> >
> > While getContent().close() works there are now many ways to clean-up the
> > same resources and the javadocs need to be very clear. Perhaps more
> > information in needed in HttpMessage and its sub-interfaces?
> >
>
> I am not much of a writer. I always tend to be too terse with my
> writing. Please do feel free to improve the javadocs and submit the
> changes as a patch.
>
> Cheers
>
> Oleg
>
How about the following javadoc additions?
For HttpMessage we could say:
Some instances of this require resources to be manually closed. If this
instance is also an instance of HttpEntityEnclosingRequest or
HttpResponse and the result of getEntity() is non-null, it must be
manually closed. See HttpEntity for how to close the resource.
For HttpRequest we could say:
Some instances of this require resources to be manually closed. If this
instance is also an instance of HttpEntityEnclosingRequest and the
result of getEntity() is non-null, it must be manually closed. See
HttpEntity for how to close the resource.
For HttpEntityEnclosingRequest we could say:
Instances of this interface may require resources to be manually closed.
If the result of getEntity() is non-null, it must be manually closed.
See HttpEntity for how to close the resource.
For HttpResponse we could say:
Instances of this interface may require resources to be manually closed.
If the result of getEntity() is non-null, it must be manually closed.
See HttpEntity for how to close the resource.
For HttpEntity we could say:
Instances of this interface require resources to be manually closed. To
free up any used resources every instance of HttpEntity must either call
#writeTo or InputStream#close() on the resust of #getContent() or if
this is also an instance of ProducingNHttpEntity or ConsumingNHttpEntity
#finish()
For ProducingNHttpEntity we could say:
See HttpEntity on how to close this resource.
For ConsumingNHttpEntity we could say:
See HttpEntity on how to close this resource.
James
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: HttpEntity#consumeContent deprecated; was Re:
HttpEntity#writeTo and #consumeContent
Posted by Oleg Kalnichevski <ol...@apache.org>.
On Mon, 2010-04-12 at 09:04 -0400, James Leigh wrote:
> On Sat, 2010-04-10 at 17:43 +0200, Oleg Kalnichevski wrote:
> > On Sat, 2010-04-10 at 11:12 -0400, James Leigh wrote:
> > >
> > > Thanks for doing this Oleg. However, it is not clear from the revised
> > > javadocs if getContent().close() must be called if the content is not
> > > read.
> >
> > The trouble is this very much depends on the connection management
> > logic, which is out of scope for HttpCore. If one does not intent to
> > re-use the underlying connection is going to close it in any way, there
> > is no point reading the content.
> >
> > I intent to address this issue in the documentation for HttpClient once
> > HttpClient is upgraded to the next version of HttpCore.
> >
> > > There needs to be a clear way to free the resources of the
> > > HttpMessage even if the caller does not care to process the HTTP message
> > > body.
> > >
> > > I also want to point out another use-case that is relevant here. Often
> > > the HttpResponse is dependent on external resources, such as a database
> > > connections or a read locks, and these need to be closed when the
> > > response message body is consumed.
> >
> > I am not entirely sure this is a good idea to make HttpResponse
> > responsible for cleanup of such resources. Unless I am missing
> > something, it just does not sound right to me.
> >
> >
> > > By using InputStream#close() as the
> > > signal to free up resources the implementers are required to wrap the
> > > underlying InputSteam to intercept this call and release locks and/or
> > > free db connections. This prevents some read optimizations to occur.
> > >
> > > In the case of a FileInputStream, for example, the OS can bypass
> > > in-memory buffers using FileChannel#transferTo, but a FileInputStream
> > > wound not be detectable if the InputStream was wrapped. However, by
> > > using the ProducingNHttpEntity interface the finish() method allows
> > > implementers to override the clean-up method without interfering with
> > > read optimizations, such as with NFileEntity.
> > >
> > > While getContent().close() works there are now many ways to clean-up the
> > > same resources and the javadocs need to be very clear. Perhaps more
> > > information in needed in HttpMessage and its sub-interfaces?
> > >
> >
> > I am not much of a writer. I always tend to be too terse with my
> > writing. Please do feel free to improve the javadocs and submit the
> > changes as a patch.
> >
> > Cheers
> >
> > Oleg
> >
> >
>
> If it was up to me, I would make HttpMessage extend java.io.Closable say
> the following in all subclass javadoc headers.
>
> #close() must be called when the message is no longer of used to
> free up any resources used by this message.
>
> Would this be possible in a future release?
>
> James
>
That is possible, but would have to wait until 5.0. However, personally,
I would rather keep all resource management at the HTTP entity level.
Oleg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: HttpEntity#consumeContent deprecated; was Re:
HttpEntity#writeTo and #consumeContent
Posted by James Leigh <ja...@leighnet.ca>.
On Sat, 2010-04-10 at 17:43 +0200, Oleg Kalnichevski wrote:
> On Sat, 2010-04-10 at 11:12 -0400, James Leigh wrote:
> >
> > Thanks for doing this Oleg. However, it is not clear from the revised
> > javadocs if getContent().close() must be called if the content is not
> > read.
>
> The trouble is this very much depends on the connection management
> logic, which is out of scope for HttpCore. If one does not intent to
> re-use the underlying connection is going to close it in any way, there
> is no point reading the content.
>
> I intent to address this issue in the documentation for HttpClient once
> HttpClient is upgraded to the next version of HttpCore.
>
> > There needs to be a clear way to free the resources of the
> > HttpMessage even if the caller does not care to process the HTTP message
> > body.
> >
> > I also want to point out another use-case that is relevant here. Often
> > the HttpResponse is dependent on external resources, such as a database
> > connections or a read locks, and these need to be closed when the
> > response message body is consumed.
>
> I am not entirely sure this is a good idea to make HttpResponse
> responsible for cleanup of such resources. Unless I am missing
> something, it just does not sound right to me.
>
>
> > By using InputStream#close() as the
> > signal to free up resources the implementers are required to wrap the
> > underlying InputSteam to intercept this call and release locks and/or
> > free db connections. This prevents some read optimizations to occur.
> >
> > In the case of a FileInputStream, for example, the OS can bypass
> > in-memory buffers using FileChannel#transferTo, but a FileInputStream
> > wound not be detectable if the InputStream was wrapped. However, by
> > using the ProducingNHttpEntity interface the finish() method allows
> > implementers to override the clean-up method without interfering with
> > read optimizations, such as with NFileEntity.
> >
> > While getContent().close() works there are now many ways to clean-up the
> > same resources and the javadocs need to be very clear. Perhaps more
> > information in needed in HttpMessage and its sub-interfaces?
> >
>
> I am not much of a writer. I always tend to be too terse with my
> writing. Please do feel free to improve the javadocs and submit the
> changes as a patch.
>
> Cheers
>
> Oleg
>
>
If it was up to me, I would make HttpMessage extend java.io.Closable say
the following in all subclass javadoc headers.
#close() must be called when the message is no longer of used to
free up any resources used by this message.
Would this be possible in a future release?
James
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: HttpEntity#consumeContent deprecated; was Re:
HttpEntity#writeTo and #consumeContent
Posted by Oleg Kalnichevski <ol...@apache.org>.
On Sat, 2010-04-10 at 11:12 -0400, James Leigh wrote:
> On Fri, 2010-04-09 at 21:12 +0200, Oleg Kalnichevski wrote:
> > ...
> >
> > > >
> > > > > I still don't understand what should be done if an IOException occurs
> > > > > (or RuntimeException) while in the read() method. Must consumeContent be
> > > > > called then? Or, in an exception enough to consider the stream released?
> > > > >
> > > >
> > > > When reading from an input stream, #close method should be called from a
> > > > finally clause to ensure proper resource release.
> > > >
> > >
> > > Sounds like you think/agree that writeTo implementations should close
> > > the inputstream in a finally block (since they read from it). I'll file
> > > a bug report and include a patch next week.
> > >
> > > James
> > >
> >
> > James et al
> >
> > I deprecated HttpEntity#consumeContent in favor of a more standard
> > InputStream#close contract. Javadocs / tutorial have been updated to
> > describe the recommended way to ensure resource deallocation:
> >
> > http://svn.apache.org/viewvc?rev=932551&view=rev
> >
> > Please review / comment / critique
> >
> > Oleg
> >
>
> Thanks for doing this Oleg. However, it is not clear from the revised
> javadocs if getContent().close() must be called if the content is not
> read.
The trouble is this very much depends on the connection management
logic, which is out of scope for HttpCore. If one does not intent to
re-use the underlying connection is going to close it in any way, there
is no point reading the content.
I intent to address this issue in the documentation for HttpClient once
HttpClient is upgraded to the next version of HttpCore.
> There needs to be a clear way to free the resources of the
> HttpMessage even if the caller does not care to process the HTTP message
> body.
>
> I also want to point out another use-case that is relevant here. Often
> the HttpResponse is dependent on external resources, such as a database
> connections or a read locks, and these need to be closed when the
> response message body is consumed.
I am not entirely sure this is a good idea to make HttpResponse
responsible for cleanup of such resources. Unless I am missing
something, it just does not sound right to me.
> By using InputStream#close() as the
> signal to free up resources the implementers are required to wrap the
> underlying InputSteam to intercept this call and release locks and/or
> free db connections. This prevents some read optimizations to occur.
>
> In the case of a FileInputStream, for example, the OS can bypass
> in-memory buffers using FileChannel#transferTo, but a FileInputStream
> wound not be detectable if the InputStream was wrapped. However, by
> using the ProducingNHttpEntity interface the finish() method allows
> implementers to override the clean-up method without interfering with
> read optimizations, such as with NFileEntity.
>
> While getContent().close() works there are now many ways to clean-up the
> same resources and the javadocs need to be very clear. Perhaps more
> information in needed in HttpMessage and its sub-interfaces?
>
I am not much of a writer. I always tend to be too terse with my
writing. Please do feel free to improve the javadocs and submit the
changes as a patch.
Cheers
Oleg
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: HttpEntity#consumeContent deprecated; was Re:
HttpEntity#writeTo and #consumeContent
Posted by James Leigh <ja...@leighnet.ca>.
On Fri, 2010-04-09 at 21:12 +0200, Oleg Kalnichevski wrote:
> ...
>
> > >
> > > > I still don't understand what should be done if an IOException occurs
> > > > (or RuntimeException) while in the read() method. Must consumeContent be
> > > > called then? Or, in an exception enough to consider the stream released?
> > > >
> > >
> > > When reading from an input stream, #close method should be called from a
> > > finally clause to ensure proper resource release.
> > >
> >
> > Sounds like you think/agree that writeTo implementations should close
> > the inputstream in a finally block (since they read from it). I'll file
> > a bug report and include a patch next week.
> >
> > James
> >
>
> James et al
>
> I deprecated HttpEntity#consumeContent in favor of a more standard
> InputStream#close contract. Javadocs / tutorial have been updated to
> describe the recommended way to ensure resource deallocation:
>
> http://svn.apache.org/viewvc?rev=932551&view=rev
>
> Please review / comment / critique
>
> Oleg
>
Thanks for doing this Oleg. However, it is not clear from the revised
javadocs if getContent().close() must be called if the content is not
read. There needs to be a clear way to free the resources of the
HttpMessage even if the caller does not care to process the HTTP message
body.
I also want to point out another use-case that is relevant here. Often
the HttpResponse is dependent on external resources, such as a database
connections or a read locks, and these need to be closed when the
response message body is consumed. By using InputStream#close() as the
signal to free up resources the implementers are required to wrap the
underlying InputSteam to intercept this call and release locks and/or
free db connections. This prevents some read optimizations to occur.
In the case of a FileInputStream, for example, the OS can bypass
in-memory buffers using FileChannel#transferTo, but a FileInputStream
wound not be detectable if the InputStream was wrapped. However, by
using the ProducingNHttpEntity interface the finish() method allows
implementers to override the clean-up method without interfering with
read optimizations, such as with NFileEntity.
While getContent().close() works there are now many ways to clean-up the
same resources and the javadocs need to be very clear. Perhaps more
information in needed in HttpMessage and its sub-interfaces?
James
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org