Re: [boost] [Boost.Serialization] Help with PR fixing a memory leak in the current version

Subject: Re: [boost] [Boost.Serialization] Help with PR fixing a memory leak in the current version
From: Alexander Grund (alexander.grund_at_[hidden])
Date: 2018-06-26 08:39:03

Hi,
> I much appreciate your willingness to invest effort in this issue.  
> But my analysis and conclusions regarding the issue are totally 
> different. Motivated your efforts I added new tests for the singleton 
> and spent time enhancing tests for dll.  One test was just to 
> complicated for me to make work with b2 so I suppress it. Only one 
> test is failing on some platforms - test_export.  This is related to 
> DLLs.  I added windows + mingw and msvc++ to my machine just to try to 
> replicate the failure in my own environment.  I have been unable to do 
> so.
I do  have (Makefile) test, which does show the behaviour described. I'm 
pretty sure, if executed on another machine, it will fail too. I tried 
to add it to the b2-Testsuite but failed due to having static boost in 
dynamic libraries (which is the way I found where it crashes for sure). 
And there is 1 testcase currently in the Boost.Serialization testsuite 
that DOES fail and I'm pretty sure the issue described is the reason but 
of course I cannot really confirm this as the root cause is undefined 
behaviour (or ambiquities with shared libs)
> Late last year I tried to address this dll/export problem by merging 
> in a fix.  This fix entailed dynamically allocated a data structure in 
> the singleton. This fixed this failing test, but unbeknownst to me, 
> resulted in the memory leak.
This is what I tried to describe too in the issue and MR: The leak is 
simply due to no one freeing the dynamically allocated memory. This is 
even deterministic and easily reproducible. I think that oversight might 
be caused by you being so used to the code, that you overlook things 
that might be visible to fresh eyes. This is why I would like to also 
have some one else comment on the PR too.
> The way forward is to add some more tests which isolate the problem. 
> I'm aware that you've submitted one test which fails.  It seems like 
> its on the right track.  But (IIRC), it dynamically loads/unloads DLLS 
> containing user level serialization code in the same sequence.  I 
> think its reasonable to insist that if a user is going to do that that 
> such DLLS be unloaded in reverse order.  I admit that setting up these 
> sorts of tests is difficult and time consuming in any build 
> environment.  But that's where I am.
I can provide a test for b2 (the same as I have the Makefile for) if the 
boost test suite is build with `-fPIC`. This does not involve unloading 
DLLs or so but simply links an executable against 2 shared libraries 
which are linked against static boost. This crashes reproducibly. If 
anyone can tell me how to do the fPIC stuff for Boost.Build I'm happy to 
add this test to show that it fails with a crash in current code and 
that my approach fixes this.

In any case, I do have the feeling that the reason for the changes done 
in my PR (and the changes itself) are not fully understood. This is why 
I want to encourage question to the changes itself as I think those were 
not (only) meant to fix an issue in some circumstances but actually 
correct the code with test cases to prove this where possible (e.g. the 
is_destroyed flag). Other testcases just try to run into the problem I 
described.

Best, Alex

-- 
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Alexander Grund
Interdisziplinäre Anwendungsunterstützung und Koordination (IAK)
Technische Universität Dresden
Zentrum für Informationsdienste und Hochleistungsrechnen (ZIH)
Chemnitzer Str. 46b, Raum 010 01062 Dresden
Tel.: [phone]
E-Mail: alexander.grund_at_[hidden]
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~