unmodifiable MDC property map

[ceki]

Discussion: unmodifiable MDC property map

This note is an invitation to discuss the change in commit 76c73d1 (add MinimalUnmodifiableMap and return it in LogbackMDCAdapter#getPropertyMap) and whether getPropertyMap() should ever return UnmodifiableWrappingMap instead of, or in addition to, MinimalUnmodifiableMap.

Comments and counter-arguments are welcome.

What changed

LogbackMDCAdapter.getPropertyMap() used to return:

Collections.unmodifiableMap(new HashMap<String, String>(current));

It now returns:

readOnlyMap = new MinimalUnmodifiableMap(current);

The intended contract is the same: an unmodifiable Map<String, String> snapshot. Later put/remove on the thread's MDC must not change a map already handed out. The snapshot is cached in a ThreadLocal until the next write.

MinimalUnmodifiableMap is a public Map<String, String>. It copies entries into two parallel String arrays (keys and values). get is a linear on indexOf method calls. It is listed on the HardenedLoggingEventInputStream whitelist. Serialization writes length plus each String, not a raw array type.

The same commit also adds UnmodifiableWrappingMap. It is not used on the getPropertyMap() path today. Its role is discussed below.

Why not Collections.unmodifiableMap as the public type?

Collections.unmodifiableMap(Map) is the JDK factory. The concrete type it returns is java.util.Collections.UnmodifiableMap, a private (package-private) nested class of java.util.Collections. Callers cannot subclass it, or invoke its constructor.

The main reason for leaving that type behind is decoding. A private unmodifiable map cannot be reconstructed if the decoder needs to add entries piecemeal via put(). put throws UnsupportedOperationException, and there is no public constructor to wrap a map that the decoder has already filled. Frameworks that instantiate the runtime class and populate it with put (or that cannot call Collections.unmodifiableMap because they only see the private class name) fail.

That is the primary motivation for MinimalUnmodifiableMap instead of shipping Collections$UnmodifiableMap as the MDC snapshot type.

Owning a public class also makes a serialization whitelist entry and tests straightforward. Those are secondary to the decode/put problem.

UnmodifiableWrappingMap as a public stand-in

UnmodifiableWrappingMap<K, V> was added to solve the same decode problem without forcing the array layout.

It is a public, generic unmodifiable view over an existing map, in the same spirit as the private Collections.UnmodifiableMap:

• constructor: UnmodifiableWrappingMap(Map<? extends K, ? extends V> m) (null rejected);
• reads (get, size, containsKey, …) delegate to the backing map;
• put / remove / clear throw UnsupportedOperationException;
• entrySet() wraps the backing set so Entry.setValue cannot mutate the backing map.

A decoder can therefore:

1. allocate a mutable HashMap;
2. put entries one by one;
3. pass that map to new UnmodifiableWrappingMap<>(map).

That sequence is impossible with Collections.UnmodifiableMap itself, because the class is private.

The wrapper does not copy. Mutating the backing map remains visible through the view. Callers that need a snapshot must pass a copy, for example new UnmodifiableWrappingMap<>(new HashMap<>(current)).

MinimalUnmodifiableMap vs UnmodifiableWrappingMap

Both are public, unmodifiable maps we own. They differ in layout and in who is responsible for the snapshot.

	MinimalUnmodifiableMap	UnmodifiableWrappingMap<K,V>
Types	Map<String, String> only	generic K, V
Storage	copy into String[] keys and String[] values	reference to an existing map
Snapshot	the copy is the snapshot; later mutation of the source is not visible	view: mutating the backing map is visible unless the caller copies first
get	linear scan (indexOf)	delegates to the backing map (HashMap.get when the backing map is a HashMap)
entrySet	index iterator	wraps the backing entrySet; each next() is an immutable entry
Empty	shared emptyMap()	wrap whatever empty map the caller supplies
Decode via put	constructor copies from a map the decoder can fill with put	constructor wraps a map the decoder can fill with put
Intended role	MDC property map	public replacement for Collections.unmodifiableMap when a view is enough

On getPropertyMap(), wrapping the live read-write HashMap would break snapshot isolation. Using wrapping there would still mean:

new UnmodifiableWrappingMap<>(new HashMap<String, String>(current));

That is a full HashMap copy (table + Nodes) plus a wrapper — the same shape as the old Collections.unmodifiableMap(new HashMap<>(...)) code, with a public wrapper type.

MinimalUnmodifiableMap(current) copies once into two compact arrays: one object + two arrays, no HashMap table, no Nodes.

MDCConverter uses get(key) when a key is configured (%X{foo}) and entrySet() when it is not (%X). Neither requires hashing at typical MDC sizes (a handful of keys: request id, user, tenant).

Size-based choice?

Should getPropertyMap() return MinimalUnmodifiableMap for very small maps and UnmodifiableWrappingMap (over a HashMap copy) otherwise?

This is the question we would like to discuss.

A possible argument for a switch: if some applications put tens of MDC keys and use keyed %X{key} on a hot logger, linear get could lose to HashMap.get. ArrayMap-style structures often lose to hashing only past roughly 8–16 keys.

A possible argument against:

1. A snapshot is required either way. Wrapping only gets O(1) get after a HashMap copy, which is heavier to build than two String[]s for realistic MDC sizes.
2. Typical MDC size is small. Construction is once per MDC mutation (then cached). For small n, that GC cost likely dominates any lookup difference. Full-map iteration allocates an entry object per key in both implementations.
3. A threshold K is a magic number. Callers would see two runtime types. Hardened deserialization would need UnmodifiableWrappingMap (and its backing HashMap) on the whitelist as well.

Is there a workload where large MDCs plus keyed lookup would justify the extra path? If so, what threshold, and should both types stay on the hardened whitelist?

Open points

• Is replacing Collections$UnmodifiableMap with a public type the right fix for piecemeal put decoding, and does UnmodifiableWrappingMap cover that case well enough for non-MDC maps?
• For MDC snapshots specifically, is the array-backed
  MinimalUnmodifiableMap preferable to wrapping a HashMap copy for all sizes, or only below some n?
• Should getPropertyMap() ever return UnmodifiableWrappingMap, or should that class remain a general-purpose view unused on this path?

Please weigh in.

[saivarun1410]

Hi Ceki,

My preference would be to keep MinimalUnmodifiableMap as the only type returned from getPropertyMap(), and leave UnmodifiableWrappingMap as a general-purpose public view outside that path.

The MDC contract here is a snapshot. A wrapping map is only safe after copying into a HashMap, which brings back the table/node allocation cost and adds another wrapper. For the usual small MDC sizes—request ID, tenant, user, trace IDs—the compact array representation seems like the better default, especially since the snapshot is cached until the next write.

I would also avoid a size-based switch unless a benchmark shows a meaningful benefit in a real workload. It introduces a magic threshold, exposes two runtime implementations, and expands the hardened-deserialization surface. The linear lookup cost is likely insignificant for typical MDC cardinalities, while the allocation savings happen on every MDC mutation.

A small JMH benchmark across 1, 4, 8, 16, and 32 entries would be useful: keyed %X{key} lookup, full %X iteration, and write-plus-snapshot cost. If a larger-map use case proves important, that data can justify a separate optimization without complicating the normal path today.

Best Regards,
Sai Varun Thupakula