Fix memory leak when there are Joined Arrays in streaming mode #3609
Conversation
| @@ -174,6 +174,7 @@ class BP5Deserializer : virtual public BP5Base | |||
| std::vector<void *> *m_FreeableMBA = nullptr; | |||
|
|
|||
| std::vector<void *> *m_JoinedDimenOffsetArrays = nullptr; | |||
| std::vector<void *> *m_FreeableJDOA = nullptr; | |||
There was a problem hiding this comment.
This attribute needs to the last one in order not to break the ABI compatibility
There was a problem hiding this comment.
Hmm. That makes sense of course, but separating it from the value it's associated with seems like a step down the road to harder-to-understand code. In this circumstance, would it make sense to put this version in master and merge a version to release_29 with it relocated to the end?
There was a problem hiding this comment.
would it make sense to put this version in master and merge a version to release_29 with it relocated to the end?
Yes, that is a good idea.
There was a problem hiding this comment.
Actually, I think this isn't an ABI issue. BP5Deserializer is a completely ADIOS-internal class. You'd only get an ABI issue if this was an externally-facing class and we were changing its public members, right? Here, if the user ends up pointing to a new version of the ADIOS library he'll end up new code that references this class as well as the new class itself. Or is there something wrong with my reasoning?
There was a problem hiding this comment.
Actually, I think this isn't an ABI issue. BP5Deserializer is a completely ADIOS-internal class. You'd only get an ABI issue if this was an externally-facing class and we were changing its public members, right? Here, if the user ends up pointing to a new version of the ADIOS library he'll end up new code that references this class as well as the new class itself. Or is there something wrong with my reasoning?
You are correct, or should be as you say, however, the problem is that BP5Deserializer.h is installed when ADIOS2 is installed and it would be consider part of the exported classes this can lead to:
- False positives errors when using automated tools for ABI compatibility.
- A Unlikely scenario in which users use this more core classes directly, which is possible as they are installed to them.
Note that I am non-expert on this regard, I studied about this years ago and I am refreshing my knowledge about this.
|
Can this be merged? |
|
IMHO, yes. @vicentebolea , did some form of this get merged to master? Kind of remember seeing that go by, but I've lost track. |
Yes this was added in #3602. There is a PR fixing this for release_29 too, I am waiting to group together a bunch of bugfixes to backport to the release branch |
|
@vicentebolea Just looking back at open PRs. Can this be closed/deleted now? I wasn't quite clear based on your May 26 comment, but it sounds like it's in master and that some other PR was putting it in 2.9. |
|
@eisenhauer looking deeper into this it appears that what got merged into master/release_29 was a different bugfix, here is: #3619. What do you think? |
Agreed, that was a different fix. That fix was for the return-a-structure on compilers that don't support copy elision. This is a different bug and needs to be in 2.9 and master. Do you want to batch this in some way? Or should we update this with current release_29 base and merge? |
Yes |
|
Can you merge to master, I will backported to release. |
|
OK, closing this in favor of a new PR targeted at master. |
Closes #3608