Destroy Node's handle before its Link

[FlyingSamson]

Fixes #659.

Nodes must first delete the handle to the underlying hdf5 resource before deleting their link.
Otherwise (i.e., link is deleted first), hdf5 attempts to close the file (if this was the last node referencing the file), which might fail (e.g., when using mpi-io, or close degree Semi) because the handle to the node would still be around.

I tried to add a test for this, but unfortunately the error message originates from the destructor of ObjectHandle catching any exception and only printing an error to stderr here without rethrowing to maintain noexept'ness and I could not find a sensible way to check what is printed to stderr within a catch2 test.

[FlyingSamson]

If that helps, this is a way to reproduce the issue (with the code prior to this PR):

namespace {
node::Group create_file_and_return_root_node() {
  file::create("node_destruction_order_test.h5", file::AccessFlags::Truncate);

  property::FileAccessList fapl{};
  fapl.close_degree(property::CloseDegree::Semi);
  auto f = file::open("node_destruction_order_test.h5", file::AccessFlags::ReadOnly, fapl);
  return f.root();
}
}  // namespace

TEST_CASE("testing Node member destruction order") {
  { node::Group n = create_file_and_return_root_node(); }
  REQUIRE(false);
}

and then running

ctest --test-dir build -T test --output-on-failure -R "^testing Node member destruction order$"

yields

HDF5-DIAG: Error detected in HDF5 (1.12.2) thread 0:
  #000: /home/f_spsoft/.spack/v0.21.3_deploy_spack_0.21_2024_11_12/spack-stage/spack-stage-hdf5-1.12.2-qdreo2z23uaq54fnrlsnfy273uk4seao/spack-src/src/H5F.c line 711 in H5Fclose(): decrementing file ID failed
    major: File accessibility
    minor: Unable to close file
  #001: /home/f_spsoft/.spack/v0.21.3_deploy_spack_0.21_2024_11_12/spack-stage/spack-stage-hdf5-1.12.2-qdreo2z23uaq54fnrlsnfy273uk4seao/spack-src/src/H5Iint.c line 1018 in H5I_dec_app_ref(): can't decrement ID ref count
    major: Object atom
    minor: Unable to decrement reference count
  #002: /home/f_spsoft/.spack/v0.21.3_deploy_spack_0.21_2024_11_12/spack-stage/spack-stage-hdf5-1.12.2-qdreo2z23uaq54fnrlsnfy273uk4seao/spack-src/src/H5Fint.c line 251 in H5F__close_cb(): unable to close file
    major: File accessibility
    minor: Unable to close file
  #003: /home/f_spsoft/.spack/v0.21.3_deploy_spack_0.21_2024_11_12/spack-stage/spack-stage-hdf5-1.12.2-qdreo2z23uaq54fnrlsnfy273uk4seao/spack-src/src/H5VLcallback.c line 3983 in H5VL_file_close(): file close failed
    major: Virtual Object Layer
    minor: Unable to close file
  #004: /home/f_spsoft/.spack/v0.21.3_deploy_spack_0.21_2024_11_12/spack-stage/spack-stage-hdf5-1.12.2-qdreo2z23uaq54fnrlsnfy273uk4seao/spack-src/src/H5VLcallback.c line 3952 in H5VL__file_close(): file close failed
    major: Virtual Object Layer
    minor: Unable to close file
  #005: /home/f_spsoft/.spack/v0.21.3_deploy_spack_0.21_2024_11_12/spack-stage/spack-stage-hdf5-1.12.2-qdreo2z23uaq54fnrlsnfy273uk4seao/spack-src/src/H5VLnative_file.c line 838 in H5VL__native_file_close(): can't close file
    major: File accessibility
    minor: Unable to decrement reference count
  #006: /home/f_spsoft/.spack/v0.21.3_deploy_spack_0.21_2024_11_12/spack-stage/spack-stage-hdf5-1.12.2-qdreo2z23uaq54fnrlsnfy273uk4seao/spack-src/src/H5Fint.c line 2341 in H5F__close(): can't close file, there are objects still open
    major: File accessibility
    minor: Unable to close file
Filters: "testing Node member destruction order"
Randomness seeded to: 2700569591
Failed ~ObjectHandle:
 ObjectHandle: could not close hid= 72057594037927937 type=FILE

[jkotan]

Thank you. It looks good for me.

[ggoneiESS]

Nothing to add to this PR, I was on holiday, but it's a bit annoying that we can't test it, is there any reason we don't implement something like https://github.com/ess-dmsc/h5cpp/blob/master/src/h5cpp/core/object_handle.cpp#L110-L112 so we can test without worrying about cerr? If not I'm happy to make the changes.

[FlyingSamson]

Nothing to add to this PR, I was on holiday, but it's a bit annoying that we can't test it, is there any reason we don't implement something like https://github.com/ess-dmsc/h5cpp/blob/master/src/h5cpp/core/object_handle.cpp#L110-L112 so we can test without worrying about cerr? If not I'm happy to make the changes.

The reason for the current implementation (as far as I as an project external developer can guess) is that you typically want destructors to be noexcept (suggested for instance in item 14 of Scott Meyers' Effective Modern C++) and throwing in there would obviously violate that quality.

[ggoneiESS]

Yes! And you did say this, apologies. Will blame it on coming in to 86 unread e-mails and writing a comment at the end of a long day..!

[FlyingSamson]

No worries :-)

[jkotan]

Thanks @FlyingSamson and @ggoneiESS for your clarifications.
BTW, can we have a similar issue for attributes i.e. where we have also link and handle objects ?

h5cpp/src/h5cpp/attribute/attribute.cpp

Lines 36 to 40 in b2669a3

	Attribute::Attribute(ObjectHandle &&handle,const node::Link &parent_link):
	handle_(std::move(handle)),
	parent_link_(parent_link)
	{
	}

[FlyingSamson]

Yes your are absolute right. Good catch! I will create another PR for that in a second.