eclipse-iceoryx / iceoryx

Eclipse iceoryx™ - true zero-copy inter-process-communication
https://iceoryx.io
Apache License 2.0
1.57k stars 373 forks source link

Global Instantiation of Publisher/Subscriber created core dump #327

Closed surendra210 closed 3 years ago

surendra210 commented 3 years ago

Required information

Operating system: Ubuntu 18.04

Compiler version: GCC 7.4.0

Observed result or behavior: Process with global Publisher/Subscriber exits with core dump.

Expected result or behavior: No core dump for global Publisher/Subscriber based communication.

Conditions where it occurred / Performed steps:

Step1: Create a global instance of a wrapper class which calls iox::runtime::PoshRuntime::getInstance("/ProcessUniqueString") in its constructor. Step2: Create a global Publisher/Subscriber Instances, to communicate over a topic. Step3: Compile code and run RouDi and start Publisher/Subscriber. Step4: You will observe core dump with SIGABT.

NOTE: We were able to get rid of this core dump issue by replacing PoshRuntime::s_runtimeFactory() with PoshRuntime::defaultRuntimeFactory() inside PoshRuntime::getInstance() in filename posh_runtime.cpp

We suspect PoshRuntime::s_runtimeFactory() has not been defined by the time PoshRuntime::getInstance() is called by global instance of the wrapper class

budrus commented 3 years ago

Thanks for reporting @surendra210

Some questions:

Our current contract is that first a runtime must be created an then you are allowed to create publishers and subscribers. We do some singleton magic behind the scenes. With that approach you don't have to provide the runtime when creating publishers and subscribers but you have to fulfil this contract.

surendra210 commented 3 years ago

In your wrapper class, do you also pass a name to the iox::runtime::PoshRuntime::getInstance() call? With the first call a name for your process must be provided and this call must also be made before creating publishers and subscribers << Sorry for the confusion with my description, we do send a unique string something like iox::runtime::PoshRuntime::getInstance("/ProcessUniqueString"), we do ensure that getInstance is called before pub/sub, by creating the Global instances in order of WrapperClass followed by pub/sub.

Note: The main essence of the wrapper class itself is to do the getInstance before pub/sub.

This are all global objects, or? So we have some kind of static initialization order fiasco? << No, we are not using static objects.

I am attaching example code that can regenerate the issue.

surendra210 commented 3 years ago

ExampleCode.zip

elfenpiff commented 3 years ago

@surendra210 You discovered a bug on our side. We have the static initialization order fiasco problem here: https://isocpp.org/wiki/faq/ctors#static-init-order

But you can circumvent it easily for now. In the example you provide us you declared in line 32 and line 34 two global variables getRoudi and myPublisher. If you would create instead two unique_ptr of that type there and initialize them in the main function you are good to go.

std::unique_ptr<instanceIdentifier::callRoudi> getRoudi;

std::unique_ptr<iox::popo::Publisher> myPublisher;

//....

int main() {
     getRoudi = std::make_unique<instanceIdentifier::callRoudi>("/Ipcf-gw-m7-ice-delivery");
     myPublisher = std::make_unique<iox::popo::Publisher>({"Radar", "FrontLeft", "Counter"});
}

Another option would be using std::optional and initialize it with emplace in main.

Now the deep dive:

budrus commented 3 years ago

I see, there is another indirection with a static which you avoid with the workaround. I think @elBoberido made this and also had a reason for. The way it is currently is then a problem for your setup.

You want to have the publishers and subscribers global and that's why you have to make the runtime global too? And this alone would be fine as all are in a single compilation unit? Is it really needed to have these objects global? Do you want to access them from different threads or what is the use case?

@elBoberido do you think we can support this? I have the feeling you did tricky things

elBoberido commented 3 years ago

@budrus I've already talked with @elfenpiff and we found a viable solution. The reason for the indirection was the RouDiEnvironment, so that we are able to have multiple runtimes in one process

surendra210 commented 3 years ago

@budrus Thanks a lot for the analysis done and suggestions provided, we will look into them. Now coming to your question

You want to have the publishers and subscribers global and that's why you have to make the runtime global too? And this alone would be fine as all are in a single compilation unit? << YES ,for the reason regarding runtime global.

Is it really needed to have these objects global? Do you want to access them from different threads or what is the use case? << Based on a model, the pub/sub instances are generated, which are ideally in global namespace/scope. And also the reason you mentioned regarding multiple thread access could be a potential usecase as well.

elBoberido commented 3 years ago

@surendra210 the more I think about the problem, the more I come to the conclusion that it might be better to take another approach

Is there any reason to not call iox::runtime::PoshRuntime::getInstance("/Ipcf-gw-m7-ice-delivery"); in main()?

For the publisher you could use static init on first use

iox::popo::Publisher& myPublisher() {
    static iox::popo::Publisher publisher({"Radar", "FrontLeft", "Counter"});
    return publisher;
}

void sending() {
    myPublisher().offer();
}

If your global variables will be in different translation units later on, you will have the problem that the publisher might outlive the runtime. https://exceptionshub.com/destruction-order-of-static-objects-in-c.html If you call getInstance() in main() and use the static on first use idiom, you should be safe

FerdinandSpitzschnueffler commented 3 years ago

I will create a fix for the static initialization order fiasco problem