Design limitations in the frontend resulting in unexpected behaviours (segfaults, premature destructor calls)
#534 opened on Jul 22, 2019
Repository metrics
- Stars
- (160 stars)
- PR merge metrics
- (PR metrics pending)
Description
After following around a segmentation fault when using the openPMD API, I came to the conclusion that there is a relatively deeply-nested issue with the design of the frontend's classes.
Most of those classes have been designed as handles to some underlying data so that users can take a lightweight copy (instead of a reference or pointer) and still interact with the same shared data.
Key to this is the class Container that wraps a shared_ptr to an internal container (e.g. std::map). Most classes exposed in the public API store their payload behind a Container (e.g. in Series or Iteration) or directly behind a shared_ptr (e.g. in Attributable).
The issue arises because atm there is no sufficiently clear separation between (1) functionality of copyable handles and (2) functionality that should be restricted to the unique non-copy internal data. So far, I have found two issues falling in this category:
(1) Destructor calls
This issue has limited extent, fortunately, since most classes use a default destructor. But also the root class of the openPMD tree Series has been designed as a handle, so its destructor should not really perform a flush.
(2) Going upward in the openPMD hierarchy
More problematic is the fact that the openPMD tree is not a tree of unique objects but a tree of the mentioned handles. The tree is linked (in upward direction) using the classes Writable and Attributable, using raw pointers in order to break reference cycles. (More idiomatic would be weak pointers, but this is not the issue here). This linkage is later used again to go upward in the openPMD hierarchy.
When doing this, in principle the handle that has been stored as a parent can already be invalidated while the underlying data is still present.
Fortunately enough, it is currently difficult to trigger this behavior. The following rather constructed examples show the issue:
{
std::unique_ptr< openPMD::Series > series_ptr;
{
openPMD::Series series( "sample%T.json", openPMD::AccessType::CREATE );
series_ptr = std::unique_ptr< openPMD::Series >(
new openPMD::Series( series ) );
// i am the marked line
}
series_ptr->iterations[0].meshes["E"]["x"].makeEmpty< int >( 1 );
}
Destructor runs at the marked line. No iteration is present yet, so flushing in file-based mode throws an exception. It seems surprising that the series is flushed.
{
std::unique_ptr< openPMD::Series > series_ptr;
{
openPMD::Series series( "sample%T.json", openPMD::AccessType::CREATE );
series_ptr = std::unique_ptr< openPMD::Series >(
new openPMD::Series( series ) );
series_ptr->iterations[0].meshes["E"]["x"].makeEmpty< int >( 1 );
}
// i am the marked line
}
We define the data earlier, so the first flush passes. Since the original handle has been destroyed, the second flush at the marked line fails with a segmentation fault.
The issue is not currently critical since triggering it requires writing rather esoteric programs, but for my current implementation of streaming this is becoming increasingly difficult to handle since re-parsing the openPMD hierarchy is necessary and I need to pay attention to not link to any copied handles but the original one instead.
I can currently work around the problem, but these issues should be considered in an upcoming refactoring of openPMD's frontend.