apache / apache/logging-log4j2

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

Open
#2,499 6 comments 0 reactions 1 assignee Claimed by @ppkarwasz View on GitHub
Dominant language
Java
Stars
3.6k
Forks
1.7k
Avg merge
21h 30m
Merged PRs (30d)
27

Description

Currently we have two bridges between `MDC` and `ThreadContext`:

- [`MDCContextMap`](https://github.com/apache/logging-log4j2/blob/2.x/log4j-to-slf4j/src/main/java/org/apache/logging/slf4j/MDCContextMap.java) in `log4j-to-slf4j` that forwards `ThreadContext` calls to `MDC`,
- [`Log4jMDCAdapter`](https://github.com/apache/logging-log4j2/blob/2.x/log4j-slf4j2-impl/src/main/java/org/apache/logging/slf4j/Log4jMDCAdapter.java) 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](https://github.com/micrometer-metrics/context-propagation) (see [`Slf4jThreadLocalAccessor`](https://github.com/micrometer-metrics/context-propagation/blob/main/context-propagation/src/main/java/io/micrometer/context/integration/Slf4jThreadLocalAccessor.java) 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`](https://logging.apache.org/log4j/2.x/javadoc/log4j-api/org/apache/logging/log4j/spi/ThreadContextMap.html) contract,
- modify `Log4jMDCAdapter` accessors to always return `null`. This value is allowed by the [`MDCAdapter`](https://www.slf4j.org/api/org/slf4j/MDC.html) 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.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.