Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SDK] Fix lifetime of GlobalLogHandler #3221

Merged
merged 8 commits into from
Jan 15, 2025
Merged

[SDK] Fix lifetime of GlobalLogHandler #3221

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
efa496c
Try to fix lifetime of GlobalLogHandler
owent Dec 24, 2024
1dba952
Ignore global handle when singlethon is destroyed
owent Dec 31, 2024
2bb008b
Fix compiling
owent Dec 31, 2024
2245d89
Merge branch 'main' into try_fix_lifetime_of_global_log_handler
owent Jan 8, 2025
09225ae
Merge branch 'main' into try_fix_lifetime_of_global_log_handler
owent Jan 13, 2025
571fb9a
Merge remote-tracking branch 'github/main' into try_fix_lifetime_of_g…
owent Jan 14, 2025
5ac5e4f
Use static variable to check if global log handle is destroyed.
owent Jan 15, 2025
f7d1993
Remove `GlobalLogHandlerData` and move functions of `GlobalLogHandler…
owent Jan 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 53 additions & 24 deletions sdk/include/opentelemetry/sdk/common/global_log_handler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,15 +93,37 @@ class NoopLogHandler : public LogHandler
*/
class GlobalLogHandler
{
private:
struct GlobalLogHandlerData
{
nostd::shared_ptr<LogHandler> handler;
LogLevel log_level;
bool destroyed;

GlobalLogHandlerData();
~GlobalLogHandlerData();

GlobalLogHandlerData(const GlobalLogHandlerData &) = delete;
GlobalLogHandlerData(GlobalLogHandlerData &&) = delete;

GlobalLogHandlerData &operator=(const GlobalLogHandlerData &) = delete;
GlobalLogHandlerData &operator=(GlobalLogHandlerData &&) = delete;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having one structure with all the state is much better than having this from the existing code:

std::pair<nostd::shared_ptr<LogHandler>, LogLevel>

I would be even more aggressive with the cleanup, and:

  • move the declaration of struct GlobalLogHandlerData to the cc file, so it is never seen in the header
  • remove GetHandlerAndLevel() entirely
  • move all the implementation, like code for GetLogLevel() and SetLogLevel(), from the header to the cc file, leaving only declarations in the header (without inline implementation)

And then, as @sjinks suggested, change the bool destroyed to a plain static in the cc file, instead of a static member. Given how this code can be executed by multiple threads, we may need an atomic boolean as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On top of the issue of random order of static initialization and destruction, there is also the issue of multiple threads making calls to the global handler, and races:

  • a thread still tries to export data and fails, writing an error to the log
  • another thread change the log handler to add/remove a custom handler on startup or cleanup

This is unsafe at the moment, so we may need a mutex to protect all operations on GlobalLogHandlerData::handle

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last, instead of returning a shared pointer with the handler in each call to GetLogHandler(), which imply a counter increment and decrement, we could have a GlobalLogHandler::Handle() method, to be invoked by OTEL_INTERNAL_LOG_DISPATCH, to actually write to the log.

This method would lock the mutex, to prevent races with SetLogHandler().

Currently, I am not convinced the shared pointer returned by GetLogHandler() is safe, when SetLogHandler is called concurrently.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having one structure with all the state is much better than having this from the existing code:

std::pair<nostd::shared_ptr<LogHandler>, LogLevel>

I would be even more aggressive with the cleanup, and:

  • move the declaration of struct GlobalLogHandlerData to the cc file, so it is never seen in the header
  • remove GetHandlerAndLevel() entirely
  • move all the implementation, like code for GetLogLevel() and SetLogLevel(), from the header to the cc file, leaving only declarations in the header (without inline implementation)

And then, as @sjinks suggested, change the bool destroyed to a plain static in the cc file, instead of a static member. Given how this code can be executed by multiple threads, we may need an atomic boolean as well.

Done

On top of the issue of random order of static initialization and destruction, there is also the issue of multiple threads making calls to the global handler, and races:

This checking is used when the whole program is exiting and some singleton objects or global objects call OTEL_INTERNAL_LOG_* when destroying. I think the behaviour should be UB and this checking is just used to prevent crash.If user want to write logs when deinitializing, they should do it before program exiting.

Changing value of a shared_ptr is not thread-safe, but increase and descrease shared_ptr's counter is thread-safe. Return a shared pointer will only increase the counter. This PR do not solve the thread-safe problem here, but we can discuss, should we use and pay for a lock to keep thread-safe here?I think SetLogHandler will not be call frequently.


public:
/**
* Returns the singleton LogHandler.
*
* By default, a default LogHandler is returned.
*/
static inline const nostd::shared_ptr<LogHandler> &GetLogHandler() noexcept
static inline nostd::shared_ptr<LogHandler> GetLogHandler() noexcept
{
return GetHandlerAndLevel().first;
if OPENTELEMETRY_UNLIKELY_CONDITION (GetHandlerAndLevel().destroyed)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I interpret the standard correctly, this may be an undefined behavior.

[basic.life] says that "[t]he lifetime of an object o of type T ends when: <...> if T is a class type, the destructor call starts", and "after the lifetime of an object has ended and before the storage which the object occupied is reused or released, any pointer that represents the address of the storage location where the object will be or was located may be used but only in limited ways. <...> The program has undefined behavior if: <...> the pointer is used to access a non-static data member or call a non-static member function of the object".

Copy link
Member Author

@owent owent Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's a trick that the memory address of a static local variable will always be available in practice. So we can visit it even if it's destructed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this is a UB, the compiler can optimize away the destroyed = true; statement in the destructor. Clang does not, but gcc does:

#include <iostream>

class test {
public:
    bool destroyed;

    test() : destroyed(false)
    {}

    void hello()
    {
        std::cout << "Hello\n";
    }

    ~test()
    {
        destroyed = true;
    }
};

int main()
{
    test t;
    t.hello();
    t.~test();

    return (int)t.destroyed;
}
main:
.LFB2064:
        .cfi_startproc
        endbr64
        sub     rsp, 8
        .cfi_def_cfa_offset 16
        mov     edx, 6
        lea     rsi, .LC0[rip]
        lea     rdi, _ZSt4cout[rip]
        call    _ZSt16__ostream_insertIcSt11char_traitsIcEERSt13basic_ostreamIT_T0_ES6_PKS3_l@PLT
        xor     eax, eax
        add     rsp, 8
        .cfi_def_cfa_offset 8
        ret

In other words, the result is not guaranteed if you rely upon undefined behavior.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I checked our singlehon codes before, it use a static variable but not the member variable to check if the singlehon object is destroyed, and it's not effected by optimization of both gcc and clang. I will modify the codes and try to add a unit test later.

{
return nostd::shared_ptr<LogHandler>();
}

return GetHandlerAndLevel().handler;
}

/**
Expand All @@ -111,25 +133,33 @@ class GlobalLogHandler
*/
static inline void SetLogHandler(const nostd::shared_ptr<LogHandler> &eh) noexcept
{
GetHandlerAndLevel().first = eh;
if OPENTELEMETRY_UNLIKELY_CONDITION (GetHandlerAndLevel().destroyed)
{
return;
}

GetHandlerAndLevel().handler = eh;
}

/**
* Returns the singleton log level.
*
* By default, a default log level is returned.
*/
static inline LogLevel GetLogLevel() noexcept { return GetHandlerAndLevel().second; }
static inline LogLevel GetLogLevel() noexcept { return GetHandlerAndLevel().log_level; }

/**
* Changes the singleton Log level.
* This should be called once at the start of application before creating any Provider
* instance.
*/
static inline void SetLogLevel(LogLevel level) noexcept { GetHandlerAndLevel().second = level; }
static inline void SetLogLevel(LogLevel level) noexcept
{
GetHandlerAndLevel().log_level = level;
}

private:
static std::pair<nostd::shared_ptr<LogHandler>, LogLevel> &GetHandlerAndLevel() noexcept;
static GlobalLogHandlerData &GetHandlerAndLevel() noexcept;
};

} // namespace internal_log
Expand All @@ -142,24 +172,23 @@ OPENTELEMETRY_END_NAMESPACE
* To ensure that GlobalLogHandler is the first one to be initialized (and so last to be
* destroyed), it is first used inside the constructors of TraceProvider, MeterProvider
* and LoggerProvider for debug logging. */
#define OTEL_INTERNAL_LOG_DISPATCH(level, message, attributes) \
do \
{ \
using opentelemetry::sdk::common::internal_log::GlobalLogHandler; \
using opentelemetry::sdk::common::internal_log::LogHandler; \
if (level > GlobalLogHandler::GetLogLevel()) \
{ \
break; \
} \
const opentelemetry::nostd::shared_ptr<LogHandler> &log_handler = \
GlobalLogHandler::GetLogHandler(); \
if (!log_handler) \
{ \
break; \
} \
std::stringstream tmp_stream; \
tmp_stream << message; \
log_handler->Handle(level, __FILE__, __LINE__, tmp_stream.str().c_str(), attributes); \
#define OTEL_INTERNAL_LOG_DISPATCH(level, message, attributes) \
do \
{ \
using opentelemetry::sdk::common::internal_log::GlobalLogHandler; \
using opentelemetry::sdk::common::internal_log::LogHandler; \
if (level > GlobalLogHandler::GetLogLevel()) \
{ \
break; \
} \
opentelemetry::nostd::shared_ptr<LogHandler> log_handler = GlobalLogHandler::GetLogHandler(); \
if (!log_handler) \
{ \
break; \
} \
std::stringstream tmp_stream; \
tmp_stream << message; \
log_handler->Handle(level, __FILE__, __LINE__, tmp_stream.str().c_str(), attributes); \
} while (false);

#define OTEL_INTERNAL_LOG_GET_3RD_ARG(arg1, arg2, arg3, ...) arg3
Expand Down
16 changes: 13 additions & 3 deletions sdk/src/common/global_log_handler.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,20 @@ void NoopLogHandler::Handle(LogLevel,
const sdk::common::AttributeMap &) noexcept
{}

std::pair<nostd::shared_ptr<LogHandler>, LogLevel> &GlobalLogHandler::GetHandlerAndLevel() noexcept
GlobalLogHandler::GlobalLogHandlerData::GlobalLogHandlerData()
: handler(nostd::shared_ptr<LogHandler>(new DefaultLogHandler)),
log_level(LogLevel::Warning),
destroyed(false)
{}

GlobalLogHandler::GlobalLogHandlerData::~GlobalLogHandlerData()
{
destroyed = true;
}

GlobalLogHandler::GlobalLogHandlerData &GlobalLogHandler::GetHandlerAndLevel() noexcept
{
static std::pair<nostd::shared_ptr<LogHandler>, LogLevel> handler_and_level{
nostd::shared_ptr<LogHandler>(new DefaultLogHandler), LogLevel::Warning};
static GlobalLogHandlerData handler_and_level;
return handler_and_level;
}

Expand Down
Loading