This code isn't secure:

1. We don't synchronize access to amqpChannel, but use it from multiple threads.

The AMQP-CPP documentation says:

AMQP-CPP is fully asynchronous and does not do any blocking (system) calls, so it can be used in high performance applications without the need for threads.

Regarding the channels:

A channel is a sort of virtual connection, and it is possible to create many channels that all use the same connection...
All operations that you can perform on a channel are non-blocking....

From the above, I don't think we need to synchronize access to the amqpChannel.

2. init method can be called multiple times and that would result in multiple threads being spawned.

Actually no. The check in the condition of

if (this->amqpChannel != nullptr)

handles this. When init will call multiple times on the already initialized connection it will be skipped because amqpChannel can be null only when it is not initialized. It was tested in D4749 when we run unit tests init calls in every test, but the initialization is skipped as expected here.

First of all, what is the reason behind introducing init method? Can't we just perform this initialization in the constructor? This should be our default strategy and we would need a really good reason to use a different approach.

According to the description of this diff (this is a follow-up for the current using approach):

Creating init() function following the existing approach that AWS-CPP client already uses. We are callinginit() to initialize AWS thread/client.

1. We don't synchronize access to amqpChannel, but use it from multiple threads.

I've added D4767 (synchronization) along with the stress test in D4768 (testing) to test in 100 threads.

It doesn't relate to this certain diff which is about the wrapping of spawning of the amqp client thread into the init() instead of repeating it in the codebase, but anyway thanks for an improvement catch!

Thanks for pointing out @tomek. Btw, what do you think about detach?

I'm just pasting my response from the other diff: https://phab.comm.dev/D4768#137425. I still think we should avoid using detach as much as possible. Quoting SO:

So, should you use join or detach?

• Use join
• Unless you need to have more flexibility AND are willing to provide a synchronization mechanism to wait for the thread completion on your own, in which case you may use detach

I'm sure we should use detach() in such initialization functions. The reason is that the initialization can be called from the thread which just can initialize and be gone instead of making it join and make alive the parent caller thread. I agree in some cases forward in the stack that detach() may not be necessary, but in the case when we are calling initialization the client thread which must live until the program exit - we can/must use detach().

A question for @max: Do you provide a synchronization mechanism to wait for the thread completion on your own here?

The AMQP client thread should be completed along with the main program and spawned only once. Looking forward the init() calls AmqpManager::connect() which connects and reconnects to the AMQP server in a loop and throws a fatal if the connection attempts are exhausted.

I'm sure we should use detach() in such initialization functions.

I see. Is it based on something?

I've described my thoughts on why initialization functions should be detached above:

I'm sure we should use detach() in such initialization functions. The reason is that the initialization can be called from the thread which just can initialize and be gone instead of making it join and make alive the parent caller thread. I agree in some cases forward in the stack that detach() may not be necessary, but in the case when we are calling initialization the client thread which must live until the program exit - we can/must use detach().

Interesting. What if an error occurs during the initialization? We'll not see it and the amqp will not be running. So we'll be pretty much in a state like "I have no clue what's going on".

If the unrecoverable error occurs during the client thread initialization we will throw an unrecoverable error with the message what's happened and exit the main program. In case of the error is recoverable (in our scenario in this case we need to reconnect) the thread needs to be run with the reconnection loop. To handle it properly we must clearly distinguish which errors are recoverable and handled properly inside the thread and which are unrecoverable and we should exit.

In our certain case if the error occurs during the initialization is a connection error only that handles properly with the waiting loop and amqpReady assignment in D4742.

I'm sure that detaching the thread whose purpose is to run all the time which the main program runs is the correct way because we don't know where init() will be invoked, from a long-lived thread or not. In case we don't use detach and call init() from the thread which will die earlier than the main we will get a bug, segfaults etc. So that's why detaching here is necessary for my thoughts.

I've described my thoughts on why initialization functions should be detached above:

My question was more if there are any external resources to support this idea, we already know that in your opinion it is ok, you may be wrong though. It's sometimes good to go through the Internet and see what other people think about this.

Of course if you google something there is a pro and cons materials and we should consider of using or not something in a certain scenarios instead of just throw it out and create a possibility of another bug. There is a scenario where we can decide which way to go and there is no certain wrong way. Both ways have its pros and cons. That's why we listening each other and make a decisions as a team. Because of the code must be maintained not by the only person in the team.

When the discussion became a time consumer we just vote for the best solution and that's it. This can take a huge amount of time to discuss which can be spent better I think.
I respect your concerns and thoughts but here is a two ways and how the @tomek will vote I'll just go with it. So let's the third voice decide )

Detaching should be used carefully - yes, correct, but we shouldn't throw it when it needs to be used. Detaching a thread is a common practice BUT it should be used carefully. That's right.
Quick example, we are heavily using AWS-CPP client, it's detaching threads in an executor because the developer don't know from where the init() will be called. The same is in our scenario. We must be sure that the client thread will run until the main exit.

In our certain case if the error occurs during the initialization is a connection error only that handles properly with the waiting loop and amqpReady assignment in D4742.

Even if this works that way, I don't think detach's good. It's not very maintainable, if we ever change the connect logic, we'll have to remember to reconsider using detach. I don't think that's safe. It may result in a bug that may be hard to find.

Using initialization in a no long-lived thread otherwise resulting in a bug. If we simplify all of this there are two ways:

• We are using a detached thread for the initialization and we don't need to keep in mind that we should run the init() only from the long-lived thread or main (as an AWS SDK do), because of the amqp-client thread should live all the time that app lives.
• We are not using a detaching, but we should keep in mind that initialization must be done only in a long-lived thread or we will have a bug where the parent thread exits along with the children ampq-client thread and we will have possible segfaults.

If the unrecoverable error occurs during the client thread initialization we will throw an unrecoverable error with the message what's happened and exit the main program.

I don't understand how this would work, sorry. From what I understand, you're only relying here on the aqmpReady flag. So if it's not set, then you'll throw an error in a different place. But I believe that if the throw happens inside of the detached thread, we'll not be able to catch it. So, essentially, we'll only have that flag, nothing more. This may make debugging a lot harder.

To support what I said, I'm going to provide two examples, let's have a look:

• The first one (with join)

#include <iostream>

#include <thread>
#include <chrono>

using namespace std;

int main()
{
  cout << "MAIN BEGIN" << endl;
  thread th([](){
    cout << "TH BEGIN" << endl;
    std::this_thread::sleep_for(std::chrono::milliseconds(500));
    throw std::runtime_error("err");
    cout << "TH END" << endl;
  });
  std::this_thread::sleep_for(std::chrono::milliseconds(100));
  cout << "MAIN END" << endl;
  th.join();
}

The output is

MAIN BEGIN
TH BEGIN
MAIN END
libc++abi: terminating with uncaught exception of type std::runtime_error: err
./build_and_run.sh: line 15: 71224 Abort trap: 6           ./cmake/bin/main

• Now, the second one (with detach)

#include <iostream>

#include <thread>
#include <chrono>

using namespace std;

int main()
{
  cout << "MAIN BEGIN" << endl;
  thread th([](){
    cout << "TH BEGIN" << endl;
    std::this_thread::sleep_for(std::chrono::milliseconds(500));
    throw std::runtime_error("err");
    cout << "TH END" << endl;
  });
  th.detach();
  std::this_thread::sleep_for(std::chrono::milliseconds(100));
  cout << "MAIN END" << endl;
}

The output is

MAIN BEGIN
TH BEGIN
MAIN END

Conclusion: we were not able to catch the error with the detached thread, but with the joined one, we could do that.

To illustrate what you're doing I prepared yet one more example:

#include <iostream>

#include <thread>
#include <chrono>

using namespace std;

int main()
{
  bool ready = false;
  cout << "MAIN BEGIN" << endl;
  thread th([&ready](){
    cout << "TH BEGIN" << endl;
    std::this_thread::sleep_for(std::chrono::milliseconds(500));
    throw std::runtime_error("err");
    ready = true;
    cout << "TH END" << endl;
  });
  th.detach();
  std::this_thread::sleep_for(std::chrono::milliseconds(100));
  // th.join();
  if (!ready) {
    throw std::runtime_error("not ready");
  }
  std::cout << "ready, all good" << std::endl;
  cout << "MAIN END" << endl;
}

There are two options:

• you can detach - you will just see that the flag is not set, so you will not know anything about the actual error that's occurred,

You would, because of the errors are logging inside the connect functions. Quick example how it works now: The tunnelbroker app starts, the flag is not set, the errors prints to the logs in case of the connection error, not established and etc. It's not silent as you think. We should use logging in a multi threaded app instead of to be scarry that the detached thread will not produce any logs. This scenario is for a single thread app. When we are creating app with multiple threads which are can be different (long lived, short lived) we should think of the proper logging instead of not using detach in any case. As of inter thread communication, race conditions etc.

• you can join just before checking for the ready flag - we are able to see the real error.

That's not true. you will see the real error because of the logging inside of the detached thread.
In your example code the log from the detached thread is not produced because of the main thread exit, not because of detached/joined thread.

One more concern: What if the initializing thread takes a long time (for some reason) and we check for the ready flag before the initialization terminates? In this case, we're going to throw an error even though the initialization would succeed (there might be one millisecond of a difference). I'm not going to post yet one more code snippet for this, you can use the ones above just removing this line throw std::runtime_error("err");.

It's not because of we are checking the flag with the reconnection time intervals in waitUntilReady() function in D4741. We will throw an fatal error only in case when the reconnection attempts are exceeded and we print a log errors on reconnect attempt, and other errors. We have callback with logging on onError in D4744 -> AmqpManager.cpp:51.