openPMD / openPMD/openPMD-api

Design limitations in the frontend resulting in unexpected behaviours (segfaults, premature destructor calls)

Open
#534 8 comments 3 reactions 0 assignees View on GitHub
bug frontend: C++17 frontend: Python3 help wanted internal
Dominant language
C++
Stars
161
Forks
59
Avg merge
2d 22h
Merged PRs (30d)
7

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`](https://github.com/openPMD/openPMD-api/blob/dev/include/openPMD/backend/Container.hpp) 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`](https://github.com/openPMD/openPMD-api/blob/c8f40a0cac359c441317d9bd9e80efb8ac12d6a7/include/openPMD/Series.hpp#L238) or [`Iteration`](https://github.com/openPMD/openPMD-api/blob/c8f40a0cac359c441317d9bd9e80efb8ac12d6a7/include/openPMD/Iteration.hpp#L89)) or directly behind a `shared_ptr` (e.g. [in `Attributable`](https://github.com/openPMD/openPMD-api/blob/c8f40a0cac359c441317d9bd9e80efb8ac12d6a7/include/openPMD/backend/Attributable.hpp#L203)).

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](https://github.com/openPMD/openPMD-api/blob/c8f40a0cac359c441317d9bd9e80efb8ac12d6a7/src/Series.cpp#L120).

**(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](https://github.com/openPMD/openPMD-api/blob/c8f40a0cac359c441317d9bd9e80efb8ac12d6a7/src/backend/Attributable.cpp#L283) (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](https://github.com/openPMD/openPMD-api/blob/c8f40a0cac359c441317d9bd9e80efb8ac12d6a7/src/Iteration.cpp#L219) 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:
```cpp
{
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.
```cpp
{
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](https://github.com/openPMD/openPMD-api/blob/c8f40a0cac359c441317d9bd9e80efb8ac12d6a7/src/Iteration.cpp#L92) 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.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.