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

Add H5FD_http_finalize function and call on hdf5 finalize #2827

Merged
merged 2 commits into from
Dec 19, 2023

Conversation

lostbard
Copy link
Contributor

Fixes #2617

@CLAassistant
Copy link

CLAassistant commented Dec 12, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@DennisHeimbigner DennisHeimbigner left a comment

Choose a reason for hiding this comment

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

I do not think this is correct. You are replacing H5FD_http_term with H5FD_http_finalize (BTW why the name change?). But there are other occurrences of H5FD_http_term that you missed e.g. near line 173 of H5FDhttp.c.

@lostbard
Copy link
Contributor Author

lostbard commented Dec 13, 2023

H5FD_http_finalize is added to unregister the class - the terminate call in the class structure (H5FD_http_term) is still there and potentially called from within the class, just it doesnt need to do anything in this case

It looks like the terminate call is called as class shutsdown, which is normally when HDF5 library closes. In a case where netcdf AND hdf5 are closing at the same time, this is ok.

In the case like octave where the H5 library is still around after netcdf has closed, it is a problem as the h5 library still has references to classes registered from netcdf.

Hence the new function intended on just unregistering the class that was registered via H5FD_http_init

@DennisHeimbigner
Copy link
Collaborator

But wouldn't it work to add the unregister to H5fD_http_term?

@lostbard
Copy link
Contributor Author

But wouldn't it work to add the unregister to H5fD_http_term?

Possibly? But you would also need to call H5fD_http_term from within hdf5dispatch and provide it as an external function within libhdf5/H5FDhttp.h

That also assumes that calling unregister from that function when its being as terminate() on a shutdown in the class is also ok.

@lostbard
Copy link
Contributor Author

I did a quick test and calling unregister from H5FD_http_term causes a crash unless it is removed from being a reference in the H5FD_http_g struct.

If you would rather keep the function as that and call it from dispatch, but remove from the class as terminate, I can change it

@WardF WardF added this to the 4.9.3 milestone Dec 13, 2023
@lostbard
Copy link
Contributor Author

So going forward, I should call unregister from H5fD_http_term, call H5fD_http_term from within hdf5dispatch, remove H5fD_http_term from being referenced in the H5FD_http_g class and remove H5FD_http_finalize ?

@WardF WardF merged commit 9836598 into Unidata:main Dec 19, 2023
99 checks passed
@lostbard lostbard deleted the octave-fix-2617 branch December 20, 2023 01:15
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.

Segfault in H5FD__free_cls with netcdf 4.9.1
4 participants