replace unsafe pointer operations with their gc-safe Ref equivalents #428
Conversation
| h5t_set_size(memtype_id, H5T_VARIABLE) | ||
| readarray(obj, memtype_id, buf) | ||
| # FIXME? Who owns the memory for each string? Will Julia free it? | ||
| # FIXME? Who owns the memory for each string? Julia v0.6+ won't free it. |
There was a problem hiding this comment.
Just to double check, do you know whether this comment still applies with your fix? If not no problem just wanted to know if this #FIXME can be removed or not. Thanks.
There was a problem hiding this comment.
I only answered one of the questions there. I suspect that implies the answer the the first question (is that this is a memory leak and we should be manually freeing the memory here after the call to unsafe_string), but perhaps instead HDF5 owns the memory (as a pointer into the mmap'd file?) and attempting to free it would immediately cause it to crash.
There was a problem hiding this comment.
Checking the docs, it appears there may supposed to be a call to H5Dvlen_reclaim before the call to h5t_close (and this function should perhaps be wrapped in try/catch so those get called even if there's an error with reading part of the data.
src/HDF5.jl
Outdated
| close, convert, done, eltype, endof, flush, getindex, ==, | ||
| isempty, isvalid, length, names, ndims, next, parent, read, | ||
| setindex!, show, size, sizeof, start, write, isopen | ||
|
|
||
| import Compat: StringVector |
There was a problem hiding this comment.
Should be using since StringVector not extended.
| end | ||
| p = Ref{Cstring}(nonnull_str) |
There was a problem hiding this comment.
if str = ["\0","a","b"]
then
julia> p = Ref{Cstring}(nonnull_str)
ERROR: ArgumentError: embedded NULs are not allowed in C strings: "\0"Is this expected?
There was a problem hiding this comment.
Yes, HDF5 appears to only support C-type strings (the string "\0" will get truncated to ""). The error message seems pretty self-explanitory to me? 🙂
There was a problem hiding this comment.
Sorry I wasn't clear. I meant to say that with this PR the following doesn't work (i.e. the "\0" doesn't get truncated to "")
using HDF5
fn = tempname()
f = h5open(fn, "w")
test = ["s","dfs","Hi there","\0"]
write(f, "test", test)
ERROR: ArgumentError: embedded NULs are not allowed in C strings: "\0"whereas on master it truncates the last element of test to ""
There was a problem hiding this comment.
Right, the last element should be "". Why would you ever want it to silently discard / truncate your data? :)
There was a problem hiding this comment.
Sry just wanted to confirm the behavior change.
There was a problem hiding this comment.
Np, happy to confirm
There was a problem hiding this comment.
Isn't there still an inconsistency here
For arrays with "\0" it will throw an ArgumentError, however if you just try to write a string with "\0" it will still truncate it to "" on this PR.
using HDF5
fn = tempname()
f = h5open(fn, "w")
write(f, "test", "\0")
Avoids using
pointerso that Julia can natively track the usage of the objects and ensure it is gc-rooted.