JuliaIO / HDF5.jl

Save and load data in the HDF5 file format from Julia
https://juliaio.github.io/HDF5.jl
MIT License
386 stars 140 forks source link

Implement try_close_finalizer, fix #1048 #1049

Closed mkitti closed 1 year ago

mkitti commented 1 year ago

The formatting is a bit weird.

function try_close_finalizer(x)
    if !islocked(liblock) && trylock(liblock) do
        close(x)
        true
    end
    else
        finalizer(try_close_finalizer, x)
    end
end

The MWE of #1048 seems to pass now. How do I turn this into a test?

julia> begin
       using HDF5

       fid = h5open(tempname() * ".h5", "w")

       d = create_dataset(fid, "data", datatype(Int), ((1_000_000,1), (-1,1)), chunk=(1,1))

       t = Threads.@spawn begin
           for i=1:1_000_000
               d[i,1] = 1
           end
       end
       wait(t)

       close(fid)
       end
mkitti commented 1 year ago

@tachawkes does this branch resolve the issue?

using Pkg
pkg"activate --temp"
pkg"add https://github.com/mkitti/HDF5.jl#mkitti/try_close_finalizer"
begin
       using HDF5

       fid = h5open(tempname() * ".h5", "w")

       d = create_dataset(fid, "data", datatype(Int), ((1_000_000,1), (-1,1)), chunk=(1,1))

       t = Threads.@spawn begin
           for i=1:1_000_000
               d[i,1] = 1
           end
       end
       wait(t)

       close(fid)
end
TacHawkes commented 1 year ago

Yes! Thank you so much for locking into this so quickly.

With this branch my MWE does not crash anymore.

mkitti commented 1 year ago

@TacHawkes does this fix the primary code where you initially observed the issue?

By the way, why are you using multithreading here? The locks we have installed basically limit access to the HDF5 library to a single thread at a time. The underlying HDF5 C library is not thread safe.

TacHawkes commented 1 year ago

Actually, it does!

I'm using this in a larger Julia package where analog data is read from a fast ADC, processed by Julia and written in chunks to the disk using HDF5. The rest of the software is using multi-threading and I have written a small convenience layer around HDF5.jl for abstracting all details of datasets, file handling etc. and these threads are calling to the library from their threads. Therefore I have added another lock in my layer to guard the HDF5 file as I am aware that only single-threaded access is possible. Actually, I do not call write that fast in my project but there is another multi-threading issue currently being investigated and while debugging, I found this issue in HDF5.jl.

mkitti commented 1 year ago

I'm considering merging and releasing this within the next 12 hours since a second user has encountered this issue.

cc: @musm @simonbyrne

mkitti commented 1 year ago

Fix resolves issues for two users. Merging. Happy to discuss organization and naming in later pull requests.