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

Fix ishdf5 error handling and improve general error silencing #780

Merged
merged 2 commits into from
Dec 15, 2020

Conversation

musm
Copy link
Member

@musm musm commented Dec 14, 2020

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.

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.
@musm musm requested a review from jmert December 14, 2020 20:39
estack = H5E_DEFAULT
func, client_data = h5e_get_auto(estack)
h5e_set_auto(estack, C_NULL, C_NULL)
try
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the try...finally statement is required in the case where f() throws an error message, otherwise the error stack wouldn't get reset back.

src/HDF5.jl Outdated
Comment on lines 638 to 639
function ishdf5(name::AbstractString)
# v1.12 use more robust h5f_isaccesible
Copy link
Contributor

@jmert jmert Dec 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
function ishdf5(name::AbstractString)
# v1.12 use more robust h5f_isaccesible
function ishdf5(name::AbstractString)
isfile(name) || return false
# v1.12 use more robust h5f_isaccesible

I think the main case where h5f_is_hdf5 prints errors rather than just returning false is when the file isn't accessible to do the check (such as it not existing). We could do any number of fancier checks on permissions to try to avoid having to do the error handler swapping, but I expect the swap eventually becomes cheap compared to a large number of filesystem queries.

Just checking if the file exists should be cheap though because (a) if it doesn't, we avoid the error handler swap, and (b) if it does, libhdf5 is just going to do the same lookup, and that should be cached by the OS so should be fast.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the main case where h5f_is_hdf5 prints errors rather than just returning false is when the file isn't accessible to do the check (such as it not existing).

yes, I think that's right, that was the main case I was working around. I think its reasonable to assume that that's the main case where an error is mistakenly thrown by libhdf5.

To be clear, are you proposing that I change this function to only do the isfile check ?

function ishdf5(name::AbstractString)
    isfile(name) || return false
    # v1.12 use more robust h5f_is_accesible
    h5f_is_hdf5(name)
end

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I think the rest probably is necessary and unavoidable since there are a myriad number of other ways the file could exist but not be validly readable (touch unreadable && chmod 000 unreadable seems to do it...), but I thought that fast-pathing past a nonexistent file might be worth the one extra check (since the OS filesystem caches should make that basically free in the case where the file does get opened later).

@musm musm merged commit d1bdad8 into JuliaIO:master Dec 15, 2020
@musm musm deleted the errorhandling branch December 15, 2020 03:02
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.

2 participants