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

Destroy Attributes's handle before its parent_link #662

Merged

Conversation

FlyingSamson
Copy link
Contributor

As pointed out by @jkotan in a comment to the related PR #660, the same issue as described in #659 also appears for Attributes:
If an Attribute instance is kept around after the last reference to a file was dropped in client code, the order of Attribute's members causes it to first try to close the underlying file before closing its own handle. If the file was opened with close degree Semi or the MPI-IO file driver was used, closing the file while the Attribute is still open is prohibited.

This PR, therefore, applies the same fix to Attribute as was applied to Node in #660; that is, it reorders the member of Attribute to ensure that the handle is destroyed first, before attempting to close the file through destroying the link.

Copy link
Member

@ggoneiESS ggoneiESS left a comment

Choose a reason for hiding this comment

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

lgtm, same as fix for #660 as described in PR

@jkotan
Copy link
Collaborator

jkotan commented Dec 11, 2024

I was just thinking, since it is hard to add proper unit tests maybe it would be good to add a small comment in the code about the issue in order no one revert it back.

@ggoneiESS
Copy link
Member

I was just thinking, since it is hard to add proper unit tests maybe it would be good to add a small comment in the code about the issue in order no one revert it back.

I don't see the harm in adding a comment to make it less opaque, but I plan to see if a test can be created using rdbuf, something like:


    std::ostringstream outputForTesting;
    auto* originalCerrBuffer = std::cerr.rdbuf(outputForTesting.rdbuf());

    // catch runtime_error

    REQUIRE(outputForTesting.str() == "Failed ~ObjectHandle:\n");

@jkotan
Copy link
Collaborator

jkotan commented Dec 12, 2024

I merge the current PR. We can add possible tests in a new PR

@jkotan jkotan merged commit 2a85825 into ess-dmsc:master Dec 12, 2024
29 checks passed
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.

3 participants