CodecZstd.jl icon indicating copy to clipboard operation
CodecZstd.jl copied to clipboard

Auto initialize in `startproc`

Open nhz2 opened this issue 1 year ago • 4 comments

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 startproc is called. If the context is null startproc initializes the context.

This means finalize is still needed to prevent memory leaks, but there is no issue of use after free.

nhz2 avatar Sep 16 '24 16:09 nhz2

Codecov Report

Attention: Patch coverage is 75.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 61.35%. Comparing base (e7edfed) to head (60079dd). Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/compression.jl 70.00% 3 Missing :warning:
src/decompression.jl 70.00% 3 Missing :warning:
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              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Sep 16 '24 16:09 codecov[bot]

Interesting approach. We may still want to consider the finalizer.

Yes, having a finalizer should be compatible with this PR, and would also prevent memory leaks.

nhz2 avatar Sep 17 '24 19:09 nhz2

@mkitti Could you review this PR again, I think I addressed your comments.

nhz2 avatar Sep 26 '24 23:09 nhz2

Overall, it looks fine. I will try to look more closely tonight.

mkitti avatar Sep 27 '24 00:09 mkitti

Thank you @nhz2 and @mkitti! This fixes https://github.com/JuliaIO/JLD2.jl/issues/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?

ali-ramadhan avatar Oct 06 '24 21:10 ali-ramadhan