Skip to content

Destroy Attributes's handle before its parent_link#662

Merged
jkotan merged 1 commit intoess-dmsc:masterfrom
FlyingSamson:fix-attribute-member-destruction-order
Dec 12, 2024
Merged

Destroy Attributes's handle before its parent_link#662
jkotan merged 1 commit intoess-dmsc:masterfrom
FlyingSamson:fix-attribute-member-destruction-order

Conversation

@FlyingSamson
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@FlyingSamson FlyingSamson deleted the fix-attribute-member-destruction-order branch January 9, 2025 08:48
@jkotan jkotan mentioned this pull request May 2, 2025
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