Opened 7 years ago

Last modified 4 years ago

#2399 new defect

make Logger::getLoggerPtr() thread safe

Reported by: jinmei Owned by:
Priority: medium Milestone: Outstanding Tasks
Component: logging Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 3 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

If I understand it correctly, the delayed initialization of
the Logger class is not thread safe:

    LoggerImpl* getLoggerPtr() {
        if (!loggerptr_) {
            initLoggerImpl();
        }
        return (loggerptr_);
    }

initLoggerImpl() could be called by multiple threads at the same time,
and due to this:

void Logger::initLoggerImpl() {
    if (isLoggingInitialized()) {
        loggerptr_ = new LoggerImpl(name_);
    } else {
        isc_throw(LoggingNotInitialized, "attempt to access logging function "
                  "before logging has been initialized");
    }
}

creating LoggerImpl could be done multiple times.

(btw: the implementation of isLoggingInitialized could also be
controversial, but this is probably safe in practice due to the
assumption of initLogger()).

The (bad) effect of this is probably not catastrophic: maybe it's just
losing some fixed (and small) amount of memory in the worst case, at
least in practice. But I may be wrong, and in any case such leak
isn't good anyway.

One way of fixing this would be to introduce some global mutex that is
initialized at the time of initLogger(), and do something like this:

    if (isLoggingInitialized()) {
        Locker locker(the_global_mutex);
        if (loggerptr_ == NULL) {
            loggerptr_ = new LoggerImpl(name_);
        }

Subtickets

Change History (7)

comment:1 follow-up: Changed 7 years ago by vorner

And how do you initialize the mutex without the static initialization fiasco in
a thread-safe manner?

One way to solve the problem could be to make sure the logging is initialized
before we start the thread.

Or is it so that there are many Logger objects, each of them having this
problem? Then we probably would have to do it the combined way ‒ have single
global mutex and make sure it is initialized in the non-threaded startup of
application dynamically.

comment:2 in reply to: ↑ 1 Changed 7 years ago by jinmei

Replying to vorner:

And how do you initialize the mutex without the static initialization fiasco in
a thread-safe manner?

As briefly explained in the description, in initLogger(). In fact,
this is *the* trick with which avoid all sorts of initialization
fiasco in the log library (it heavily relies on singleton/static
things. It's API contract basis (on the assumption that the
application calls initLogger() before doing anything with log library)
and in that sense not a perfect solution in theory, but since we
already rely on it I don't see a reason for not using this for this
case too.

One way to solve the problem could be to make sure the logging is initialized
before we start the thread.

Or is it so that there are many Logger objects, each of them having this
problem? Then we probably would have to do it the combined way ‒ have single
global mutex and make sure it is initialized in the non-threaded startup of
application dynamically.

There are many (at least multiple for normal apps) logger objects, and
its full initialization is delayed until it's used first time via
LOG_INFO() etc. And, actually, if I understand it correctly, the
"combined way" is exactly what I suggested here.

comment:3 Changed 7 years ago by shane

Boost appears to have support for doing something once:

http://www.boost.org/doc/libs/1_32_0/doc/html/call_once.html

comment:4 Changed 7 years ago by muks

Such a mutex is in trac2198_4, initialized by LoggerManager::init() which is called by initLogger().

comment:5 Changed 5 years ago by tomek

  • Milestone set to Remaining BIND10 tickets

comment:6 Changed 4 years ago by tomek

  • Milestone changed from Remaining BIND10 tickets to DHCP Outstanding Tasks

As part of bug scrubbing during Kea 1.0, we decided to move a number of tickets to DHCP Outstanding.

comment:7 Changed 4 years ago by tomek

  • Milestone changed from DHCP Outstanding Tasks to Outstanding Tasks

Milestone renamed

Note: See TracTickets for help on using tickets.