Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement try_close_finalizer, fix #1048 #1049

Merged
merged 3 commits into from
Feb 14, 2023

Conversation

mkitti
Copy link
Member

@mkitti mkitti commented Feb 3, 2023

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
Copy link
Member Author

mkitti commented Feb 3, 2023

@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
Copy link

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

With this branch my MWE does not crash anymore.

@mkitti
Copy link
Member Author

mkitti commented Feb 3, 2023

@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
Copy link

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.

src/api/lock.jl Outdated Show resolved Hide resolved
@mkitti
Copy link
Member Author

mkitti commented Feb 14, 2023

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

cc: @musm @simonbyrne

@mkitti
Copy link
Member Author

mkitti commented Feb 14, 2023

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

@mkitti mkitti merged commit 2203de3 into JuliaIO:master Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants