Reliable clean-up of SafeHandles in Deflater/Inflater #114826
Conversation
If the call to CreateZLibStreamForXXflate throws an exception from the constructor, explicitly dispose of the ZLibStreamHandle (rather than relying on the finalizer to do it.) Also incorporates a change to DeallocateBufferHandle (called from the Dispose path) to prevent it using a ZlibStreamHandle after it's been Disposed.
This aligns Inflater with Deflater. Removed now-unnecessary usings.
src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Deflater.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Deflater.cs
Show resolved
Hide resolved
src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Deflater.cs
Outdated
Show resolved
Hide resolved
rzikm
left a comment
There was a problem hiding this comment.
Looks reasonable so far, @edwardneal, are you still looking to contribute this change? Have you had time to look into addressing #89445 as well?
src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Inflater.cs
Outdated
Show resolved
Hide resolved
Separates the initialization of a ZLibStreamHandle and only instantiates a Deflater or Inflater instance if this initialization succeeds
rzikm
left a comment
There was a problem hiding this comment.
I think this looks good now, besides the few nits from @stephentoub .
Stephen, do you want to take another look?
Add explanatory comment to WebSockets csproj.
|
@stephentoub I think your comments are addressed now, do you want to give this PR another review? |
|
@stephentoub Gentle ping in case this slipped your inbox |
src/libraries/System.IO.Compression/src/System/IO/Compression/DeflateZLib/Inflater.cs
Show resolved
Hide resolved
Moved error handling to XxflateInit2_. These methods will either return successfully or throw an exception. Resource string and code style changes.
|
Hey @edwardneal, there still seem to be some unaddressed code review feedback, can you look into it? |
|
There was one review comment on the name for the exception in Another two comments (the string resource and the reference) are covered in this response. I think I've answered the question, but I've left it open in case there's anything to follow up. This leaves some feedback on code duplication between |
|
Thanks for being patient with us, I think the PR is in good shape to go, I am happy to leave further improvements on #114826 (comment) to a follow up PRs |
Fixes #89445
This reimplements #71991 in
DeflaterandInflater. The PR was previously reverted in #85001 as a result of issue #84994.As noted in the reverting PR,
Deflatersimply needed to dispose of the returned SafeHandle and supress its own finalizer ifCreateZLibStreamForDeflatethrew an exception. We now do this.We couldn't do this for
Inflaterat the time - it had to support concatenated GZip data, and as part of that support it would recreate itsZLibStreamHandle. As a result of #113587, this recreation no longer happens and we can treatInflateras we would treatDeflater. I've refactoredInflater.InflateInitout of existence and made_zlibStreamreadonly to reflect this.This also incorporates a change to both class'
DeallocateBufferHandlemethods (when called byDispose) to prevent them dereferencing aZLibStreamHandleafter it's been disposed of. This is roughly how #84994 was originally discovered.