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

Try to fix lifetime of GlobalLogHandler #3221

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+66 −27
Open

Try to fix lifetime of GlobalLogHandler #3221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 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
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;
};

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.

{
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