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

Make file closing and get_obj_ids more robust #1854

Merged
merged 4 commits into from
Mar 17, 2021

Conversation

takluyver
Copy link
Member

@takluyver takluyver commented Mar 10, 2021

This now contains two related changes.

First, temporarily disable garbage collection in h5py.h5f.get_obj_ids, so it can't invalidate HDF5 IDs between us retrieving them and calling H5Iinc_ref to keep them alive. This fixes #1852.

However, get_obj_ids is still a fairly awkward thing to call from File.close() - it creates h5py ObjectID objects, which can cause problems at interpreter shutdown, and it increments HDF5 ref counts only to decrement them again. Now it also has to turn garbage collection off and on again. So I moved part of the File.close logic into a FileID._close_open_objects method, which calls the autogenerated bindings directly and avoids these complexities. This fixes #1495.

Preventing IDs from being dec_ref-ed to 0 between retrieving them and
inc_ref-ing them.

Closes h5pygh-1852
@codecov
Copy link

codecov bot commented Mar 10, 2021

Codecov Report

Merging #1854 (db12655) into master (7f91885) will decrease coverage by 1.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1854      +/-   ##
==========================================
- Coverage   88.40%   87.28%   -1.13%     
==========================================
  Files          17       17              
  Lines        2285     2178     -107     
==========================================
- Hits         2020     1901     -119     
- Misses        265      277      +12     
Impacted Files Coverage Δ
h5py/_hl/files.py 91.47% <100.00%> (+0.08%) ⬆️
h5py/_hl/filters.py 84.97% <0.00%> (-7.81%) ⬇️
h5py/_hl/attrs.py 95.20% <0.00%> (-0.99%) ⬇️
h5py/_hl/dims.py 95.18% <0.00%> (-0.70%) ⬇️
h5py/_hl/datatype.py 95.00% <0.00%> (-0.66%) ⬇️
h5py/_hl/selections.py 87.43% <0.00%> (-0.57%) ⬇️
h5py/_hl/dataset.py 92.30% <0.00%> (-0.22%) ⬇️
h5py/_hl/group.py 96.62% <0.00%> (-0.13%) ⬇️
h5py/_hl/vds.py 97.33% <0.00%> (-0.04%) ⬇️
h5py/_hl/base.py 95.83% <0.00%> (+0.20%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f91885...db12655. Read the comment docs.

@takluyver takluyver changed the title Disable garbage collection during h5f.get_obj_ids Make file closing and get_obj_ids more robust Mar 10, 2021
@asmeurer
Copy link
Contributor

I can confirm that this also makes the issue go away for the other reproducers I'm aware of.

@tacaswell tacaswell merged commit cff8537 into h5py:master Mar 17, 2021
@takluyver takluyver deleted the get-obj-ids-gc branch April 27, 2021 13:43
@asmeurer
Copy link
Contributor

asmeurer commented Jun 4, 2021

Is there a rough timeline on when a version with this will be released?

@takluyver
Copy link
Member Author

Sorry for the delay. Things seem to have gone quiet, which hopefully means 3.2.1 is working OK for most people. But I'll try to do a new release some time soon. I would like to:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants