API Lock Preferences #1022

mkitti · 2022-11-01T05:42:20Z

This builds on the sb/lock branch by using Preferences.jl to configure the lock usage at compile time.

The preference, use_api_lock can be set to either true or false.

The preference is set via the API.set_use_lock_pref! function and can be quered by API.get_use_lock_pref().

This mainly meant to help assess the sb/lock branch.

Other features such as the RawAPI module have been withdrawn.

simonbyrne · 2022-11-08T02:48:14Z

This adds a bunch of complexity, and 2 additional runtime lookups at each call. Personally i don't think it's worth it (especially since I haven't been able to see any noticeable overhead with the locking).

I think if you want to make it optional, just make it an all-or-nothing: use_api_lock is true/false with

const use_api_lock = @load_preference("use_api_lock", true)

and make the API function wrappers:

    use_api_lock && lock(liblock)
    var"#status#" = try
            # ccall
        finally
            use_api_lock && unlock(liblock)
        end
    var"#status#" < 0 && @h5error ...
    return nothing

mkitti · 2022-11-08T04:19:49Z

After testing, I am getting this error. I suspect this is because the finalizer is trying to run after we call h5_close().

HDF5.jl/test/runtests.jl

Line 77 in af62ebf

HDF5.API.h5_close()

error in running finalizer: HDF5.API.H5Error(msg="Error closing file", id=1008806316530991104)
macro expansion at /home/mkitti/.julia/dev/HDF5/src/api/error.jl:18 [inlined]
h5f_close at /home/mkitti/.julia/dev/HDF5/src/api/functions.jl:1054
close at /home/mkitti/.julia/dev/HDF5/src/file.jl:120 [inlined]
lock_and_close at /home/mkitti/.julia/dev/HDF5/src/api/lock.jl:58
unknown function (ip: 0x7f8eb5b0b122)
_jl_invoke at /cache/build/default-amdci5-0/julialang/julia-release-1-dot-8/src/gf.c:2367 [inlined]
ijl_apply_generic at /cache/build/default-amdci5-0/julialang/julia-release-1-dot-8/src/gf.c:2549
jl_apply at /cache/build/default-amdci5-0/julialang/julia-release-1-dot-8/src/julia.h:1838 [inlined]
run_finalizer at /cache/build/default-amdci5-0/julialang/julia-release-1-dot-8/src/gc.c:280
jl_gc_run_finalizers_in_list at /cache/build/default-amdci5-0/julialang/julia-release-1-dot-8/src/gc.c:367
run_finalizers at /cache/build/default-amdci5-0/julialang/julia-release-1-dot-8/src/gc.c:410
ijl_atexit_hook at /cache/build/default-amdci5-0/julialang/julia-release-1-dot-8/src/init.c:236
jl_repl_entrypoint at /cache/build/default-amdci5-0/julialang/julia-release-1-dot-8/src/jlapi.c:720
main at /cache/build/default-amdci5-0/julialang/julia-release-1-dot-8/cli/loader_exe.c:59
unknown function (ip: 0x7f8f0cec8d8f)
__libc_start_main at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
unknown function (ip: 0x401098)

simonbyrne · 2022-11-08T05:32:34Z

Is there a reason why the tests call HDF5.API.h5_close()?

mkitti · 2022-11-08T05:51:47Z

test/virtual_dataset.jl

@@ -5,36 +5,36 @@ using Test, HDF5

    filename = joinpath(dirname, "main.hdf5")

-    f = h5open(filename, "w")


The issue was here. We forgot to close this handle explicitly.

Probably best to pull this into a different PR

I can pull this into a different pull request, but this needs to be here as well since CI will error otherwise.

mkitti · 2022-11-08T18:23:54Z

@simonbyrne I simplified this along the lines of your #1022 (comment) . I think this will allow us to more easily diagnose any potential issues or regressions that might emerge from this.

I also removed the RawAPI module. Something I would like to pay particular attention to is the use of the iterate functions where the C library is calling back into Julia code. Do we have sufficient guarantees on what thread the calls back will be run on?

simonbyrne · 2022-11-08T23:24:34Z

src/api/lock.jl

+    the HDF5 C library is not thread-safe. Concurrent calls to the HDF5 library
+    from multiple threads will likely fail.
+    """
+    const use_api_lock = Preferences.@load_preference("use_api_lock", default = true)


Suggested change

const use_api_lock = Preferences.@load_preference("use_api_lock", default = true)

const use_api_lock::Bool = Preferences.@load_preference("use_api_lock", default = true)

then the error below is not necessary

My thinking here is that the error I wrote would be more descriptive than the following since this potentially faces an end user. I suppose I could try catch the MethodError though.

ERROR: MethodError: Cannot `convert` an object of type String to an object of type Bool Closest candidates are: convert(::Type{T}, ::Ptr) where T<:Integer at pointer.jl:23 convert(::Type{T}, ::T) where T<:Number at number.jl:6 convert(::Type{T}, ::Number) where T<:Number at number.jl:7

I guess? Seems like a bit of a weird error case to be worried about though.

It seems a bit too easy for someone to edit their LocalPreferences.jl and accidentally quote true. Maybe we need to add a tryparse.

julia> convert(Bool, "true") ERROR: MethodError: Cannot `convert` an object of type String to an object of type Bool Closest candidates are: convert(::Type{T}, ::Ptr) where T<:Integer at pointer.jl:23 convert(::Type{T}, ::T) where T<:Number at number.jl:6 convert(::Type{T}, ::Number) where T<:Number at number.jl:7 ... Stacktrace: [1] top-level scope @ REPL[65]:1 julia> tryparse(Bool, "true") true

Actually the type assertion a global const will not work with Julia 1.6, and I think we're still trying to maintain compat with Julia 1.3

simonbyrne · 2022-11-08T23:25:11Z

src/api/lock.jl

+    const use_lock_and_close =
+        Preferences.@load_preference("use_lock_and_close", default = true) && !use_api_lock


use_lock_on_close?

simonbyrne · 2022-11-09T00:04:08Z

src/api/lock.jl

+        @info "Please restart Julia for the use_api_lock preference to take effect"
+        Preferences.@set_preferences!("use_api_lock" => use_api_lock)


maybe only print the message if it has changed?

mkitti · 2022-11-09T01:07:54Z

Is there a reason why the tests call HDF5.API.h5_close()?

I don't know, but it sure does help catch unclosed handles.

mkitti · 2022-11-18T00:04:42Z

Closing and resubmitting PR directly to master.

mkitti added 2 commits October 31, 2022 20:54

Implement a configurable HDF5.API.use_lock() via a Ref, add RawAPI

e006db2

Use Preferences.jl to control API locking at compile time

9a43edc

mkitti requested a review from simonbyrne November 1, 2022 05:42

simonbyrne mentioned this pull request Nov 8, 2022

add lock around all API calls #1021

Merged

mkitti added 2 commits November 7, 2022 23:23

Simplify

61ae0ed

Fix file handle leak

a55f1ea

mkitti added 2 commits November 8, 2022 00:48

Add pref for use_lock_and_close, make 1.3 compatible

c226d15

Formatting

e073259

mkitti commented Nov 8, 2022

View reviewed changes

mkitti added 3 commits November 8, 2022 00:56

Add lock tests

b23d2eb

Add the test lock file

6bbd35c

Fix test typo

e1f2351

simonbyrne reviewed Nov 8, 2022

View reviewed changes

simonbyrne reviewed Nov 9, 2022

View reviewed changes

Apply suggestions from codereview

339c52c

mkitti added 4 commits November 8, 2022 22:54

Formatting

fa2c9cb

Add Julia 1.6 to tests

597e1c0

Make lock Julia 1.6 compatible

2919995

Formatting

b45bfb7

mkitti closed this Nov 18, 2022

This was referenced Nov 18, 2022

API Lock Preferences #1024

Open

Merge strategy #1025

Open

Fix virtual dataset tests, from mkitti/api_lock_preferences #1023

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API Lock Preferences #1022

API Lock Preferences #1022

mkitti commented Nov 1, 2022 •

edited

Loading

simonbyrne commented Nov 8, 2022 •

edited

Loading

mkitti commented Nov 8, 2022

simonbyrne commented Nov 8, 2022

mkitti Nov 8, 2022

musm Nov 9, 2022

mkitti Nov 11, 2022

mkitti commented Nov 8, 2022

simonbyrne Nov 8, 2022

mkitti Nov 8, 2022

simonbyrne Nov 9, 2022

mkitti Nov 9, 2022

mkitti Nov 9, 2022 •

edited

Loading

simonbyrne Nov 8, 2022

simonbyrne Nov 9, 2022

mkitti commented Nov 9, 2022

mkitti commented Nov 18, 2022

		@@ -5,36 +5,36 @@ using Test, HDF5

		filename = joinpath(dirname, "main.hdf5")

		f = h5open(filename, "w")

	const use_api_lock = Preferences.@load_preference("use_api_lock", default = true)
	const use_api_lock::Bool = Preferences.@load_preference("use_api_lock", default = true)

		const use_lock_and_close =
		Preferences.@load_preference("use_lock_and_close", default = true) && !use_api_lock

		@info "Please restart Julia for the use_api_lock preference to take effect"
		Preferences.@set_preferences!("use_api_lock" => use_api_lock)

API Lock Preferences #1022

API Lock Preferences #1022

Conversation

mkitti commented Nov 1, 2022 • edited Loading

simonbyrne commented Nov 8, 2022 • edited Loading

mkitti commented Nov 8, 2022

simonbyrne commented Nov 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkitti commented Nov 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkitti Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkitti commented Nov 9, 2022

mkitti commented Nov 18, 2022

mkitti commented Nov 1, 2022 •

edited

Loading

simonbyrne commented Nov 8, 2022 •

edited

Loading

mkitti Nov 9, 2022 •

edited

Loading