[compress] close() and writing invalid streams

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[compress] close() and writing invalid streams

Stefan Bodewig
Hi all

https://issues.apache.org/jira/browse/COMPRESS-457 raises an issue that
I vaguely recall we've talked about in the past but I may be wrong.

Almost all our OutputStream close methods go along the lines of

public void close() throws IOException {
    finishFormatSpecificStuff();
    closeUnderlyingStreamsOrChannelsAndOtherResources();
}

as some formats need to write trailers in order to create valid
output. If for some reason finishFormatSpecificStuff() stuff fails the
underlying stream will not be closed leading to a resource leak - unless
you keep track of the underlying stream and close it yourself
(try-with-resources probably).

Do we want to do anything about this and if so what?

We could modify all close implementations to perform resource cleanup in
a finally block. Likewise we could add new (let's say unsafeClose())
methods that only perform resource cleanup. Or we could properly
document the behavior and ensure our examples contain resource cleanup
code.

I'm leaning toward "documenting" but want to ensure there are no leaks
the user cannot prevent. For example DeflateCompressorOutputStream
"end()"s the Deflater instance in a finally block while zip's
StreamCompressor holds a reference to a Deflater that may never get
closed.

Stefan

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [compress] close() and writing invalid streams

garydgregory
I'm for making it obvious and keeping clean ups localized, meaning add
try-with-resources blocks to all close() methods that need them.

Gary

On Thu, Jun 28, 2018 at 3:54 AM Stefan Bodewig <[hidden email]> wrote:

> Hi all
>
> https://issues.apache.org/jira/browse/COMPRESS-457 raises an issue that
> I vaguely recall we've talked about in the past but I may be wrong.
>
> Almost all our OutputStream close methods go along the lines of
>
> public void close() throws IOException {
>     finishFormatSpecificStuff();
>     closeUnderlyingStreamsOrChannelsAndOtherResources();
> }
>
> as some formats need to write trailers in order to create valid
> output. If for some reason finishFormatSpecificStuff() stuff fails the
> underlying stream will not be closed leading to a resource leak - unless
> you keep track of the underlying stream and close it yourself
> (try-with-resources probably).
>
> Do we want to do anything about this and if so what?
>
> We could modify all close implementations to perform resource cleanup in
> a finally block. Likewise we could add new (let's say unsafeClose())
> methods that only perform resource cleanup. Or we could properly
> document the behavior and ensure our examples contain resource cleanup
> code.
>
> I'm leaning toward "documenting" but want to ensure there are no leaks
> the user cannot prevent. For example DeflateCompressorOutputStream
> "end()"s the Deflater instance in a finally block while zip's
> StreamCompressor holds a reference to a Deflater that may never get
> closed.
>
> Stefan
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [compress] close() and writing invalid streams

Stefan Bodewig
Hi Gary

I'm afraid I don't really follow you here. close() doesn't open any
resources so I'm not sure what try-with-resources can do here.

Stefan

On 2018-06-28, Gary Gregory wrote:

> I'm for making it obvious and keeping clean ups localized, meaning add
> try-with-resources blocks to all close() methods that need them.


> On Thu, Jun 28, 2018 at 3:54 AM Stefan Bodewig <[hidden email]> wrote:

>> Hi all

>> https://issues.apache.org/jira/browse/COMPRESS-457 raises an issue that
>> I vaguely recall we've talked about in the past but I may be wrong.

>> Almost all our OutputStream close methods go along the lines of

>> public void close() throws IOException {
>>     finishFormatSpecificStuff();
>>     closeUnderlyingStreamsOrChannelsAndOtherResources();


>> as some formats need to write trailers in order to create valid
>> output. If for some reason finishFormatSpecificStuff() stuff fails the
>> underlying stream will not be closed leading to a resource leak - unless
>> you keep track of the underlying stream and close it yourself
>> (try-with-resources probably).

>> Do we want to do anything about this and if so what?

>> We could modify all close implementations to perform resource cleanup in
>> a finally block. Likewise we could add new (let's say unsafeClose())
>> methods that only perform resource cleanup. Or we could properly
>> document the behavior and ensure our examples contain resource cleanup
>> code.

>> I'm leaning toward "documenting" but want to ensure there are no leaks
>> the user cannot prevent. For example DeflateCompressorOutputStream
>> "end()"s the Deflater instance in a finally block while zip's
>> StreamCompressor holds a reference to a Deflater that may never get
>> closed.

>> Stefan

>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [compress] close() and writing invalid streams

garydgregory
I am saying that I prefer the option:

public void close() throws IOException {
     try {
         finishFormatSpecificStuff();
     } finally {
         closeUnderlyingStreamsOrChannelsAndOtherResources();
     }
}

Gary

On Fri, Jun 29, 2018 at 10:45 AM Stefan Bodewig <[hidden email]> wrote:

> Hi Gary
>
> I'm afraid I don't really follow you here. close() doesn't open any
> resources so I'm not sure what try-with-resources can do here.
>
> Stefan
>
> On 2018-06-28, Gary Gregory wrote:
>
> > I'm for making it obvious and keeping clean ups localized, meaning add
> > try-with-resources blocks to all close() methods that need them.
>
>
> > On Thu, Jun 28, 2018 at 3:54 AM Stefan Bodewig <[hidden email]>
> wrote:
>
> >> Hi all
>
> >> https://issues.apache.org/jira/browse/COMPRESS-457 raises an issue that
> >> I vaguely recall we've talked about in the past but I may be wrong.
>
> >> Almost all our OutputStream close methods go along the lines of
>
> >> public void close() throws IOException {
> >>     finishFormatSpecificStuff();
> >>     closeUnderlyingStreamsOrChannelsAndOtherResources();
>
>
> >> as some formats need to write trailers in order to create valid
> >> output. If for some reason finishFormatSpecificStuff() stuff fails the
> >> underlying stream will not be closed leading to a resource leak - unless
> >> you keep track of the underlying stream and close it yourself
> >> (try-with-resources probably).
>
> >> Do we want to do anything about this and if so what?
>
> >> We could modify all close implementations to perform resource cleanup in
> >> a finally block. Likewise we could add new (let's say unsafeClose())
> >> methods that only perform resource cleanup. Or we could properly
> >> document the behavior and ensure our examples contain resource cleanup
> >> code.
>
> >> I'm leaning toward "documenting" but want to ensure there are no leaks
> >> the user cannot prevent. For example DeflateCompressorOutputStream
> >> "end()"s the Deflater instance in a finally block while zip's
> >> StreamCompressor holds a reference to a Deflater that may never get
> >> closed.
>
> >> Stefan
>
> >> ---------------------------------------------------------------------
> >> To unsubscribe, e-mail: [hidden email]
> >> For additional commands, e-mail: [hidden email]
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>