oleg-st / ZstdSharp

Port of zstd compression library to c#
MIT License
200 stars 29 forks source link

DecompressionStream exception on dispose #4

Closed alexanderzuban closed 2 years ago

alexanderzuban commented 2 years ago

DecompressionStream raises an exception on disposal:

{{{ ... if (lastDecompressResult != 0) throw new EndOfStreamException("Premature end of stream"); ... }}}

Because the class has a finalizer which is calling Disposal this may lead to exception/crash during finalization, which is impossible to handle/control. It will be great to change it to avoid crashes during GC.

{{{ System.IO.EndOfStreamException: Premature end of stream DecompressionStream.Dispose (System.Boolean disposing) DecompressionStream.Finalize () }}}

Also, it may have the sense to always release input stream buffer during disposal, to avoid possible memory leaks (I didn't dive deep what does lastDecompressResult means if this is a flag that we still have something to decompress, it is possible that the reader just stop to read from the stream for some reason, but disposal should release buffer in this case anyway)

oleg-st commented 2 years ago

Yes, nonzero lastDecompressResult means we are not at the end of the frame. It should be checked after innerStream's end, not in Dispose / Finalize. It's means premature end of innerStream (truncated input).

alexanderzuban commented 2 years ago

this is a possible situation when the source stream is a response that was interrupted.

oleg-st commented 2 years ago

Yes, I think EndOfStreamException is valid in this case

alexanderzuban commented 2 years ago

but this will lead to mem leak in the general case(stream will never release pooled buffer) and app crash if stream usage is missing using(this is bad, but the app doesn't expect GC resource cleanup will crash)

oleg-st commented 2 years ago

It will be thrown in Read / ReadAsync, not in Dispose / Finalize I'll fix it after a while

alexanderzuban commented 2 years ago

Somehow I am getting it the Finalize :) Also, even if you correctly use "Dispose", because this exception prevents setting decompressor to null Finalize will get this exception as well. So it is literally not possible to release resources/avoid crashes.

image

oleg-st commented 2 years ago

Fixed in 0.5.2

oleg-st commented 2 years ago

@alexanderzuban Could you check it?

alexanderzuban commented 2 years ago

Seems to work fine now, thank you!

oleg-st commented 2 years ago

Thank you!