[All] auto-init/cleanup libraries

[maxime-tournier]

I am unsure this PR is correct, so it really needs a green light from others before merge.

When trying to call std::exit in some plugin, I had a segfault in cleanup code similar to this one:

// Detect missing cleanup() call.
static const struct CleanupCheck
{
    CleanupCheck() {
    }

    ~CleanupCheck() {
        if (core::isInitialized() && !core::isCleanedUp())
            helper::printLibraryNotCleanedUpWarning("SofaCore", "sofa::core::cleanup()"); // segfault
    }
} check;

In fact, most init.cpp files in SOFA have code similar to this one. In my case, MessageDispatcher::LoggerStream::~LoggerStream caused a call to MessageDispatcher::process which would then cause a segfault.

What I don't understand is why the above code snipped does not simply cleanup() the library in the destructor, since the latter is being called while the library unloads and this is the last chance to actually clean things up, instead of (trying to) emit a warning while the whole program is being terminated.

So unless there is a compelling reason to leave it this way (and I would really like to know it), I suggest the above to be changed to:

static const struct CleanupCheck
{
    CleanupCheck() {
        init();
    }

    ~CleanupCheck() {
        cleanup();
        // leaving the warning if for some reason cleanup failed
        if (core::isInitialized() && !core::isCleanedUp())
            helper::printLibraryNotCleanedUpWarning("SofaCore", "sofa::core::cleanup()");
    }
} check;

so that the library automatically init() and cleanup() upon dlopen/exit.

This PR:

• [ ] builds with SUCCESS for all platforms on the CI.
• [ ] does not generate new warnings nor unit test failures.
• [ ] does not break existing scenes.
• [ ] does not break API compatibility.
• [ ] has been reviewed and agreed to be transitional.
• [ ] is more than 1 week old. Reviewers will merge only if all this checks are true.

[sofabot]

Thank you for your pull request! Someone will soon check it and start the builds.

[guparan]

I think this PR is related to https://www.sofa-framework.org/community/forum/topic/end-simulation-from-a-python-script-controller/

[ci-build]

[guparan]

Ahem, let's retry to build... [ci-build]

[maxime-tournier]

@guparan had'nt seen the reply on the forum, that's exactly my use case.

[damienmarchal]

[ci-build]

[hugtalbot]

Just tested it, fix indeed the issue

[matthieu-nesme]

Who was calling 'init()' previously? Is it called two times now?

[maxime-tournier]

There is a static bool flag preventing double init/cleanup, so we should be fine. And apparently it had to be called manually.

[damienmarchal]

@hugtalbot unless it is a very important fix, please don't forget to wait a 1 week quarantaine before merging to give an oportunity to anyone to give feedback :)

[matthieu-nesme]

I am a bit afraid about call order. I guess the order of initializations is important, but RAII does not guarantee anything.

[maxime-tournier]

@matthieu-nesme not quite sure what you mean: RAII ensures that the library is initialized as soon as it is dlopen-ed, where does the order of initialization come into the picture ?

[matthieu-nesme]

In that case, why bother with manual init/cleanup? Where is it done? If manual init/cleanup are removed, s_initialized/s_cleanedUp become obsolete.

What does guarantee that libs are loaded in the right order? Maybe it is the case, but I am not convinced. (e.g. I guess SofaSimulationCommon must be initialized before SofaSimulationTree).

[maxime-tournier]

In that case, why bother with manual init/cleanup? Where is it done?

This is precisely why I asked for feedback before merging, because I have no idea. I don't quite know why it was not automatic in the first place since this is all c++98.

If manual init/cleanup are removed, s_initialized/s_cleanedUp become obsolete.

agreed, but until we know exactly what's going on we don't want to get rid of it.

What does guarantee that libs are loaded in the right order?

nothing, but libraries could probably load their dependencies upon init (+ simple cycle detection strategy) to have it all automatic?

[damienmarchal]

@maxime-tournier, Sorry for that but this PR have been merged a bit too fast, our mistake. We probably have to revert it to give more time to the discussion.

[maxime-tournier]

@damienmarchal no problem as far as I'm concerned, but that would probably be safer to revert if possible until we hear more from others.

[marc-legendre]

Hi everyone, Marc here :-)

I remember I introduced the init() and cleanup() functions, so I'll add a few words of explanations. Bear with me, it's actually simpler that the length of my response may suggest.

So unless there is a compelling reason to leave it this way (and I would really like to know it)

The trick which consists in using static variables to run dynamic initialization code (or cleanup code, for that matter) is an anti-pattern, for a couple of reasons:

• determinism: as @matthieu-nesme mentionned, the order of initialization of objects with static storage duration is undefined across translation units. This is the first reason why I introduced the init() functions: to make initialization deterministic and reliable. And this is the same reason why it's better to call those functions explicitely, at the right time: the program will work correctly by design, not by coincidence;

• readability (or something like that): calling the initialization function "manually" makes programs more readable: you can just start from the main() function and understand what code is going to run without actually running the code inside a debugger (or reading the entirety of SOFA's source code);

• choice (or whatever): with an explicit call to init(), an application writer gets to decide both whether and when to initialize SOFA.

Now...

In the end, I didn't actually bother to search for any single piece of initialization code in SOFA and move it into the appropriate init() function, so the init() functions are pretty light, but the setup is there.

Also, the static-dynamic-initialization trick is used all over the place in SOFA, and what happens in the init() functions is ridiculous compared to all the code that probably runs when SOFA is loaded, so here again it is a drop in the ocean, but, I think, a step in the right direction.

Anyway...

What I don't understand is why the above code snipped does not simply cleanup() the library in the destructor, since the latter is being called while the library unloads and this is the last chance to actually clean things up, instead of (trying to) emit a warning while the whole program is being terminated.

@maxime-tournier The CleanupCheck thingy is disposable; it's a just safeguard, a way to tell developpers about the init() and cleanup() function. You're right, this is almost the last chance to call cleanup(), but no guarantee of correctness.

Hope this helps, Marc Legendre

--

trying to call std::exit in some plugin

Sweating

[maxime-tournier]

Hello Marc, and thanks a lot for your feedback !

I'll address some of your points below:

determinism: as @matthieu-nesme mentionned, the order of initialization of objects with static storage duration is undefined across translation units. This is the first reason why I introduced the init() functions: to make initialization deterministic and reliable. And this is the same reason why it's better to call those functions explicitely, at the right time: the program will work correctly by design, not by coincidence;

To be more explicit: there is no guarantee whatsoever that calling init in a static RAII constructor will get called after every other static variable in the shared library is initialized. This can indeed be an issue.

I was under the impression that there is exactly one of such RAII per dynamic library loaded by SOFA, so as long as init does not involve messing around with other static variables in the library we should be safe, right?

readability (or something like that): calling the initialization function "manually" makes programs more readable: you can just start from the main() function and understand what code is going to run without actually running the code inside a debugger (or reading the entirety of SOFA's source code);

Come on, we're talking about SOFA here ;-)

choice (or whatever): with an explicit call to init(), an application writer gets to decide both whether and when to initialize SOFA.

Not sure I agree: when dlopen-ing a shared library, I prefer to have it initialized automatically if possible (and safe) as the converse is error-prone.

In any case, cleanup should really be called in the RAII destructor, otherwise resources will leak in case an exception is thrown and not caught, or somebody calls std::exit (which they can). And of course, the destructor should not do silly stuff like calling a MessageDispatcher that was destructed already.

[matthieu-nesme]

@maxime-tournier did convince me now!

As long as the init of a lib is calling the init of its dependencies the init order should be ok (with the safeguard "s_initialized" not to init several times).

But the cleanup order cannot be guarantee, but is it a big deal? For what I can see in the public code, these init/cleanup function do nothing yet. The doc can precise than "cleanup" must clean the actual lib, w/o depending on any other lib, and then the actual cleanup functions should not call cleanup from their dependencies.

Also "helper::printLibraryNotCleanedUpWarning" was creating a big mess, because using the message logger while a lot of things were already destroyed could easily creating a segfault. Anyway it would not be useful anymore and could be removed too.

• "CleanupCheck" RAII should be renamed
• s_cleanedUp could be removed
• all the call to init/cleanup could be removed from app/tests
• helper::printLibraryNotCleanedUpWarning should be removed

[maxime-tournier]

I've had another look at the code, and I've found:

• All init functions are trivial, except helper::init which calls Console::init() which does involve other static variables in Console
• All init functions happen in dlls which are linked in the runSofa binary (so they are not dlopen-ed on runtime as I thought they were)

Based on this, my suggestion is to get rid of all init/cleanup functions, and put console data behind a 'nifty counter' to make sure static initialization and finalization is done correctly and automatically.

[marc-legendre]

Fair enough :-)

[maxime-tournier]

It's actually way more complex: while trying to do the above, I got bitten hard by the linker.

Here is what I found:

• runSofa is currently calling initComponentGeneral() which calls in turn initOpenGLVisual(),
• now if I remove calls to initComponentGeneral from runSofa, no symbol from libComponentGeneral.so is ever referenced in runSofa and the linker does its job so libComponentGeneral.so is completely linked out from runSofa,
• this means no static variable from libComponentGeneral.so is ever constructed nor destructed while running runSofa,
• so none of the static class registration through RegisterObject is ever performed!

In my case, the above caused the following problem:

• when constructing the viewer, an alias from VisualModel to OglModel is added, which fails since OglModel was never registered at this point,
• (bonus) when loading some other plugin later on (in this case, Compliant), the plugin might reference code from libComponentGeneral, which would thus be linked in the plugin, triggering DLL initialization and class registration for OglModel when the plugin is loaded

*TL;DR: manual `initcalls act as a (crude) way of forcing symbols to be pulled from each dll into the main executable, ensuring class registration is performed correctly beforemain` is executed.**

Based on the above, a reasonable fix would be:

• remove all CleanupCheck as they may cause issues
• remove all cleanup-related functions
• keep all init calls everywhere
• any non-trivial static initialization and especially finalization should use a nifty counter

[matthieu-nesme]

But is there something wrong about using RAII to perform the init automatically?

[maxime-tournier]

Nothing wrong as long as the main executable references at least one symbol from each of the dlls everything should be fine.

Really the right way to do it would be to have all library initialization behind a nifty counter like this:

initComponentGeneral.h:

static struct SOFA_COMPONENT_GENERAL_API SofaComponentGeneral {
    SofaComponentGeneral();
    ~SofaComponentGeneral();
} SofaComponentGeneral;

initComponentGeneral.cpp:

static int nifty;

SofaComponentGeneral::SofaComponentGeneral() {
    if(nifty++ == 0) {
        // init goes here
        std::clog << "SofaComponentGeneral init" << std::endl;
    }
}

SofaComponentGeneral::~SofaComponentGeneral() {
    if(--nifty == 0) {
        // cleanup goes here
        std::clog << "SofaComponentGeneral cleanup" << std::endl; 
    }
}

It has roughly the same code size compared to existing init/isInitialized/cleanup/isCleanedUp, is fully automatic as long as the header is included from the main executable, and initializes/finalizes safely.

[matthieu-nesme]

+1 yapuka ;)

[marc-legendre]

Oh, "my" init functions are only the init() and cleanup() functions in the helper, defaulttype, core, and simulation* libraries.

The other are way older, and I think someone explained to me that, indeed, they were needed to ensure that linking is done appropriately on all platforms.

[matthieu-nesme]

[offtopic]ca fait plasir de te voir trainer par ici Marc ;)[/offtopic]