1

I got a class which holds the application's logger as a unique_ptr. The logger can be set through a static function. Also, one can obviously log messages. I left out any thread synchronization (mutexes) to make things easier.

class LoggerWrapper {
 public:
  static void SetLogger(std::unique_ptr<logger::ILogger> new_logger);

  static void Log(const std::string& message);

 private:
  static std::unique_ptr<logger::ILogger> logger_;
};
void LoggerWrapper::Log(const std::string& message) {
  if (!logger_) {
    // cannot log
  } else {
    logger_->OnLogEvent(message);
  }
}

void LoggerWrapper::SetLogger(std::unique_ptr<logger::ILogger> new_logger) {
  logger_ = std::move(new_logger);
}

My problem is: The unique_ptr gets destructed before some of the other classes inside the application. E.g. is the DTOR of Class Foo wants to log something, the unique_ptr might have already been destroyed (which is the case at the moment). This causes the ILogger implementation to be destroyed, resulting in no log output being possible.

Does anyone have an idea on how to easily fix this? I somehow need to "delay" the destruction of the static unique_ptr. I also tried changing it to a shared_ptr, but that only caused SIGABRTs with "pure virtual method called" errors.

Thanks in advance!

EDIT: Created a minimal working example which contradicts my experience. In this case, the static logger outlives the Foo class.

EDIT2: My application uses exit. That seems to change the order of destruction.

EDIT3: exit does not destroy local objects.

/******************************************************************************

                              Online C++ Compiler.
               Code, Compile, Run and Debug C++ program online.
Write your code in this editor and press "Run" button to compile and execute it.

*******************************************************************************/

#include <iostream>
#include <memory>
#include <string>

using namespace std;

class ILogger {
  public:
  ILogger() {
      std::cout << "ILogger CTOR" << std::endl;
  }
  ~ILogger() {
      std::cout << "ILogger DTOR" << std::endl;
  }
  
  virtual void OnLogEvent(const std::string& log_message) {
        std::cout << "OnLogEvent: " << log_message << std::endl;
  }
};

class LoggerWrapper {
 public:
  static void SetLogger(std::unique_ptr<ILogger> new_logger) {
  logger_ = std::move(new_logger);
}

  static void Log(const std::string& message) {
  if (!logger_) {
    // cannot log
  } else {
    logger_->OnLogEvent(message);
  }
};

 private:
  static std::unique_ptr<ILogger> logger_;
};

class Foo {
  public:
   Foo(const std::string& name) : name_{name} {
       LoggerWrapper::Log(name_ + ": CTOR");
   }
   ~Foo() {
       LoggerWrapper::Log(name_ + ": DTOR");
   }
  private:
   std::string name_;
};

// declaring logger_ first causes it to be deleted AFTER foo
std::unique_ptr<ILogger> LoggerWrapper::logger_;
std::unique_ptr<Foo> foo;


int main()
{
    LoggerWrapper::SetLogger(std::make_unique<ILogger>());
    foo = std::make_unique<Foo>("Global FOO");
    
    // local variables do NOT get destroyed when calling exit!
    auto foo_local = Foo("Local FOO");

    exit(1);
}
Lexusminds
  • 335
  • 2
  • 14
  • Destruction order of static objects is the reverse of the construction order. It's not always easy to control the construction order of things with static storage duration. I would, if possible, move away from depending on the order at all. Maybe move the logging from the destructor to a `before_exit` method or something. – super Jul 09 '21 at 04:38
  • @super Thanks for your answer. Yeah, I was thinking about using a `Shutdown` method instead. Still, it bothers me not to find another solution :D – Lexusminds Jul 09 '21 at 04:39
  • @super I've heard about the reverse construction order before. That only applies across static variables though, right? E.g. static variable `A` was constructed before static variable `B`, so it is destroyed after it. Is there any rule to figure out if a static member (e.g. my logger) gets destructed before/after another, non-static class (e.g. class `Foo`) which makes use of it? – Lexusminds Jul 09 '21 at 04:41
  • [This](https://stackoverflow.com/questions/469597/destruction-order-of-static-objects-in-c) might be of some relevance, altough a bit old. – super Jul 09 '21 at 04:41
  • Yes, things with non-static storage duration will always go out of scope before any static objects gets destructed. – super Jul 09 '21 at 04:43
  • Exactly like iostreams do. The keyword is "nifty counter", look it up. – n. m. could be an AI Jul 09 '21 at 04:45
  • @super Regrading your "Yes, things with non-static storage duration will always go out of scope before any static objects gets destructed.": Doesn't that contradict what I'm experiencing right now? In my application, `class Foo` (non-static) tries to call the logger (static) which has already gotten out of scope (destructed). – Lexusminds Jul 09 '21 at 04:47
  • @Lexusminds That may only happen if you have used `new Foo` to create instance of `Foo` and have not deleted it. if it is on stack like `Foo x;` or using a smart pointer with non-static creation, it should be deleted before your static logger class. – Afshin Jul 09 '21 at 04:51
  • @Lexusminds Sounds like you are doing something strange, or interpreting the result wrong. A [mcve] would help. – super Jul 09 '21 at 05:01
  • @super @Afshin Huh, seems like you guys are right. I created a minimal example and the static logger lives until after the destruction of `Foo`. https://onlinegdb.com/oJkCb9m8w – Lexusminds Jul 09 '21 at 05:13
  • @super @Afshin Okay guys, I think I managed to reproduce my problem. My application calls `exit(1)` on shutting down. If I add that in the minimal working example, I get the "expected" wrong output where the logger gets destroyed before `Foo`. Thoughts on this? – Lexusminds Jul 09 '21 at 05:21
  • Output after adding `exit(1)` at end of `main`: ILogger CTOR > OnLogEvent: Foo CTOR > ILogger DTOR – Lexusminds Jul 09 '21 at 05:21
  • @Lexusminds The destructor of `Foo` is not called at all in that example. You can find a similar example [here](https://en.cppreference.com/w/cpp/utility/program/exit). – super Jul 09 '21 at 05:26
  • @super Good point. I adjusted the example to use a global instance of `Foo`. That then caused the DTOR to be called. Whatever I declared first gets destructed last, as you expected. Hmm I will have to see how that applies to my actual code. Unfortunately, the application's structure is a bit more complicated. – Lexusminds Jul 09 '21 at 05:38
  • I have to disagree with @super. Solving the order of initialization is trivial. How to solve the order of static initialization (both construction and destruction order).https://stackoverflow.com/a/335746/14065 – Martin York Jul 09 '21 at 05:56
  • Hey @MartinYork, thanks for your comment. So the idea is to trigger the creation of `Logger` inside `Foo`'s CTOR to make sure that `Logger` is constructed before `Foo` and therefore destroyed after it. That's an interesting approach. I'll see if I can fit it into the existing application's architecture. – Lexusminds Jul 09 '21 at 06:09
  • @Lexusminds That is correct. – Martin York Jul 09 '21 at 06:14
  • @MartinYork Not sure how that is disagreeing with what I said. Unless you mean to say that it's **always** trivial to solve the order of initialization? If only 2 objects are involved, sure. In a big project, not so much. From my experience, avoiding relying on the initialization order is almost always a good thing. – super Jul 09 '21 at 06:23
  • @super Sure. But even in a big project its simple to solve. Don't allow file scope static storage duration objects. The pattern of using function scope static storage duration object has been a standard pattern for replacing file scope global for decades now and considered best practice. This then you can always guarantee the creation order. – Martin York Jul 09 '21 at 06:32
  • @MartinYork What if we don't have full control of all the code involved? Or the uneeded extra cost of using function statics are an actual factor? I do agree that it's a much better/safer/easier route then file scope statics though in general. But I wasn't really advocating any of them to begin with. – super Jul 09 '21 at 06:43

2 Answers2

1

This is trivial.

First you don't use global static objects ( you should not be using global state like that). You use function static objects so you can control the order of creation/destruction.

So change this:

std::unique_ptr<ILogger> LoggerWrapper::logger_;
std::unique_ptr<Foo> foo;

Into:

 class GlobalLogger
 {
     public:
         ILogger& getLogger() {
             static ILogger  logger;  // if you must use unique_ptr you can do that here
             return logger;           // But much simpler to use a normal object.
         }
  };
  class GlobalFoo
  {
      public:
          Foo& getFoo() {
              // If there is a chance that foo is going to 
              // use global logger in its destructor
              // then it should simply call `GlobalLogger::getLogger()`
              // in the constructor of Foo. You then 
              // guarantee the order of creation and thus destruction.
              // Alternatively, you can call it here in thus
              // function just before the declaration of foo.
              static Foo foo;
              return foo;
          }
  };

  // Where you were using `logger_` use `GlobalLogger::getLogger()`
  // Where you were using `foo`     use `GlobalFoo::getFoo()`

If we use your original code as the starting point we can do this:

#include <iostream>
#include <memory>
#include <string>

// Please don't do this.
// This is the number one worst practice.
// https://stackoverflow.com/questions/1452721/why-is-using-namespace-std-considered-bad-practice
using namespace std;

class ILogger {
    public:
        ILogger() {
            std::cout << "ILogger CTOR" << std::endl;
        }
        ~ILogger() {
            std::cout << "ILogger DTOR" << std::endl;
        }

        virtual void OnLogEvent(const std::string& log_message) {
            std::cout << "OnLogEvent: " << log_message << std::endl;
        }
};

class LoggerWrapper
{
    // Here store the logger
    // as a static member of this private function.
    // The the SetLogger() Log() functions get this reference.
    static std::unique_ptr<ILogger>& getLogReference() {
        static std::unique_ptr<ILogger> logger;
        return logger;
    }

    public:
        static void SetLogger(std::unique_ptr<ILogger> new_logger) {
            // Save the new reference.
            getLogReference() = std::move(new_logger);
        }

        // Use the logger if it has been set.
        static void Log(const std::string& message) {
            std::unique_ptr<ILogger>& logger_ = getLogReference();
            if (!logger_) {
                // cannot log
            } else {
                logger_->OnLogEvent(message);
            }
        };
};

class Foo {
    public:
        Foo(const std::string& name) : name_{name} {
            // This calls Log()
            // Which calls getLogReference()
            // Which forces the creation of the function static
            // variable logger so it is created before this
            // object is fully initialized (if it has not already
            // been created).
            // 
            // This means this object was created after the logger
            LoggerWrapper::Log(name_ + ": CTOR");
        }
        ~Foo() {
            // Because the Log() function was called in the
            // constructor we know the loger was fully constructed first
            // thus this object will be destroyed first
            // so the logger object is guaranteed to be
            // available in this objects destructor
            // so it is safe to use.
            LoggerWrapper::Log(name_ + ": DTOR");
        }
    private:
        std::string name_;
};


std::unique_ptr<Foo>& globalFoo() {
    // foo may destroy an object created later
    // that has a destructor that calls LoggerWrapper::Log()
    // So we need to call the Log function here before foo
    // is created.
    LoggerWrapper::Log("Initializing Global foo");
    // Note: Unless somebody else has explicitly called SetLogger()
    // the above line is unlikely to log anything as the logger
    // will be null at this point.

    static std::unique_ptr<Foo> foo;
    return foo;
}

int main()
{
    LoggerWrapper::SetLogger(std::make_unique<ILogger>());
    globalFoo() = std::make_unique<Foo>("Global FOO");

    // local variables do NOT get destroyed when calling exit!
    auto foo_local = Foo("Local FOO");

    exit(1);
}
Martin York
  • 257,169
  • 86
  • 333
  • 562
  • But this still has a destructor order problem. I think the logger object should never be destroyed. – prehistoricpenguin Jul 09 '21 at 06:09
  • @prehistoricpenguin No it does not. If you think you are going to use the Global Logger in the destructor then you simply call the `GlobalLogger::getLogger()` in the constructor. This gurantees the object is constructed first which gurantees that it wil be destroyed after this object. – Martin York Jul 09 '21 at 06:12
  • Regarding your comment about using `unique_ptr`: In my application, ILogger is an abstract class which prevents me from using a reference. Also, the logger should be exchangeable as seen in the minimal working example (`SetLogger`). How would that apply to your proposed solution? – Lexusminds Jul 09 '21 at 06:19
  • @MartinYork To further clarify this (the minimal working example): There is a wrapper class `LoggerWrapper` holding a static logger `ILogger`. By calling the wrapper's static `Log` method, the application can log messages through the static logger. The static logger can be set/exchanged using the wrappers `SetLogger` method. – Lexusminds Jul 09 '21 at 06:26
  • To ` call `GlobalLogger::getLogger()`` in the constructor will guarantee the sequence, but I think it's boring and error prone. – prehistoricpenguin Jul 09 '21 at 06:26
  • @Lexusminds updated the example to allow you to set the ILogger class dynamically. – Martin York Jul 09 '21 at 06:32
  • 1
    @prehistoricpenguin You don't need to force the creation in the constructor if you follow best practive by not having file scope static storage duration objects and moving them all into file scope static storage duration obejcts. Then they are lazily constructed as needed and you can force the logger to be created first by creating it first in main. – Martin York Jul 09 '21 at 06:35
  • I agree with you, the OP's code seems not from a large project, so we don't need to consider too many details(Thread safety, multiple compiler units...) – prehistoricpenguin Jul 09 '21 at 06:38
  • @MartinYork Thanks for updating the example. Things are getting clearer now. However, I won't be able to call `getLogReference` inside `SetLogger` since `SetLogger` is a static method while `getLogReference` is not. – Lexusminds Jul 09 '21 at 06:38
  • @Lexusminds My mistake `getLogReference()` is supposed to be static. – Martin York Jul 09 '21 at 06:40
  • @MartinYork I will give your example a try now and come back to you. Thanks for taking this much time! It's really appreciated. – Lexusminds Jul 09 '21 at 06:42
  • @MartinYork Another question just came across my mind. Let's say we have classes `Foo` and `Bar`. Also let's assume that the `SetLogger` method is called twice. Once before creating `Foo`, and once before creating `Bar`. Will that still guarantee that the logger gets destructed after both classes? Or does calling `SetLogger` "reset" the creation timepoint and put it behind `Foo` and in front of `Bar` instead. – Lexusminds Jul 09 '21 at 06:58
  • @Lexusminds The guarantee holds. Static storage duration objects are destroyed in a strict order (guaranteed by the standard to be the inverse of creation). In the code above we use file scope static storage duration objects which get created on first use (ie first time the function is called in this case getLogReference()). Once created the object lives (and is not reset) until it is destroyed on application termination. – Martin York Jul 09 '21 at 07:07
  • The call to set `SetLogger()` does not create the object, it simply uses the value that was already created in `getLogReference()` (the first time it was called) and stores a logging object in this already created smart pointer. Note. It is the smart pointer that is the static storage duration object. So it is this object that is created destoyed. Your logger object is a dynamic storage duration object whose lifespan is controlled by this object. – Martin York Jul 09 '21 at 07:09
  • @Lexusminds Spotted a bug and fixed it. You can't have `foo` as a global static. As this also has a creation order and potentially destroys a dynamic object you need to force creation of the logger before `foo`. So I have moved that into function scope as well. – Martin York Jul 09 '21 at 07:26
-1

The order in which static (global) objects are created and destroyed is undefined. This leave you a few options.

  1. Don't use global objects, just raw use pointers which you can destroy yourself in the correct order.
  2. Use a pointer to your logger that you never destroy. This technically is a memory leak but the kernel will cleanup when your application exits.
  3. Use a shared_ptr for your logger. Each object that uses the logger gets a shared_ptr and the logger will be cleaned up after the last object is destroyed.
doron
  • 27,972
  • 12
  • 65
  • 103
  • The first sentence is far too broad. The order of construction of static objects that are defined in the same translation unit is the order of their definitions. The order of construction of static objects that are defined in different translation units is unspecified (not undefined; that's a different category). The order of destruction of static objects is the reverse of the order of construction (yes, the runtime has to keep track of what got constructed when in order to do this). – Pete Becker Jul 09 '21 at 12:12