Consider removing accessors from `MDC <-> ThreadContext` bridges

[ppkarwasz]

Currently we have two bridges between MDC and ThreadContext:

• MDCContextMap in log4j-to-slf4j that forwards ThreadContext calls to MDC,
• Log4jMDCAdapter in log4j-slf4j2-impl and log4j-slf4j-impl that forwards MDC calls to ThreadContext.

While the mutator methods of these bridges are required by user code, the accessors are basically useless:

• Logback does not call any ThreadContext accessor,
• Log4j Core does not call any MDC accessor.

The accessors are only used by third-party integrators like the Context Propagation Library (see Slf4jThreadLocalAccessor for example).

In view of a future integration with context-propagation I would propose to:

• modify MDCContextMap accessors to always return null in the get and getImmutableMapOrNull and return a new HashMap in the getCopy method. These values are allowed by the ThreadContextMap contract,
• modify Log4jMDCAdapter accessors to always return null. This value is allowed by the MDCAdapter contract.

These changes will guarantee that at least one of MDC#getCopyOfContextMap and ThreadContextMap#getImmutableMapOrNull will return null. According to the semantics of the context-propagation project, null means "don't propagate" and will prevent the propagation of MDC for Log4j Core users and the propagation of ThreadContext for Logback users.

Remark: we can also introduce a log4j.threadContext.map.bridgeAccessors property in Log4j 3 and log4j2.threadContextMapBridgeAccessors property in Log4j 2 to allow users that use MDC and ThreadContext for non-logging purposes to restore the previous behavior.

[jvz]

This is a solid proposal I think.

[vy]

[I am not much familiar with the part of the code base you are talking about, hence apologies in advance if I say something stupid.]

will prevent the propagation of MDC for Log4j Core users and the propagation of ThreadContext for Logback users.

@ppkarwasz, this is the core problem this problem is attacking to, right? That is, avoiding redundant propagation when a particular bridge is in play.

In view of a future integration with context-propagation I would propose to:

• modify MDCContextMap ...
• modify Log4jMDCAdapter ...

I would greatly appreciate it if you can thoroughly document the rationale in the touched parts of the code base while implementing these changes.

[ppkarwasz]

@ppkarwasz, this is the core problem this problem is attacking to, right? That is, avoiding redundant propagation when a particular bridge is in play.

Yes, my main motivation is to reduce the overhead of context propagation. See also the discussion at micrometer-metrics/context-propagation#108.

Unfortunately I don't think we can implement these changes by default, since I found examples of legitimate usages of ThreadContext accessors. E.g.:

Map<String, String> oldMap = ThreadContext.getImmutableMapOrNull();
try {
    ThreadContext.put("key", "value");
    ...
} finally {
    ThreadContext.clear();
    if (oldMap != null) {
        ThreadContext.putAll(oldMap);
    }
}

or

String oldValue = ThreadContext.get("key");
try {
    ThreadContext.put("key", "value");
    ...
} finally {
    if (oldValue == null) {
        ThreadContext.remove("key");
    } else {
        ThreadContext.put("key", oldValue);
    }
}

ThreadContext API users unfortunately must use accessors or need to switch to CloseableThreadContext.

[rgoers]

I still don't get what the perceived benefit is here. What problem does this solve?

[ppkarwasz]

@rgoers,

The problem this tries to solve is to prevent context propagators from copying both MDC#getCopyOfContextMap() and ThreadContext#getImmutableMapOrNull() between threads.

Since these two methods return the same data, it would be nice to only propagate one of them: MDC if the user uses Logback and ThreadContext if the user uses Log4j Core.

In the case of MDC and ThreadContext propagating both of the is only not effective, but for other logging backends it might even be wrong. For example org.jboss.logmanager.MDC supports objects. If we propagate its data using ThreadContext we will loose information.

[rgoers]

@ppkarwasz Thanks for that. That definitely sounds like something should be done to address it.