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

H5Fget_obj_ids/count no longer count copies of transient types #3928

Closed
wants to merge 2 commits into from

Conversation

mattjala
Copy link
Contributor

@mattjala mattjala commented Jan 5, 2024

When using H5Fget_obj_ids/H5Fget_obj_count over all files, copies of transient datatypes are considered open object identifiers. Transient datatypes don't exist on any file, don't need to be closed, and the documentation specifically states that only committed datatypes are returned, so this behavior doesn't make sense.

The Fortran mounting test previously used copies of transient datatypes to test object counting in mounted files. It now uses groups for that purpose.

This could be considered a change to the behavior of API functions, so I added a note to RELEASE.txt.

Addresses #3316

@mattjala mattjala added Merge - To 1.14 Priority - 2. Medium ⏹ It would be nice to have this in the next release Component - C Library Core C library issues (usually in the src directory) Type - Bug / Bugfix Please report security issues to [email protected] instead of creating an issue on GitHub labels Jan 5, 2024
Copy link
Contributor

@qkoziol qkoziol left a comment

Choose a reason for hiding this comment

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

Shouldn't call API routines from internal library routines.

src/H5F.c Outdated
FUNC_ENTER_PACKAGE_NOERR
FUNC_ENTER_PACKAGE

if ((obj_type = H5Iget_type(obj_id)) < 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a bug and should be corrected. However, the library internal routines should never (with very few exceptions) call API routines. I'll take a look at which internal routines to use later tonight. (If someone else doesn't get to it before me :-) ).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced H5Iget_type -> H5I_get_type, H5Tcommitted -> H5I_object_verify + H5T_is_named

@brtnfld
Copy link
Contributor

brtnfld commented Jan 5, 2024

It seems inconsistent that an object is not considered "technically" open (and thus not counted), yet it can still be closed using the H5Tclose function. Additionally, a copied type should be closed even if not created.

This is similar to the debate on whether the objects opened through an external link should be counted, as they are also considered "transient." ref. HDFFV-11231.

Regardless, I'm assuming most applications use H5Fget_obj_count to get a count of objects that need to be closed, including H5Tcopy ids. Besides, we currently don't provide a way to detect these types of open objects ( property lists, datatypes, etc...) otherwise, which is another issue.

@mattjala
Copy link
Contributor Author

mattjala commented Jan 8, 2024

Regardless, I'm assuming most applications use H5Fget_obj_count to get a count of objects that need to be closed, including H5Tcopy ids. Besides, we currently don't provide a way to detect these types of open objects ( property lists, datatypes, etc...) otherwise, which is another issue.

This is true - these changes should be paired with some kind of API addition that does return all ids that need to be closed, whether that's a new API call or a new option for these calls. That would be for the next release, since it's definitely an API change. For now, I'll mark this as draft and update the current documentation for H5Fget_obj_ids/H5Fget_obj_count to reflect how they currently work (#3932).

@mattjala mattjala marked this pull request as draft January 8, 2024 17:13
@mattjala
Copy link
Contributor Author

mattjala commented Mar 8, 2024

For now, this work is being moved to the h5f_get_obj_rework feature branch.

@mattjala mattjala closed this Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - C Library Core C library issues (usually in the src directory) Priority - 2. Medium ⏹ It would be nice to have this in the next release Type - Bug / Bugfix Please report security issues to [email protected] instead of creating an issue on GitHub
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants