Fix ishdf5 error handling and improve general error silencing (#780)

* Fix ishdf5 error handling and improve general error silencing

The docs state that h5f_is_hdf5 does not error throw, but that is not true.
We first suppress the hdf5 libraries stack trace printing, and then fix
the return to ensure false condition is thrown when the file is not
accessible in the hdf5 format.

We have also renamed the hiding_errors method and ensured that the final
stacktrace reset is guaranteed in case the underlying function call
throws an error.

docs/src/api_bindings.md

@@ -70,6 +70,7 @@ h5d_write(dataset_id::hid_t, mem_type_id::hid_t, mem_space_id::hid_t, file_space
 ## [`H5E`](https://portal.hdfgroup.org/display/HDF5/Error+Handling) — Error Interface
 ```julia
 h5e_get_auto(estack_id::hid_t, func::Ref{Ptr{Cvoid}}, client_data::Ref{Ptr{Cvoid}})
+h5e_get_current_stack()
 h5e_set_auto(estack_id::hid_t, func::Ptr{Cvoid}, client_data::Ptr{Cvoid})
 ```
 

gen/api_defs.jl

@@ -74,6 +74,7 @@
 
 @bind h5e_get_auto(estack_id::hid_t, func::Ref{Ptr{Cvoid}}, client_data::Ref{Ptr{Cvoid}})::herr_t "Error getting error reporting behavior"
 @bind h5e_set_auto(estack_id::hid_t, func::Ptr{Cvoid}, client_data::Ptr{Cvoid})::herr_t "Error setting error reporting behavior"
+@bind h5e_get_current_stack()::hid_t "Unable to return current error stack"
 
 ###
 ### File Interface
@@ -89,7 +90,7 @@
 @bind h5f_get_obj_count(file_id::hid_t, types::Cuint)::Cssize_t "Error getting object count"
 @bind h5f_get_obj_ids(file_id::hid_t, types::Cuint, max_objs::Csize_t, obj_id_list::Ptr{hid_t})::Cssize_t "Error getting objects"
 @bind h5f_get_vfd_handle(file_id::hid_t, fapl_id::hid_t, file_handle::Ref{Ptr{Cvoid}})::herr_t "Error getting VFD handle"
-@bind h5f_is_hdf5(pathname::Cstring)::htri_t error("Cannot access file ", pathname)
+@bind h5f_is_hdf5(pathname::Cstring)::htri_t error("Unable to access file ", pathname)
 @bind h5f_open(pathname::Cstring, flags::Cuint, fapl_id::hid_t)::hid_t error("Error opening file ", pathname)
 @bind h5f_start_swmr_write(id::hid_t)::herr_t "Error starting SWMR write"
 

src/HDF5.jl

@@ -409,8 +409,8 @@ Pass `swmr=true` to enable (Single Writer Multiple Reader) SWMR write access for
 """
 function h5open(filename::AbstractString, mode::AbstractString = "r"; swmr::Bool = false, pv...)
     # With garbage collection, the other modes don't make sense
-    apl = create_property(H5P_FILE_ACCESS; pv..., fclose_degree = H5F_CLOSE_STRONG) # file access property list
-    cpl = isempty(pv) ? DEFAULT_PROPERTIES : create_property(H5P_FILE_CREATE; pv...) # file create property list
+    fapl = create_property(H5P_FILE_ACCESS; pv..., fclose_degree = H5F_CLOSE_STRONG) # file access property list
+    fcpl = isempty(pv) ? DEFAULT_PROPERTIES : create_property(H5P_FILE_CREATE; pv...) # file create property list
     rd, wr, cr, tr, ff =
         mode == "r"  ? (true,  false, false, false, false) :
         mode == "r+" ? (true,  true,  false, false, true ) :
@@ -425,20 +425,18 @@ function h5open(filename::AbstractString, mode::AbstractString = "r"; swmr::Bool
 
     if cr && (tr || !isfile(filename))
         flag = swmr ? H5F_ACC_TRUNC|H5F_ACC_SWMR_WRITE : H5F_ACC_TRUNC
-        fid = h5f_create(filename, flag, cpl, apl)
+        fid = h5f_create(filename, flag, fcpl, fapl)
     else
-        if !h5f_is_hdf5(filename)
-            error("This does not appear to be an HDF5 file")
-        end
+        ishdf5(filename) || error("unable to determine if $filename is accessible in the HDF5 format (file may not exist)")
         if wr
             flag = swmr ? H5F_ACC_RDWR|H5F_ACC_SWMR_WRITE : H5F_ACC_RDWR
         else
             flag = swmr ? H5F_ACC_RDONLY|H5F_ACC_SWMR_READ : H5F_ACC_RDONLY
         end
-        fid = h5f_open(filename, flag, apl)
+        fid = h5f_open(filename, flag, fapl)
     end
-    close(apl)
-    cpl != DEFAULT_PROPERTIES && close(cpl)
+    close(fapl)
+    fcpl != DEFAULT_PROPERTIES && close(fcpl)
     return File(fid, filename)
 end
 
@@ -635,9 +633,19 @@ end
 """
     ishdf5(name::AbstractString)
 
-Returns `true` if `name` is a path to a valid hdf5 file, `false` otherwise.
+Returns `true` if the file specified by `name` is in the HDF5 format, and `false` otherwise.
 """
-ishdf5(name::AbstractString) = h5f_is_hdf5(name)
+function ishdf5(name::AbstractString)
+    isfile(name) || return false # fastpath in case the file is non-existant
+    # TODO: v1.12 use the more robust h5f_is_accesible
+    try
+        # docs falsely claim h5f_is_hdf5 doesn't error, but it does and prints the error stack on fail
+        # silence the error stack in case the call throws
+        return silence_errors(() -> h5f_is_hdf5(name))
+    catch
+        return false
+    end
+end
 
 # Extract the file
 file(f::File) = f
@@ -1968,14 +1976,15 @@ function create_external(source::Union{File,Group}, source_relpath, target_filen
 end
 
 # error handling
-function hiding_errors(f)
-    error_stack = H5E_DEFAULT
-    # error_stack = ccall((:H5Eget_current_stack, libhdf5), hid_t, ())
-    old_func, old_client_data = h5e_get_auto(error_stack)
-    h5e_set_auto(error_stack, C_NULL, C_NULL)
-    res = f()
-    h5e_set_auto(error_stack, old_func, old_client_data)
-    return res
+function silence_errors(f::Function)
+    estack = H5E_DEFAULT
+    func, client_data = h5e_get_auto(estack)
+    h5e_set_auto(estack, C_NULL, C_NULL)
+    try
+        return f()
+    finally
+        h5e_set_auto(estack, func, client_data)
+    end
 end
 
 # Define globally because JLD uses this, too

src/api.jl

@@ -249,6 +249,12 @@ function h5e_set_auto(estack_id, func, client_data)
     return nothing
 end
 
+function h5e_get_current_stack()
+    var"#status#" = ccall((:H5Eget_current_stack, libhdf5), hid_t, ())
+    var"#status#" < 0 && error("Unable to return current error stack")
+    return var"#status#"
+end
+
 function h5f_close(file_id)
     var"#status#" = ccall((:H5Fclose, libhdf5), herr_t, (hid_t,), file_id)
     var"#status#" < 0 && error("Error closing file")
@@ -311,7 +317,7 @@ end
 
 function h5f_is_hdf5(pathname)
     var"#status#" = ccall((:H5Fis_hdf5, libhdf5), htri_t, (Cstring,), pathname)
-    var"#status#" < 0 && error("Cannot access file ", pathname)
+    var"#status#" < 0 && error("Unable to access file ", pathname)
     return var"#status#" > 0
 end
 

src/api_helpers.jl

@@ -51,10 +51,10 @@ end
 ### Error Interface
 ###
 
-function h5e_get_auto(error_stack)
+function h5e_get_auto(estack_id)
     func = Ref{Ptr{Cvoid}}()
     client_data = Ref{Ptr{Cvoid}}()
-    h5e_get_auto(error_stack, func, client_data)
+    h5e_get_auto(estack_id, func, client_data)
     return func[], client_data[]
 end
 

src/show.jl

@@ -54,20 +54,15 @@ function Base.show(io::IO, dtype::Datatype)
         h5t_committed(dtype) && print(io, name(dtype), " ")
         print(io, h5lt_dtype_to_text(dtype))
     else
-        # Note that h5i_is_valid returns `false` on the built-in datatypes (e.g.
-        # H5T_NATIVE_INT), apparently because they have refcounts of 0 yet are always
-        # valid. Just temporarily turn off error printing and try the call to probe if
-        # dtype is valid since H5LTdtype_to_text special-cases all of the built-in types
-        # internally.
-        old_func, old_client_data = h5e_get_auto(H5E_DEFAULT)
-        h5e_set_auto(H5E_DEFAULT, C_NULL, C_NULL)
+        # Note that h5i_is_valid returns `false` on the built-in datatypes (e.g. H5T_NATIVE_INT),
+        # apparently because they have refcounts of 0 yet are always valid. Just temporarily turn
+        # off error printing and try the call to probe if dtype is valid since H5LTdtype_to_text
+        # special-cases all of the built-in types internally.
         local text
         try
-            text = h5lt_dtype_to_text(dtype)
+            text = silence_errors(() -> h5lt_dtype_to_text(dtype))
         catch
             text = "(invalid)"
-        finally
-            h5e_set_auto(H5E_DEFAULT, old_func, old_client_data)
         end
         print(io, text)
     end

test/plain.jl

@@ -1185,3 +1185,22 @@ create_dataset(hfile, "/group1/dset1", 1)
 @test_throws ErrorException create_dataset(g1, "dset1", 1)
 
 end
+
+@testset "HDF5 existance" begin
+
+fn1 = tempname()
+fn2 = tempname()
+
+open(fn1, "w") do f
+    write(f, "Hello text file")
+end
+
+@test !HDF5.ishdf5(fn1) # check that a non-hdf5 file retuns false
+@test !HDF5.ishdf5(fn2) # checks that a file that does not exist returns false
+
+@test_throws ErrorException h5write(fn1, "x", 1) # non hdf5 file throws
+h5write(fn2, "x", 1)
+
+@test HDF5.ishdf5(fn2)
+
+end

test/properties.jl

@@ -41,7 +41,7 @@ h5open(fn, "w";
     @test HDF5.h5i_get_type(fapl[:driver]) == HDF5.H5I_VFL
     # Docs say h5p_get_driver_info() doesn't error, but it does print an error message...
     #   https://portal.hdfgroup.org/display/HDF5/H5P_GET_DRIVER_INFO
-    HDF5.hiding_errors() do
+    HDF5.silence_errors() do
         @test fapl[:driver_info] == C_NULL
     end
     @test fapl[:fclose_degree] == HDF5.H5F_CLOSE_STRONG