On 6/26/2018 3:39 AM, Alexander Grund via Boost wrote:
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
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
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
Hi, 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) that might be visible to fresh eyes. This is why I would like to also have some one else comment on the PR too. 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.
I confirm that the 1.66 version never ever invokes the singleton destructor declared and defined in boost/serialization/singleton.hpp, approximately line 175. It does not matter whether it is a debug or release build, 32-bit, or 64-bit Microsoft or Intel C++ on Windows. A DLL wrapper around Boost.Serialization is used at run time. The actual Boost.Serialization library is statically linked to the DLL. The roughly 20 line main function, test executable then uses the DLL to invoke the serialization. I verify the behavior using an old school "printf" approach, with std::cout messages and static ptrdiff_t counting variables inside the get_instance() and ~singleton() methods. I observe eleven independent instances of the singleton class, with ZERO calls to the singleton destructor. The eleven singletons are created with ONE instance of the serialization object, regardless of invoking the serialize method. The main function: int main() { { HINSTANCE hGetProcIDDLL = LoadLibrary(L"BoostSerializeDLL.dll"); } /**/ std::cout << "Beginning unit test..."; std::shared_ptr<thislibrary::myclass> myc = thislibrary::create_myclass(); { std::shared_ptr<std::ofstream> ostream = std::make_shared<std::ofstream>(); ostream->open("C:\\temp\\leak_test.xml"); { boost::archive::xml_oarchive xml(*ostream); myc->serialize(xml, 1); // comment this line out and the leak remains. } ostream->close(); } /* */ return 0; } Fundamentally, there is something between 300 to 400 bytes memory leak. Yes, it is not much with today's world of GB RAM. However, when you have production code that leaks, then ANY memory leak is unacceptable. Essentially, the leak is due to the lack of the eleven singleton instances NOT invoking the respective destructor. Plus, the group I am part of spends tens of hours tracking down why the production product has memory leaks. The final convincing point that must be clearly understood is this: due to the leak, Boost.Serialization has been replaced. If that does not convince a reader that there is a real problem, then nothing will. Sincerely, Robert
Best, Alex