Auto initialize in startproc
#74
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #74 +/- ##
==========================================
- Coverage 61.55% 61.35% -0.21%
==========================================
Files 5 5
Lines 372 370 -2
==========================================
- Hits 229 227 -2
Misses 143 143 ☔ View full report in Codecov by Sentry. |
| @@ -111,6 +111,9 @@ function TranscodingStreams.startproc(codec::ZstdCompressor, mode::Symbol, error | |||
| end | |||
|
|
|||
| function TranscodingStreams.process(codec::ZstdCompressor, input::Memory, output::Memory, error::Error) | |||
| if codec.cstream.ptr == C_NULL | |||
| error("startproc must be called before process") | |||
There was a problem hiding this comment.
This error should also be unreachable in normal operation.
| codec.cstream.ptr = ptr | ||
| i_code = initialize!(codec.cstream, codec.level) | ||
| if iserror(i_code) | ||
| error[] = ErrorException("zstd initialization error") |
There was a problem hiding this comment.
These errors are unreachable unless there is some allocation error.
There was a problem hiding this comment.
Could you mock the out of memory condition them by using that advanced API that provides the memory allocation functions?
There was a problem hiding this comment.
I'm not sure if even that would reliably trigger an error specifically here, because memory allocations are happening in ZSTD_createCStream.
| codec.cstream.ptr = ptr | ||
| i_code = initialize!(codec.cstream, codec.level) | ||
| if iserror(i_code) | ||
| error[] = ErrorException("zstd initialization error") |
There was a problem hiding this comment.
Could you mock the out of memory condition them by using that advanced API that provides the memory allocation functions?
| reset!(codec.cstream.ibuffer) | ||
| reset!(codec.cstream.obuffer) |
There was a problem hiding this comment.
This happens in reset!
Lines 73 to 74 in 60079dd
Which is called in startproc
CodecZstd.jl/src/compression.jl
Line 104 in 60079dd
| throw(OutOfMemoryError()) | ||
| end | ||
| codec.cstream.ptr = ptr | ||
| i_code = initialize!(codec.cstream, codec.level) |
There was a problem hiding this comment.
Also see notes in #73
Should initialize! throw so we can catch it here and transmit the error?
There was a problem hiding this comment.
Yes, if initialize! is changed to throw, then this needs to catch that error and return :error, but for now initialize! returns an error code on failure.
Co-authored-by: Mark Kittisopikul <[email protected]>
Yes, having a finalizer should be compatible with this PR, and would also prevent memory leaks. |
|
@mkitti Could you review this PR again, I think I addressed your comments. |
|
Overall, it looks fine. I will try to look more closely tonight. |
|
Thank you @nhz2 and @mkitti! This fixes JuliaIO/JLD2.jl#599 which will allow us to use zstd to compress Oceananigans.jl outputs. Would it be possible to tag a new version of CodecZstd.jl? |
Fixes #70. Third time's the charm
This PR avoids the complexity of interacting with GC in #71 by checking if the context is null when
startprocis called.If the context is null
startprocinitializes the context.This means
finalizeis still needed to prevent memory leaks, but there is no issue of use after free.