Add tests for frontendbugs – Making copies of the Series object will result in strange behavior #804
Conversation
franzpoeschel
changed the title
test/SerialIOTest.cpp
Outdated
| { | ||
| openPMD::Series series( | ||
| "sample%T.json", openPMD::AccessType::CREATE ); | ||
| series_ptr = std::unique_ptr< openPMD::Series >( |
There was a problem hiding this comment.
just for modern C++, new-free style:
| series_ptr = std::unique_ptr< openPMD::Series >( | |
| series_ptr = std::make_unique< openPMD::Series >( series ); |
There was a problem hiding this comment.
Oo, I keep forgetting we're allowed to use C++14 now :)
There was a problem hiding this comment.
Oh, we did not officially switch yet - dang I always forget this is missing in C++11.
Soon - how far is PIConGPU in dev?
There was a problem hiding this comment.
Ah right, I confused both of them. PIConGPU is now running on C++14. So I'm keeping the raw constructors for now?
There was a problem hiding this comment.
Problem solved - you can now use C++14 #825 :)
There was a problem hiding this comment.
Nice, updated
| * series goes out of scope before series_ptr | ||
| * destructor should only do a flush after both of them go out of | ||
| * scope, but it will currently run directly after this comment | ||
| * since no iteration has been written yet, an error will be thrown |
There was a problem hiding this comment.
perfect description, thank you! ✨
looks like we should add reference counting or forbid the Series copy constructor...
Not sure how useful an alias for a Series in form of a handle object truly is. I would rather expect that if I copy a series that I can do things like bp to h5 conversion or so. We could implement such things via a .copy("new_name.ending") method later on that then returns a truly new Series object.
What do you think?
There was a problem hiding this comment.
I'm still liking the idea of having a clear separation of:
- Non-copyable shared internal resources
- Copyable handles that are exposed to user code and that have shared pointers to the internal resources.
This way, we could put the flushing destructor on the internal Series and things would be fine.
Currently, all of our objects just hide any member variable that need to be shared between instances behind shared pointers, which results in bugs like this.
Also, having actual internal classes for non-copyable resources would allow us to have proper upwards pointers through the openPMD hierarchies. Because, currently Writable::parent just goes to a copyable handle, and that concerns not only Series but all of our objects inheriting from Attributable. The issue is fortunately hard enough to trigger, but I should maybe try to find a way to make it fail for classes other than Series too...
Not sure how useful an alias for a Series in form of a handle object
All other classes that represent entities in the openPMD hierarchies are designed as handles, too, (sharing similar issues as argued above), so it's a question of consistency imo
There was a problem hiding this comment.
or forbid the Series copy constructor...
Until we get to it, this would likely be a sane thing to do. (This is what I temporarily did when writing the openPMD plugin for PIConGPU in order to avoid this problem). Breaking change tho
There was a problem hiding this comment.
I agree with your rationale above.
Yes, deleting the copy constructor would just be a quick-fix, because it is in fact broken and will likely catch bugs if someone used this on two alive instances of the same Series. For deleting this, we also need to define a Move constructor in Series and Attributable #790
franzpoeschel
force-pushed
the
topic-frontend-segfaults
branch
from
839e043
to
5473d6c
Compare
This is stolen from PR openPMD#804. This PR fixes the issues from that one, so those tests are passing now.
|
I've cherry-picked this test into #886 now. The tests are passing there. |
This is stolen from PR openPMD#804. This PR fixes the issues from that one, so those tests are passing now.
This is stolen from PR openPMD#804. This PR fixes the issues from that one, so those tests are passing now.
This is stolen from PR openPMD#804. This PR fixes the issues from that one, so those tests are passing now.
This is stolen from PR openPMD#804. This PR fixes the issues from that one, so those tests are passing now.
* Remove shortcuts from Attributable into Writable members Fix invasive tests * Split Attributable into internal and external class Make AttributableImpl destructor virtual Rename Attributable -> LegacyAttributable Use Attributable::retrieveSeries for Writable::flushSeries * Split Series into internal and external class Make BASEPATH a member of SeriesImpl Documentation for Series * Use an owning copy of Series in ReadIterations * Add tests for frontend maladies This is stolen from PR #804. This PR fixes the issues from that one, so those tests are passing now. * Alias AttributableImpl to Attributable * Make constructors of *Impl classes protected * Add documentation for the PIMPL pattern
This adds tests (currently failing) for the issues discussed in #534.