Removes weak references from LoggerRepository

[ppkarwasz]

Removes weak references to Loggers in LoggerRepository. The usage of weak references in LoggerRepository might cause null to be returned by LogManager.getLogger() of all Log4j Core versions up to 2.24.1. Versions of Log4j API up to 2.24.0 did hold hard references to all the registered loggers, so the change will not alter the previous behavior.

This PR also inverts the order of the String and MessageFactory keys to the LoggerRepository multi-map to limit the number of internal maps. The external map is a WeakHashMap to allow logger-specific message factories to be GC-ed.

Closes #3143.

[ppkarwasz]

The weak reference to MessageFactory might require additional explanations. Unlike a weak reference to Logger, it will not cause null to be returned by LoggerContext#getLogger, since the method keeps a hard reference to the factory:

logging-log4j2/log4j-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java

Lines 526 to 536 in 8ee9387

	public Logger getLogger(final String name, final MessageFactory messageFactory) {
	final MessageFactory effectiveMessageFactory =
	messageFactory != null ? messageFactory : DEFAULT_MESSAGE_FACTORY;
	final Logger oldLogger = loggerRegistry.getLogger(name, effectiveMessageFactory);
	if (oldLogger != null) {
	return oldLogger;
	}
	final Logger newLogger = newInstance(name, effectiveMessageFactory);
	loggerRegistry.putIfAbsent(name, effectiveMessageFactory, newLogger);
	return loggerRegistry.getLogger(name, effectiveMessageFactory);
	}

[ppkarwasz]

In order to preserve backward compatibility and allow Loggers to be garbage-collected, I will:

• Copy LoggerContext to an internal package of log4j-core.
• Restore the usage of WeakReference<T> in the copied class.
• Remove the new methods in LoggerRepository in the original class.

[vy]

In order to preserve backward compatibility and allow Loggers to be garbage-collected, I will:

• Remove the new methods in LoggerRepository in the original class.

How will you implement this while preserving the backward compatibility?

[ppkarwasz]

In order to preserve backward compatibility and allow Loggers to be garbage-collected, I will:

• Remove the new methods in LoggerRepository in the original class.

How will you implement this while preserving the backward compatibility?

LoggerRepository#computeIfAbsent is a new method added in 2.25.0. There is no BC to preserve.

[vy]

LoggerRepository#computeIfAbsent is a new method added in 2.25.0. There is no BC to preserve.

Isn't it released with 2.24.1?

[ppkarwasz]

LoggerRepository#computeIfAbsent is a new method added in 2.25.0. There is no BC to preserve.

Isn't it released with 2.24.1?

No, the backport of #2936 for 2.24.1 is in PR #2961, which did not introduce any API changes.

[vy]

@ppkarwasz, in 8963f70, we removed 110 LoC and added 240 LoC, voiding the entire point of computeIfAbsent(). What is the rationale for this decision? Nevermind.

[garydgregory]

A complex PR without tests? Isn't it possible to test this issue to avoid regressions in the future?

[pjfanning]

Would it be possible to optionally support not having weak references? I understand why weak references are useful for edge cases like users running with dangerously low heap for their applications. There are also users who have applications that don't need weak references here and would appreciate not risking having NPEs.

[vy]

@pjfanning, making loggers garbage-collectible is not only a concern for environments with constrained memory resources. This will also greatly benefit users creating loggers programmatically.

[ppkarwasz]

@garydgregory,

A complex PR without tests? Isn't it possible to test this issue to avoid regressions in the future?

Fixed in 041e5eb

[garydgregory]

@garydgregory,

A complex PR without tests? Isn't it possible to test this issue to avoid regressions in the future?

Fixed in 041e5eb

😊 ty!

[ppkarwasz]

There are also users who have applications that don't need weak references here and would appreciate not risking having NPEs.

There should be no such problem. The only place were weak references are used in this PR is the WeakHashMap<MessageFactory, T extends ExtendedLogger> used in LoggerRegistry.

Loggers could become unreachable if the message factory becomes unreachable. However the message factory never ceases to be reachable. In a typical getLogger() call, the message factory is last used on line 166 of LoggerRegistry:

logging-log4j2/log4j-core/src/main/java/org/apache/logging/log4j/core/LoggerContext.java

Lines 533 to 535 in 8ee9387

	final Logger newLogger = newInstance(name, effectiveMessageFactory);
	loggerRegistry.putIfAbsent(name, effectiveMessageFactory, newLogger);
	return loggerRegistry.getLogger(name, effectiveMessageFactory);

logging-log4j2/log4j-api/src/main/java/org/apache/logging/log4j/spi/LoggerRegistry.java

Lines 159 to 170 in bf79552

	public @Nullable T getLogger(final String name, @Nullable final MessageFactory messageFactory) {
	requireNonNull(name, "name");
	readLock.lock();
	try {
	final @Nullable Map<MessageFactory, T> loggerByMessageFactory = loggerByMessageFactoryByName.get(name);
	final MessageFactory effectiveMessageFactory =
	messageFactory != null ? messageFactory : ParameterizedMessageFactory.INSTANCE;
	return loggerByMessageFactory == null ? null : loggerByMessageFactory.get(effectiveMessageFactory);
	} finally {
	readLock.unlock();
	}
	}

According to the blog post shared by Volkan this means that the message factory will be reachable up to line 166. However, that line returns a Logger that has a reference to the message factory, so the message factory becomes reachable again. Worst case scenario, we might loose the entry in the WeakHashMap, but not the Logger.

[ppkarwasz]

@vy,

Do you have any comments regarding InternalLoggerRepository? The class should be similar to the current LoggerRegistry with two significant changes:

• All unused methods are removed.
• The order of the keys in inverted: the external map uses MessageFactory, since there are less message factories than logger names, which reduces the number of internal maps.