JuliaVTK / WriteVTK.jl

Julia package for writing VTK XML files
Other
151 stars 32 forks source link

Release allocated resources in Zlib, fixes #43. #100

Closed fredrikekre closed 2 years ago

fredrikekre commented 2 years ago

Together with @KristofferC I managed to find a solution to the memory leak issue (https://github.com/jipolanco/WriteVTK.jl/issues/43). It seems that the allocated resources in zlib are never released, but calling finalize does this.

It is a bit unclear from the TranscodingStreams documentation whether this is necessary, or if writing the END_TOKEN should do this. For example: https://juliaio.github.io/TranscodingStreams.jl/latest/#Wrapped-streams-1 hints that one must do something to release them, but in the code here we don't want to call close(zWriter) because that would also close the wrapped stream. https://juliaio.github.io/TranscodingStreams.jl/latest/examples/#Explicitly-finish-transcoding-by-writing-TOKEN_END-1 is a bit unclear what it means to "finishes the current transcoding process without closing the underlying stream", i.e. should that also release the allocated resources or not.

Draft for now, since it is possible this is a bug in TranscodingStreams (see https://github.com/JuliaIO/TranscodingStreams.jl/issues/117)

codecov-commenter commented 2 years ago

Codecov Report

Merging #100 (1565685) into master (6ef44e4) will increase coverage by 0.00%. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #100   +/-   ##
=======================================
  Coverage   96.01%   96.01%           
=======================================
  Files          16       16           
  Lines         803      804    +1     
=======================================
+ Hits          771      772    +1     
  Misses         32       32           
Impacted Files Coverage Δ
src/write_data.jl 96.42% <100.00%> (+0.02%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6ef44e4...1565685. Read the comment docs.

KristofferC commented 2 years ago

Imo this should be merged since it fixes a real issue. If the API of TranscodungStreams changes, the code here can change too.

jipolanco commented 2 years ago

I agree with @KristofferC. I'll merge this if that's OK for @fredrikekre.

BTW thank you both for finding the solution!

KristofferC commented 2 years ago

Valgrind to the rescue :P

fredrikekre commented 2 years ago

Sounds good. I don't know what the failure on nightly is, but I don't think it is related.

jipolanco commented 2 years ago

Great, thanks! The failure on nightly doesn't seem to be related; it happens also on the current master.