Re: [compress] close() and archives/streams in an invalid state
On 2011-08-17, Honton, Charles wrote: > Why guard against closing System.in? If the client application needs that > sort of functionality, it should wrap the incoming stream in a proxy that > doesn't delegate the close method to the underlying stream. You and I agree here - there even has been a thread about this not too long ago. Changing it would break backwards compatibility, though, something we are willing to do in a 2.x release not too far away in the future (I hope). For 1.x we prefer to keep it as it is. > (A new class for commons-io?) I'm pretty sure that already exists. CloseShieldInputStream IIRC. Stefan - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [compress] close() and archives/streams in an invalid state
Why guard against closing System.in? If the client application needs that sort of functionality, it should wrap the incoming stream in a proxy that doesn't delegate the close method to the underlying stream. (A new class for commons-io?) Chas Honton On 8/16/11 11:16 PM, "sebb" wrote: >On 17 August 2011 03:42, Stefan Bodewig wrote: >> On 2011-08-15, sebb wrote: >> >>> On 15 August 2011 09:56, Stefan Bodewig wrote: Hi, >> while working on the Zip64 stuff I deliberately created some invalid archives to test I get the expected exceptions. While doing so I realized I couldn't close the underlying stream because ZipArchiveOutputStream's close would throw an exception as finish() failed before ever closing the wrapped stream. The calling code may not even have a handle to the underlying stream if it used the File-arg constructor and so in the end a broken archive leaks the file descriptor. >> Then I went looking through the other implementations and found we are consistent here as far as archivers and bzip2 go. All of them will not close the underlying stream if the archive/stream is invalid. I'm not sure what gzip does as it just delegates to close in the Java classlib's GZipOutputStream. >> I wonder whether we should rather change the behavior to always close the underlying stream or whether we should add a new method - I called in destroy in ZipArchiveOutputStream - that does so. >> Or maybe we just document it for 1.3, add destroy as a non-common method where needed (like in ZIP, in all other cases the user code should have access to the underlying stream) - and revisit this in the 2.x timeframe, even though I can't see what could be broken by always closing the stream in a finally block. >> Does anybody see a good reason why the close behavior should stay the way it is right now? >> >>> For output, it seems sensible to close the underlying stream on >>> failure, as the content is likely to be garbage. >> >> Yes, this is the case I was talking about. >> >>> For input, there might be a use case for leaving the stream open, in >>> case some kind of recovery is possible. >> >> All our implementations - except for the new dump code, that doesn't >> properly handle close ATM - try to close the input stream regardless of >> whether there has been a problem with the archive or not (they don't >> even check). This is what I'd expect it to do as well. >> >> Some of the guard against closing System.in, but not all. >> >>> It would be useful to have a way of determining the input file pointer >>> at the point of failure. >> >> stream.getBytesRead() or are you thinking about anything else? > >That would be fine. > >> Stefan >> >> - >> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >> For additional commands, e-mail: dev-h...@commons.apache.org >> >> > >- >To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org >For additional commands, e-mail: dev-h...@commons.apache.org > - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [compress] close() and archives/streams in an invalid state
On 17 August 2011 06:18, Stefan Bodewig wrote: > On 2011-08-17, sebb wrote: > >> On 17 August 2011 03:42, Stefan Bodewig wrote: >>> On 2011-08-15, sebb wrote: > For input, there might be a use case for leaving the stream open, in case some kind of recovery is possible. > It would be useful to have a way of determining the input file pointer at the point of failure. > >>> stream.getBytesRead() or are you thinking about anything else? > >> That would be fine. > > I'm not sure whether we need to do anything for that. The streams > already count the bytes and you can access it when an exception occurs. > We might want to revisit the exception messages whether they can include > some sort of byte count. > > For "normal users" of compress I don't expect that information to be too > useful, though. Most users won't be familiar with the details of the > format. It may be useful for us during debugging or when bugs get > reported. Yes, that was my main concern. > Stefan > > - > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > > - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [compress] close() and archives/streams in an invalid state
On 2011-08-17, sebb wrote: > On 17 August 2011 03:42, Stefan Bodewig wrote: >> On 2011-08-15, sebb wrote: >>> For input, there might be a use case for leaving the stream open, in >>> case some kind of recovery is possible. >>> It would be useful to have a way of determining the input file pointer >>> at the point of failure. >> stream.getBytesRead() or are you thinking about anything else? > That would be fine. I'm not sure whether we need to do anything for that. The streams already count the bytes and you can access it when an exception occurs. We might want to revisit the exception messages whether they can include some sort of byte count. For "normal users" of compress I don't expect that information to be too useful, though. Most users won't be familiar with the details of the format. It may be useful for us during debugging or when bugs get reported. Stefan - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [compress] close() and archives/streams in an invalid state
On 17 August 2011 03:42, Stefan Bodewig wrote: > On 2011-08-15, sebb wrote: > >> On 15 August 2011 09:56, Stefan Bodewig wrote: >>> Hi, > >>> while working on the Zip64 stuff I deliberately created some invalid >>> archives to test I get the expected exceptions. While doing so I >>> realized I couldn't close the underlying stream because >>> ZipArchiveOutputStream's close would throw an exception as finish() >>> failed before ever closing the wrapped stream. The calling code may not >>> even have a handle to the underlying stream if it used the File-arg >>> constructor and so in the end a broken archive leaks the file >>> descriptor. > >>> Then I went looking through the other implementations and found we are >>> consistent here as far as archivers and bzip2 go. All of them will not >>> close the underlying stream if the archive/stream is invalid. I'm not >>> sure what gzip does as it just delegates to close in the Java classlib's >>> GZipOutputStream. > >>> I wonder whether we should rather change the behavior to always close >>> the underlying stream or whether we should add a new method - I called >>> in destroy in ZipArchiveOutputStream - that does so. > >>> Or maybe we just document it for 1.3, add destroy as a non-common method >>> where needed (like in ZIP, in all other cases the user code should have >>> access to the underlying stream) - and revisit this in the 2.x >>> timeframe, even though I can't see what could be broken by always >>> closing the stream in a finally block. > >>> Does anybody see a good reason why the close behavior should stay the >>> way it is right now? > >> For output, it seems sensible to close the underlying stream on >> failure, as the content is likely to be garbage. > > Yes, this is the case I was talking about. > >> For input, there might be a use case for leaving the stream open, in >> case some kind of recovery is possible. > > All our implementations - except for the new dump code, that doesn't > properly handle close ATM - try to close the input stream regardless of > whether there has been a problem with the archive or not (they don't > even check). This is what I'd expect it to do as well. > > Some of the guard against closing System.in, but not all. > >> It would be useful to have a way of determining the input file pointer >> at the point of failure. > > stream.getBytesRead() or are you thinking about anything else? That would be fine. > Stefan > > - > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > > - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [compress] close() and archives/streams in an invalid state
On 2011-08-15, sebb wrote: > On 15 August 2011 09:56, Stefan Bodewig wrote: >> Hi, >> while working on the Zip64 stuff I deliberately created some invalid >> archives to test I get the expected exceptions. While doing so I >> realized I couldn't close the underlying stream because >> ZipArchiveOutputStream's close would throw an exception as finish() >> failed before ever closing the wrapped stream. The calling code may not >> even have a handle to the underlying stream if it used the File-arg >> constructor and so in the end a broken archive leaks the file >> descriptor. >> Then I went looking through the other implementations and found we are >> consistent here as far as archivers and bzip2 go. All of them will not >> close the underlying stream if the archive/stream is invalid. I'm not >> sure what gzip does as it just delegates to close in the Java classlib's >> GZipOutputStream. >> I wonder whether we should rather change the behavior to always close >> the underlying stream or whether we should add a new method - I called >> in destroy in ZipArchiveOutputStream - that does so. >> Or maybe we just document it for 1.3, add destroy as a non-common method >> where needed (like in ZIP, in all other cases the user code should have >> access to the underlying stream) - and revisit this in the 2.x >> timeframe, even though I can't see what could be broken by always >> closing the stream in a finally block. >> Does anybody see a good reason why the close behavior should stay the >> way it is right now? > For output, it seems sensible to close the underlying stream on > failure, as the content is likely to be garbage. Yes, this is the case I was talking about. > For input, there might be a use case for leaving the stream open, in > case some kind of recovery is possible. All our implementations - except for the new dump code, that doesn't properly handle close ATM - try to close the input stream regardless of whether there has been a problem with the archive or not (they don't even check). This is what I'd expect it to do as well. Some of the guard against closing System.in, but not all. > It would be useful to have a way of determining the input file pointer > at the point of failure. stream.getBytesRead() or are you thinking about anything else? Stefan - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [compress] close() and archives/streams in an invalid state
On 15 August 2011 09:56, Stefan Bodewig wrote: > Hi, > > while working on the Zip64 stuff I deliberately created some invalid > archives to test I get the expected exceptions. While doing so I > realized I couldn't close the underlying stream because > ZipArchiveOutputStream's close would throw an exception as finish() > failed before ever closing the wrapped stream. The calling code may not > even have a handle to the underlying stream if it used the File-arg > constructor and so in the end a broken archive leaks the file > descriptor. > > Then I went looking through the other implementations and found we are > consistent here as far as archivers and bzip2 go. All of them will not > close the underlying stream if the archive/stream is invalid. I'm not > sure what gzip does as it just delegates to close in the Java classlib's > GZipOutputStream. > > I wonder whether we should rather change the behavior to always close > the underlying stream or whether we should add a new method - I called > in destroy in ZipArchiveOutputStream - that does so. > > Or maybe we just document it for 1.3, add destroy as a non-common method > where needed (like in ZIP, in all other cases the user code should have > access to the underlying stream) - and revisit this in the 2.x > timeframe, even though I can't see what could be broken by always > closing the stream in a finally block. > > Does anybody see a good reason why the close behavior should stay the > way it is right now? For output, it seems sensible to close the underlying stream on failure, as the content is likely to be garbage. For input, there might be a use case for leaving the stream open, in case some kind of recovery is possible. It would be useful to have a way of determining the input file pointer at the point of failure. > Stefan > > - > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > > - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
[compress] close() and archives/streams in an invalid state
Hi, while working on the Zip64 stuff I deliberately created some invalid archives to test I get the expected exceptions. While doing so I realized I couldn't close the underlying stream because ZipArchiveOutputStream's close would throw an exception as finish() failed before ever closing the wrapped stream. The calling code may not even have a handle to the underlying stream if it used the File-arg constructor and so in the end a broken archive leaks the file descriptor. Then I went looking through the other implementations and found we are consistent here as far as archivers and bzip2 go. All of them will not close the underlying stream if the archive/stream is invalid. I'm not sure what gzip does as it just delegates to close in the Java classlib's GZipOutputStream. I wonder whether we should rather change the behavior to always close the underlying stream or whether we should add a new method - I called in destroy in ZipArchiveOutputStream - that does so. Or maybe we just document it for 1.3, add destroy as a non-common method where needed (like in ZIP, in all other cases the user code should have access to the underlying stream) - and revisit this in the 2.x timeframe, even though I can't see what could be broken by always closing the stream in a finally block. Does anybody see a good reason why the close behavior should stay the way it is right now? Stefan - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org